From cea631cd53c3bfd64693ea0a37c8b5e54aa9b511 Mon Sep 17 00:00:00 2001 From: Josef Andersson Date: Fri, 17 Jul 2026 08:28:09 +0200 Subject: [PATCH 1/2] fix(gcd): unify story credit search and honour linked-credits-only One path for all six story credit fields, replacing the near-identical blocks that hid the editing-credit bug. Two behaviour changes fall out: linked-credits-only with no linked match now returns nothing for every credit field, not just issue editing; and story editing splits on ';' like the others. Signed-off-by: Josef Andersson --- apps/gcd/views/search.py | 85 ++++++++++++++++++++++------------------ 1 file changed, 46 insertions(+), 39 deletions(-) diff --git a/apps/gcd/views/search.py b/apps/gcd/views/search.py index 87b05193a..a33a3fee1 100644 --- a/apps/gcd/views/search.py +++ b/apps/gcd/views/search.py @@ -2216,6 +2216,35 @@ def handle_numbers(field, data, prefix): return reduce(lambda x, y: x | y, q_or_only) +def _linked_story_ids(creator, credit_field, op): + creator_q_obj = Q(**{'name__%s' % op: creator}) + creator_q_obj |= Q(**{ + 'creator__gcd_official_name__%s' % op: creator, + }) + creators = CreatorNameDetail.objects.filter(creator_q_obj).values_list( + 'id', flat=True) + return list(Story.objects.filter( + credits__creator__id__in=creators, + credits__deleted=False, + credits__credit_type__id=CREDIT_TYPES[credit_field]) + .values_list('id', flat=True)) + + +def _linked_story_credit_filters(search_value, credit_field, prefix, op): + creator_names = [creator.strip() + for creator in search_value.split(';') + if creator.strip()] + if not creator_names: + return [Q(**{'%sid__in' % prefix: [-1]})] + + filters = [] + for creator in creator_names: + # Keep unmatched terms represented so AND searches cannot drop them. + stories = _linked_story_ids(creator, credit_field, op) or [-1] + filters.append(Q(**{'%sid__in' % prefix: stories})) + return filters + + def search_stories(data, op): """ Build the query against the story table. As it is the lowest @@ -2229,45 +2258,23 @@ def search_stories(data, op): linked_credits_q_objs = [] q_and_only = [] - for field in ('script', 'pencils', 'inks', 'colors', 'letters'): - if data[field]: - text_credits_q_objs.append( - Q(**{'%s%s__%s' % (prefix, field, op): data[field]})) - for creator in data[field].split(';'): - creator = creator.strip() - creator_q_obj = Q(**{'name__%s' % (op): creator}) - creator_q_obj |= Q(**{'creator__gcd_official_name__%s' % (op): - creator}) - creators = list(CreatorNameDetail.objects.filter(creator_q_obj) - .values_list('id', flat=True)) - stories = list(Story.objects.filter( - credits__creator__id__in=creators, - credits__deleted=False, - credits__credit_type__id=CREDIT_TYPES[field]) - .values_list('id', flat=True)) - if (stories): - linked_credits_q_objs.append( - (Q(**{'%sid__in' % (prefix): stories})) - ) - - if data['story_editing']: - text_credits_q_objs.append(Q(**{'%sediting__%s' % (prefix, op): - data['story_editing']})) - - creator_q_obj = Q(**{'name__%s' % (op): data['story_editing']}) - creator_q_obj |= Q(**{'creator__gcd_official_name__%s' % (op): - data['story_editing']}) - creators = list(CreatorNameDetail.objects.filter(creator_q_obj) - .values_list('id', flat=True)) - stories = list(Story.objects.filter( - credits__creator__id__in=creators, - credits__deleted=False, - credits__credit_type__id=CREDIT_TYPES['editing']) - .values_list('id', flat=True)) - if (stories): - linked_credits_q_objs.append( - (Q(**{'%sid__in' % (prefix): stories})) - ) + story_credit_fields = { + 'script': 'script', + 'pencils': 'pencils', + 'inks': 'inks', + 'colors': 'colors', + 'letters': 'letters', + 'story_editing': 'editing', + } + for search_field, credit_field in story_credit_fields.items(): + search_value = data[search_field] + if not search_value: + continue + + text_credits_q_objs.append( + Q(**{'%s%s__%s' % (prefix, credit_field, op): search_value})) + linked_credits_q_objs.extend(_linked_story_credit_filters( + search_value, credit_field, prefix, op)) for field in ('title', 'first_line', 'job_number', 'characters', 'synopsis', 'reprint_notes', 'notes'): From 594c1822af2ed5e1407797354106872f8a9ce0cf Mon Sep 17 00:00:00 2001 From: Josef Andersson Date: Fri, 17 Jul 2026 08:28:09 +0200 Subject: [PATCH 2/2] test(gcd): cover advanced search credit behaviour Credit-type matching, deleted credits, semicolon AND, empty segments, credit source selection, and issue vs story editing. Signed-off-by: Josef Andersson --- apps/gcd/tests/test_search.py | 52 ++++++- apps/gcd/tests/test_search_credit_sources.py | 131 ++++++++++++++++++ apps/gcd/tests/test_search_linked_only.py | 134 +++++++++++++++++++ 3 files changed, 312 insertions(+), 5 deletions(-) create mode 100644 apps/gcd/tests/test_search_credit_sources.py create mode 100644 apps/gcd/tests/test_search_linked_only.py diff --git a/apps/gcd/tests/test_search.py b/apps/gcd/tests/test_search.py index 2242f855e..4b19c6a18 100644 --- a/apps/gcd/tests/test_search.py +++ b/apps/gcd/tests/test_search.py @@ -11,7 +11,7 @@ from apps.gcd.models.story import CREDIT_TYPES from apps.gcd.views.search import do_advanced_search -CREATOR = 'Search Person' +CREATOR = 'Test Person A' # The advanced-search field for each credit type. 'editing' is reached # through story_editing; the rest share their name with the credit. @@ -43,13 +43,13 @@ def credited_stories(db): story_type = StoryType.objects.get_or_create( name='test-sequence', defaults={'sort_code': 99001})[0] country = Country.objects.get_or_create( - id=902, defaults={'code': 'zs', 'name': 'Searchland'})[0] + id=902, defaults={'code': 'zs', 'name': 'Testland B'})[0] language = Language.objects.get_or_create( - id=902, defaults={'code': 'zs', 'name': 'Searchish'})[0] + id=902, defaults={'code': 'zs', 'name': 'Testish B'})[0] publisher = Publisher.objects.create( - name='Search Publisher', country=country, year_began=1950) + name='Test Publisher', country=country, year_began=1950) series = Series.objects.create( - name='Search Series', sort_name='Search Series', year_began=1950, + name='Test Series', sort_name='Test Series', year_began=1950, country=country, language=language, publisher=publisher, is_comics_publication=True, has_gallery=False, publication_dates='1950') @@ -81,3 +81,45 @@ def test_credit_search_matches_only_its_own_credit_type(credit, matched = advanced_search(**{SEARCH_FIELD[credit]: CREATOR}) assert matched == {credited_stories[credit].id} + + +def test_credit_search_ignores_deleted_credits(credited_stories): + StoryCredit.objects.filter( + story=credited_stories['inks']).update(deleted=True) + + assert advanced_search(inks=CREATOR) == set() + + +@pytest.mark.parametrize('credit', sorted(SEARCH_FIELD)) +def test_semicolon_separated_creators_must_all_be_credited( + credit, credited_stories): + # 'A; B' means stories credited to both A and B, not either of them. + solo = credited_stories[credit] + shared = Story.objects.create( + issue=solo.issue, type=solo.type, sequence_number=1) + credit_type = CreditType.objects.get(id=CREDIT_TYPES[credit]) + for name in (CREATOR, 'Test Person B'): + creator = Creator.objects.create( + gcd_official_name=name, sort_name=name) + StoryCredit.objects.create( + creator=CreatorNameDetail.objects.create( + name=name, creator=creator, + in_script=Script.objects.get(id=Script.LATIN_PK)), + credit_type=credit_type, story=shared) + + matched = advanced_search( + **{SEARCH_FIELD[credit]: '%s; Test Person B' % CREATOR}) + + assert matched == {shared.id} + + +@pytest.mark.parametrize('credit', sorted(SEARCH_FIELD)) +def test_empty_creator_segments_do_not_match_every_credit( + credit, credited_stories): + matched = advanced_search(**{SEARCH_FIELD[credit]: ';'}) + + assert matched == set() + + +def test_unmatched_creator_returns_no_stories(credited_stories): + assert advanced_search(script='Test Person Absent') == set() diff --git a/apps/gcd/tests/test_search_credit_sources.py b/apps/gcd/tests/test_search_credit_sources.py new file mode 100644 index 000000000..cc71748d8 --- /dev/null +++ b/apps/gcd/tests/test_search_credit_sources.py @@ -0,0 +1,131 @@ +# -*- coding: utf-8 -*- +import pytest +from django.contrib.auth.models import AnonymousUser +from django.test import RequestFactory + +from apps.stddata.models import Country, Language, Script +from apps.gcd.models import ( + Publisher, Series, Issue, Story, StoryType, StoryCredit, CreditType, +) +from apps.gcd.models.creator import Creator, CreatorNameDetail +from apps.gcd.models.issue import IssueCredit +from apps.gcd.models.story import CREDIT_TYPES +from apps.gcd.views.search import do_advanced_search + +CREATOR = 'Test Person A' + +# credit_is_linked values, exactly as the advanced-search dropdown sends them. +# LINKED_ONLY matches only linked credit records, TEXT_CREDITS_ONLY only the +# free-text credit fields, and BOTH matches either. +LINKED_ONLY = '' # dropdown "linked credits only" +BOTH = 'True' # dropdown "both linked and text credits" +TEXT_CREDITS_ONLY = 'False' # dropdown "text credits only" + + +def advanced_search(**fields): + request = RequestFactory().get( + '/search/advanced/process/', + dict({'target': 'sequence', 'method': 'icontains'}, **fields)) + request.user = AnonymousUser() + items, _target = do_advanced_search(request) + return set(items.values_list('id', flat=True)) + + +@pytest.fixture +def world(db): + script = Script.objects.get_or_create( + id=Script.LATIN_PK, + defaults={'code': 'Latn', 'number': Script.LATIN_PK, + 'name': 'Latin'})[0] + story_type = StoryType.objects.get_or_create( + name='credit-sequence', defaults={'sort_code': 99003})[0] + country = Country.objects.get_or_create( + id=904, defaults={'code': 'q5', 'name': 'Testland C'})[0] + language = Language.objects.get_or_create( + id=904, defaults={'code': 'q6', 'name': 'Testish C'})[0] + publisher = Publisher.objects.create( + name='Test Publisher', country=country, year_began=1960) + series = Series.objects.create( + name='Test Series', sort_name='Test Series', year_began=1960, + country=country, language=language, publisher=publisher, + is_comics_publication=True, has_gallery=False, + publication_dates='1960') + creator = Creator.objects.create( + gcd_official_name=CREATOR, sort_name=CREATOR) + name = CreatorNameDetail.objects.create( + name=CREATOR, creator=creator, in_script=script) + return {'series': series, 'type': story_type, 'name': name} + + +def make_issue(world, number): + return Issue.objects.create( + number=number, series=world['series'], sort_code=int(number), + publication_date='1960', key_date='1960-01-00') + + +@pytest.fixture +def linked_and_text(world): + # One story credited through a StoryCredit object, one only in the + # free-text field. The migration moves stories from the latter to the + # former, so the two sources must stay distinguishable. + issue = make_issue(world, '1') + linked = Story.objects.create( + issue=issue, type=world['type'], sequence_number=0) + StoryCredit.objects.create( + creator=world['name'], + credit_type=CreditType.objects.get_or_create( + id=CREDIT_TYPES['script'], + defaults={'name': 'script', 'sort_code': 1})[0], + story=linked) + text = Story.objects.create( + issue=issue, type=world['type'], sequence_number=1, script=CREATOR) + return {'linked': linked, 'text': text} + + +@pytest.mark.parametrize('credit_is_linked,expected', [ + (LINKED_ONLY, ['linked']), + (BOTH, ['linked', 'text']), + (TEXT_CREDITS_ONLY, ['text']), +], ids=['linked_only', 'both', 'text_only']) +def test_credit_is_linked_picks_the_credit_source(credit_is_linked, expected, + linked_and_text): + matched = advanced_search(script=CREATOR, + credit_is_linked=credit_is_linked) + + assert matched == {linked_and_text[k].id for k in expected} + + +def test_issue_editing_matches_stories_of_the_edited_issue(world): + edited = make_issue(world, '1') + other = make_issue(world, '2') + IssueCredit.objects.create( + creator=world['name'], + credit_type=CreditType.objects.get_or_create( + id=CREDIT_TYPES['editing'], + defaults={'name': 'editing', 'sort_code': 6})[0], + issue=edited) + wanted = Story.objects.create( + issue=edited, type=world['type'], sequence_number=0) + Story.objects.create(issue=other, type=world['type'], sequence_number=0) + + assert advanced_search(issue_editing=CREATOR) == {wanted.id} + + +def test_story_editing_and_issue_editing_are_separate_credits(world): + story_credit_issue = make_issue(world, '1') + issue_credit_issue = make_issue(world, '2') + editing_type = CreditType.objects.get_or_create( + id=CREDIT_TYPES['editing'], defaults={'name': 'editing', + 'sort_code': 6})[0] + story_edited = Story.objects.create( + issue=story_credit_issue, type=world['type'], sequence_number=0) + issue_edited = Story.objects.create( + issue=issue_credit_issue, type=world['type'], sequence_number=0) + StoryCredit.objects.create( + creator=world['name'], credit_type=editing_type, story=story_edited) + IssueCredit.objects.create( + creator=world['name'], credit_type=editing_type, + issue=issue_credit_issue) + + assert advanced_search(story_editing=CREATOR) == {story_edited.id} + assert advanced_search(issue_editing=CREATOR) == {issue_edited.id} diff --git a/apps/gcd/tests/test_search_linked_only.py b/apps/gcd/tests/test_search_linked_only.py new file mode 100644 index 000000000..b97c17fbb --- /dev/null +++ b/apps/gcd/tests/test_search_linked_only.py @@ -0,0 +1,134 @@ +# -*- coding: utf-8 -*- +import pytest +from django.contrib.auth.models import AnonymousUser +from django.test import RequestFactory + +from apps.stddata.models import Country, Language, Script +from apps.gcd.models import ( + Publisher, Series, Issue, Story, StoryType, StoryCredit, CreditType, +) +from apps.gcd.models.creator import Creator, CreatorNameDetail +from apps.gcd.models.story import CREDIT_TYPES +from apps.gcd.views.search import do_advanced_search + +PERSON = 'Test Person A' + +# credit_is_linked values, exactly as the advanced-search dropdown sends them. +# A story credited only in a free-text field is matched by TEXT_CREDITS_ONLY +# and BOTH, but not by LINKED_ONLY (it has no linked credit record). +LINKED_ONLY = '' # dropdown "linked credits only" +BOTH = 'True' # dropdown "both linked and text credits" +TEXT_CREDITS_ONLY = 'False' # dropdown "text credits only" + +# Search field -> the story text field holding the same credit. +TEXT_FIELD = { + 'script': 'script', + 'pencils': 'pencils', + 'inks': 'inks', + 'colors': 'colors', + 'letters': 'letters', + 'story_editing': 'editing', +} + + +def advanced_search(**fields): + request = RequestFactory().get( + '/search/advanced/process/', + dict({'target': 'sequence', 'method': 'icontains'}, **fields)) + request.user = AnonymousUser() + items, _target = do_advanced_search(request) + return set(items.values_list('id', flat=True)) + + +@pytest.fixture +def issue(db): + Script.objects.get_or_create( + id=Script.LATIN_PK, + defaults={'code': 'Latn', 'number': Script.LATIN_PK, + 'name': 'Latin'}) + country = Country.objects.get_or_create( + id=907, defaults={'code': 'q3', 'name': 'Testland A'})[0] + language = Language.objects.get_or_create( + id=907, defaults={'code': 'q4', 'name': 'Testish A'})[0] + publisher = Publisher.objects.create( + name='Test Publisher', country=country, year_began=1965) + series = Series.objects.create( + name='Test Series', sort_name='Test Series', + year_began=1965, country=country, language=language, + publisher=publisher, is_comics_publication=True, has_gallery=False, + publication_dates='1965') + return Issue.objects.create( + number='1', series=series, sort_code=0, + publication_date='1965', key_date='1965-01-00') + + +@pytest.fixture +def story_type(db): + return StoryType.objects.get_or_create( + name='linked-only-sequence', defaults={'sort_code': 99010})[0] + + +def text_credited_story(issue, story_type, field): + # Credited only in the free-text field: no StoryCredit object exists, + # as is the case before a creator's credits are migrated. + return Story.objects.create( + issue=issue, type=story_type, sequence_number=0, + **{TEXT_FIELD[field]: PERSON}) + + +@pytest.mark.parametrize('field', sorted(TEXT_FIELD)) +def test_linked_only_ignores_a_creator_with_no_linked_credit(field, issue, + story_type): + text_credited_story(issue, story_type, field) + + matched = advanced_search(**{field: PERSON}, + credit_is_linked=LINKED_ONLY) + + assert matched == set() + + +@pytest.mark.parametrize('credit_is_linked', [BOTH, TEXT_CREDITS_ONLY], + ids=['both', 'text_only']) +def test_text_credits_are_still_found_when_asked_for(credit_is_linked, issue, + story_type): + story = text_credited_story(issue, story_type, 'script') + + matched = advanced_search(script=PERSON, + credit_is_linked=credit_is_linked) + + assert matched == {story.id} + + +def linked_credited_story(issue, story_type): + creator = Creator.objects.create( + gcd_official_name=PERSON, sort_name=PERSON) + name = CreatorNameDetail.objects.create( + name=PERSON, creator=creator, + in_script=Script.objects.get(id=Script.LATIN_PK)) + linked = Story.objects.create( + issue=issue, type=story_type, sequence_number=1) + StoryCredit.objects.create( + creator=name, + credit_type=CreditType.objects.get_or_create( + id=CREDIT_TYPES['script'], + defaults={'name': 'script', 'sort_code': 1})[0], + story=linked) + return linked + + +def test_linked_only_still_finds_a_linked_credit(issue, story_type): + linked = linked_credited_story(issue, story_type) + + matched = advanced_search(script=PERSON, credit_is_linked=LINKED_ONLY) + + assert matched == {linked.id} + + +def test_both_sources_require_every_linked_creator(issue, story_type): + linked_credited_story(issue, story_type) + + matched = advanced_search( + script='%s; Test Person Absent' % PERSON, + credit_is_linked=BOTH) + + assert matched == set()