[GrandComicsDatabase/gcd-django] Update API v2 for feature-name relationships (PR #744)
Adam Hernandez
What changed
I synced the current beta branch into api-v2 and updated the API for the new feature-name relationships. Story detail still uses the documented feature_object response key, but each entry now comes from the selected FeatureNameDetail and returns that name with the parent Feature ID and type.
I also updated Feature detail to expose description and active name_details, moved Feature logo lookup to the feature-name relationship, and made the Feature name filter match active aliases. The branch includes the empty migration needed to join the API and feature-name migration histories and updates API CI to the supported Python 3.13 line.
I left the broader existing OpenAPI validation cleanup out of this PR so the feature-name compatibility change stays focused.
Testing
pytest apps/api_v2/tests/— 315 passed- merge-sensitive OI tests — 66 passed after loading the documented
storytypefixture ruff check apps/api_v2/ruff format --check apps/api_v2/python manage.py makemigrations --check --dry-runpython manage.py check
You can view, comment on, or merge this pull request online at:
https://github.com/GrandComicsDatabase/gcd-django/pull/744
Commit Summary
- 1851c00 add character order editing
- 04df8f5 for python 3.14
- 395e037 limit edit/delete of character in character order
- 5073967 comment old brand_id on issue, cleanup
- 90896f3 commit_to_display for character orders, display of character orders
- fbd438f Merge remote-tracking branch 'origin' into character-ordering
- e3d24c2 show more info about character appearance
- fd93269 format
- 3a97e03 auto-commit if only character order changes
- ddf58e1 Merge remote-tracking branch 'origin' into character-ordering
- 802e0a7 Merge remote-tracking branch 'origin' into character-ordering
- 37e74ab Merge branch 'beta' into character-ordering
- 762e90f change donation
- e715d15 Merge branch 'beta' of github.com:GrandComicsDatabase/gcd-django into character-ordering
- 595ede2 center
- 8052287 remove clutter
- 9495ed5 simplify deleting of characters with relations
- b998b7a fix check on cover indexing for non-comics
- 7ed1bc2 Merge remote-tracking branch 'origin' into character-ordering
- 7d68a65 add tabbing for different orders
- b840bf6 add auto-approve comment for characterorder-only changes
- df57ce4 move JS, column/row order flipped for characters
- b2e667a spacing
- 20036bd spacing
- 0c21ea5 space
- 22202aa Pin setuptools<81 to resolve pkg_resources ModuleNotFoundError
- 3b10181 Consolidate setuptools version pin to line 43
- f6fa68e ranked choice had no border
- db0cc96 Fix Python 3.12+ invalid escape sequence warnings in search_haystack (#714)
- f4a383c Bugfix/issue 505 relations same object (#719)
- edcf7d0 Fix cross series variants (#717)
- 5942b60 Bugfix/issue 619 json export series overview (#718)
- ed3a631 filter the character names when a group is selected (#721)
- 966b1e4 move JSON button, order of numbered lists
- df89d7b Merge remote-tracking branch 'origin' into character-ordering
- 8102f61 prevent click on feature logo
- af7f9de handle edga case
- a64747f two columns only when needed
- 2201797 replace by brand_emblem m2m
- bb3cd3d replace by brand_emblem m2m
- e42bebb add API to my
- 77c3019 fix deprecated brand
- a7e4228 Fix brand emblem (#723)
- 0c65329 add dependency checks for StoryXRevision
- c5d077e Merge remote-tracking branch 'origin' into beta
- d16201b test: repair the suite after the brand emblem renames (#724)
- 2781af1 fix compare & copy for m2m
- 5bf82f0 only reset the series issue count cache if explicitly requested
- 2ff1859 fix(scripts): clear default ordering in the reset_stats subquery (#725)
- 875f9cc Add pytest config (#726)
- e1a81a7 fix(gcd): use editing credit type in advanced story search (#727)
- 3127822 Fix/advanced story credit search (#728)
- 8227388 Handle Cloudflare challenges in editing forms (#734)
- 189050c Establish the supported development environment baseline (#735)
- fa271ee Avoid duplicate active-name query in creator name handling (#737)
- 8915140 Fix character-order revision commit handling (#736)
- 551a9aa move test
- b6a95ea add FORMS_URLFIELD_ASSUME_HTTPS
- 5062e72 Add Feature Names and Description (#741)
- 180e9c4 remove not used field from imps
- 6192d7a feature description on form
- c275944 fix order search by brand
- a0f2759 Remove related revisions when deleting stories (#740)
- c0b5c3a update search link
- e76fa04 change API access mode
- 500be54 Merge remote-tracking branch 'origin' into beta
- 0a07679 add missing colors
- 8ddcf53 update use of new filter_issues
- beb8052 update use of new filter_issues
- e095010 remove story filter disclaimer
- 7d82158 use feature names, remove sequence disclaimer partly
- 9d4b57a display reboot
- ba64af2 filter_sequence change
- 2d12743 unneeded check
- 64d7e2b feature_name instead of feature_object
- b6d6cfb names
- 7424d7b extend add_generic with extra_forms, use for add_feature
- 02ad21c genre from featurename when editing
- a810655 feature_name path to active_stories
- 780718b feature_name path to active_stories
- 77a15bc feature_name instead of feature_object
- cbc7b98 feature_name
- 80df1e7 feature_name for feature_logo
- c442f5d Show reviewing changeset states on mobile devices (#742)
- b8355f6 reorg display of state for mobile reviews
- 7a212a7 Merge beta into api-v2 for feature names
- a2111ea fix(api-v2): use feature names for story links
File Changes
(72 files)
- M .github/workflows/api-v2-ci.yml (6)
- A .github/workflows/dev-environment-ci.yml (90)
- M apps/api_v2/filters/features.py (15)
- M apps/api_v2/filters/stories.py (6)
- M apps/api_v2/serializers/features.py (57)
- M apps/api_v2/serializers/stories.py (30)
- M apps/api_v2/tests/test_filters/test_features.py (37)
- M apps/api_v2/tests/test_filters/test_stories.py (21)
- M apps/api_v2/tests/test_migrations/test_series_bond_timestamps.py (12)
- M apps/api_v2/tests/test_performance/test_features.py (11)
- M apps/api_v2/tests/test_performance/test_stories.py (11)
- M apps/api_v2/tests/test_serializers/test_features.py (43)
- M apps/api_v2/tests/test_serializers/test_stories.py (69)
- M apps/api_v2/tests/test_views/test_features.py (11)
- M apps/api_v2/tests/test_views/test_stories.py (21)
- M apps/api_v2/views/features.py (31)
- M apps/api_v2/views/stories.py (17)
- A apps/gcd/migrations/0068_remove_issue_brand_from_db_add_multiple_feature_names.py (35)
- A apps/gcd/migrations/0069_add_feature_names_to_story.py (18)
- A apps/gcd/migrations/0070_feature_logo_2_feature_name.py (18)
- A apps/gcd/migrations/0071_feature_description.py (19)
- A apps/gcd/migrations/0075_merge_feature_names_api_v2.py (11)
- M apps/gcd/models/__init__.py (4)
- M apps/gcd/models/creator.py (10)
- M apps/gcd/models/feature.py (74)
- M apps/gcd/models/issue.py (2)
- M apps/gcd/models/seriesbond.py (1)
- M apps/gcd/models/story.py (11)
- M apps/gcd/templatetags/credits.py (6)
- M apps/gcd/templatetags/display.py (6)
- A apps/gcd/tests/test_creator.py (68)
- A apps/gcd/tests/test_dev_environment.py (27)
- M apps/gcd/urls.py (6)
- M apps/gcd/views/details.py (276)
- M apps/gcd/views/search.py (13)
- M apps/oi/forms/__init__.py (3)
- M apps/oi/forms/feature.py (109)
- M apps/oi/forms/story.py (27)
- M apps/oi/forms/support.py (2)
- A apps/oi/migrations/0063_remove_issue_brand_from_db_add_multiple_feature_names.py (53)
- A apps/oi/migrations/0064_add_feature_names_to_story.py (19)
- A apps/oi/migrations/0065_feature_logo_2_feature_name.py (19)
- A apps/oi/migrations/0066_featurerevision_description.py (18)
- M apps/oi/models.py (396)
- M apps/oi/templatetags/compare.py (33)
- A apps/oi/tests/db/test_character_order_revision.py (177)
- M apps/oi/tests/db/test_story_revision.py (111)
- A apps/oi/tests/test_standard_queues_template.py (69)
- M apps/oi/views.py (61)
- M apps/select/urls.py (5)
- M apps/select/views.py (78)
- A apps/stats/tests/test_reset_stats.py (78)
- A docs/development/SUPPORTED_VERSIONS.md (37)
- M pyproject.toml (9)
- A scripts/check_dev_environment.py (72)
- M settings.py (10)
- A settings_dev.py (45)
- M static/css/input.css (21)
- M static/css/output.css (22)
- A static/js/cloudflare_challenge.js (42)
- M static/js/oi/issuerevision_form_utils.js (3)
- M static/js/oi/storyrevision_form_utils.js (2)
- M templates/gcd/details/tw_feature.html (14)
- M templates/gcd/details/tw_feature_logo.html (6)
- M templates/oi/base_view.html (14)
- M templates/oi/bits/jquery.html (3)
- M templates/oi/bits/standard_queues.html (19)
- M templates/oi/edit/add_frame.html (6)
- M templates/oi/edit/bulk_frame.html (6)
- M templates/oi/edit/revision.html (6)
- M templates/projects/index.html (2)
- M templates/search/indexes/gcd/feature_text.txt (3)
Patch Links:
- https://github.com/GrandComicsDatabase/gcd-django/pull/744.patch
- https://github.com/GrandComicsDatabase/gcd-django/pull/744.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
gemini-code-assist[bot]
@gemini-code-assist[bot] commented on this pull request.
Code Review
This pull request introduces support for multiple feature names via the new FeatureNameDetail model, updating filters, serializers, views, and the Online Indexer workflow to handle multiple names per feature. It also updates the development environment contract to Python 3.13, Django 5.2, and MySQL 8.0. The review feedback focuses on improving robustness and preventing runtime errors, such as avoiding potential KeyError, IndexError, and AttributeError exceptions through safer dictionary and queryset access, preventing duplicate querysets with .distinct(), and ensuring empty extra forms in formsets are not saved.
In apps/gcd/models/feature.py:
> def active_stories(self): - return self.story_set.filter(deleted=False) + from apps.gcd.models import Story + return Story.objects.filter(deleted=False, + feature_name__feature_id=self.id)
Since Story.feature_name is a ManyToMany relation, filtering on feature_name__feature_id can return duplicate Story objects if a story is linked to multiple names of the same feature. To prevent duplicates, append .distinct() to the queryset.
- def active_stories(self): - return self.story_set.filter(deleted=False) - from apps.gcd.models import Story - return Story.objects.filter(deleted=False, - feature_name__feature_id=self.id) + def active_stories(self): + from apps.gcd.models import Story + return Story.objects.filter(deleted=False, + feature_name__feature_id=self.id).distinct()
> feature_logo = get_gcd_object(FeatureLogo, feature_logo_id) - if feature_logo.feature.all()[0].feature_type.id == 1: + if feature_logo.feature_name.all()[0].feature.feature_type.id == 1:
Accessing [0] on the queryset directly can raise an IndexError if the feature_name relation is empty. It is safer to use .first() and check if it is not None before accessing its attributes.
first_name = feature_logo.feature_name.first() if first_name and first_name.feature.feature_type.id == 1:
> template='oi/bits/uni_field.html'))
- for field in fields[description_pos:]])
+ for field in fields[description_pos:-1]])
self.helper.layout = Layout(*(f for f in field_list))
Relying on a hardcoded slice index :-1 to exclude additional_names_help is fragile and can easily break if fields are reordered or new fields are added to the form. It is much safer to explicitly exclude additional_names_help by name.
field_list.extend([BaseField(Field(field, template='oi/bits/uni_field.html')) for field in fields[description_pos:] if field != 'additional_names_help'])
> + def clean_feature_name(self):
+ languages = self.cleaned_data['feature_name']\
+ .values('feature__language')
+ if languages.count() != languages.distinct().count():
+ raise forms.ValidationError(
+ 'Only one feature can be assigned per language.')
+ return self.cleaned_data['feature_name']
+
Accessing self.cleaned_data['feature_name'] directly can raise a KeyError if the field failed previous validation. Additionally, calling .count() and .distinct().count() triggers two separate database queries. You can optimize this and make it safer by using .get() and evaluating the languages in Python.
- def clean_feature_name(self):
- languages = self.cleaned_data['feature_name']\
- .values('feature__language')
- if languages.count() != languages.distinct().count():
- raise forms.ValidationError(
- 'Only one feature can be assigned per language.')
- return self.cleaned_data['feature_name']
-
+ def clean_feature_name(self):
+ feature_names = self.cleaned_data.get('feature_name')
+ if feature_names:
+ languages = [fn.feature.language_id for fn in feature_names]
+ if len(languages) != len(set(languages)):
+ raise forms.ValidationError(
+ 'Only one feature can be assigned per language.')
+ return feature_names
> + if (cd['feature'] or cd['feature_name'] or cd['feature_logo']) and \
cd['type'].id in NO_FEATURE_TYPES:
Accessing cd['type'] and other fields directly can raise a KeyError if those fields failed validation and are missing from cleaned_data. It is safer to use .get() to access these fields.
type_obj = cd.get('type') if (cd.get('feature') or cd.get('feature_name') or cd.get('feature_logo')) and type_obj and type_obj.id in NO_FEATURE_TYPES:
> + if cd['feature'] and (cd['feature_name'] or cd['feature_logo']):
raise forms.ValidationError(
['Either use the text feature field or the database objects.'])
- if cd['feature_object']:
- for feature in cd['feature_object']:
+ if cd['feature_name']:
+ for feature_name in cd['feature_name']:
Accessing cd['feature'] and cd['feature_name'] directly can raise a KeyError if those fields failed validation and are missing from cleaned_data. It is safer to use .get() to access these fields.
- if cd['feature'] and (cd['feature_name'] or cd['feature_logo']):
- raise forms.ValidationError(
- ['Either use the text feature field or the database objects.'])
-
- if cd['feature_object']:
- for feature in cd['feature_object']:
- if cd['feature_name']:
- for feature_name in cd['feature_name']:
+ if cd.get('feature') and (cd.get('feature_name') or cd.get('feature_logo')):
+ raise forms.ValidationError(
+ ['Either use the text feature field or the database objects.'])
+
+ if cd.get('feature_name'):
+ for feature_name in cd['feature_name']:
> + for related_name in ('story_credit_revisions',
+ 'story_character_revisions',
+ 'story_group_revisions'):
+ for revision in getattr(self, related_name).all():
+ for field in revision._get_single_value_fields():
+ setattr(revision, field,
+ getattr(revision.source, field))
+ revision.deleted = True
+ revision.save()
+ for field in revision._get_multi_value_fields():
+ getattr(revision, field).set(
+ getattr(revision.source, field).all())
If revision.source is None (which can happen for newly added revisions that do not have a source yet), calling getattr(revision.source, field) will raise an AttributeError. It is safer to add a guard to skip revisions where source is None.
- for related_name in ('story_credit_revisions',
- 'story_character_revisions',
- 'story_group_revisions'):
- for revision in getattr(self, related_name).all():
- for field in revision._get_single_value_fields():
- setattr(revision, field,
- getattr(revision.source, field))
- revision.deleted = True
- revision.save()
- for field in revision._get_multi_value_fields():
- getattr(revision, field).set(
- getattr(revision.source, field).all())
+ for related_name in ('story_credit_revisions',
+ 'story_character_revisions',
+ 'story_group_revisions'):
+ for revision in getattr(self, related_name).all():
+ if revision.source is None:
+ continue
+ for field in revision._get_single_value_fields():
+ setattr(revision, field,
+ getattr(revision.source, field))
+ revision.deleted = True
+ revision.save()
+ for field in revision._get_multi_value_fields():
+ getattr(revision, field).set(
+ getattr(revision.source, field).all())
> + for feature_name_form in feature_names_formset: + if feature_name_form.is_valid() and \ + feature_name_form.cleaned_data and \ + feature_name_form not in removed_names: + cd = feature_name_form.cleaned_data + if 'id' in cd and cd['id']: + feature_revision = feature_name_form.save() + else: + feature_revision = feature_name_form.save(commit=False) + feature_revision.save_added_revision( + changeset=self.changeset, feature_revision=self)
When manually iterating over a formset's forms, extra forms that have not been filled out by the user (and thus have no changes) will still be validated and saved, resulting in empty/spurious FeatureNameDetailRevision records. You should check form.has_changed() to skip unchanged extra forms.
for feature_name_form in feature_names_formset: if feature_name_form.is_valid() and feature_name_form.cleaned_data and feature_name_form not in removed_names: if not feature_name_form.has_changed() and not feature_name_form.instance.pk: continue cd = feature_name_form.cleaned_data if 'id' in cd and cd['id']: feature_revision = feature_name_form.save() else: feature_revision = feature_name_form.save(commit=False) feature_revision.save_added_revision( changeset=self.changeset, feature_revision=self)
In apps/oi/views.py:
> + valid = form.is_valid() + for extra_form in instantiated_extra_forms.values(): + valid = extra_form.is_valid() and valid
Relying on the short-circuiting behavior of the and operator (by placing extra_form.is_valid() on the left side) is subtle and prone to accidental breakage if the order is ever changed. A more explicit loop that validates all forms without short-circuiting is much safer and more readable.
- valid = form.is_valid() - for extra_form in instantiated_extra_forms.values(): - valid = extra_form.is_valid() and valid + valid = form.is_valid() + for extra_form in instantiated_extra_forms.values(): + if not extra_form.is_valid(): + valid = False
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Adam Hernandez
@DeusExTaco commented on this pull request.
In apps/gcd/models/feature.py:
> def active_stories(self): - return self.story_set.filter(deleted=False) + from apps.gcd.models import Story + return Story.objects.filter(deleted=False, + feature_name__feature_id=self.id)
Good catch. This line is already present unchanged on beta, and it isn't part of the API v2 compatibility change in this PR. I agree that .distinct() is the right follow-up because multiple names for one feature can duplicate stories, but I want to keep that beta-model hardening in a separate focused change instead of mixing it into this sync.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Adam Hernandez
@DeusExTaco commented on this pull request.
> feature_logo = get_gcd_object(FeatureLogo, feature_logo_id) - if feature_logo.feature.all()[0].feature_type.id == 1: + if feature_logo.feature_name.all()[0].feature.feature_type.id == 1:
This code is already present unchanged on beta. I agree that [0] can fail if the relation is empty, but I don't want to silently send an unlinked logo down the non-type-1 branch without first deciding whether the correct behavior is a fallback or a 404. I'll keep that for a focused beta follow-up with explicit behavior and a regression test.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Adam Hernandez
@DeusExTaco commented on this pull request.
> template='oi/bits/uni_field.html'))
- for field in fields[description_pos:]])
+ for field in fields[description_pos:-1]])
self.helper.layout = Layout(*(f for f in field_list))
Agreed that explicitly excluding additional_names_help would be clearer and less dependent on field order. This layout code is inherited unchanged from beta, so I'm keeping the cleanup out of this API compatibility PR and will treat it as a separate beta-form follow-up.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Adam Hernandez
> + def clean_feature_name(self):
+ languages = self.cleaned_data['feature_name']\
+ .values('feature__language')
+ if languages.count() != languages.distinct().count():
+ raise forms.ValidationError(
+ 'Only one feature can be assigned per language.')
+ return self.cleaned_data['feature_name']
+
Good point. This method is inherited unchanged from beta. When I address it separately, I'll use .get() plus one values_list(..., flat=True) query so the missing-field case is safe without introducing per-object queries. I'm keeping that beta-form hardening out of this API compatibility PR.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Adam Hernandez
@DeusExTaco commented on this pull request.
> + if cd['feature'] and (cd['feature_name'] or cd['feature_logo']):
raise forms.ValidationError(
['Either use the text feature field or the database objects.'])
- if cd['feature_object']:
- for feature in cd['feature_object']:
+ if cd['feature_name']:
+ for feature_name in cd['feature_name']:
Agreed on the general robustness concern. This code is inherited unchanged from beta, and it is part of the same broader direct-lookup pattern elsewhere in this clean() method. I want to fix those validation paths together with focused tests in a separate beta-form change instead of applying a partial fix here.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Adam Hernandez
@DeusExTaco commented on this pull request.
> + for related_name in ('story_credit_revisions',
+ 'story_character_revisions',
+ 'story_group_revisions'):
+ for revision in getattr(self, related_name).all():
+ for field in revision._get_single_value_fields():
+ setattr(revision, field,
+ getattr(revision.source, field))
+ revision.deleted = True
+ revision.save()
+ for field in revision._get_multi_value_fields():
+ getattr(revision, field).set(
+ getattr(revision.source, field).all())
I don't think this guard should be applied. During a deletion commit, _pre_delete() runs before _reset_values() and removes added dependent revisions whose source is None. The existing story-revision regression test also verifies that lifecycle. Skipping a source-less revision here would mask a lifecycle violation rather than restore it correctly, so I'm leaving this unchanged.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Adam Hernandez
@DeusExTaco commented on this pull request.
> + if (cd['feature'] or cd['feature_name'] or cd['feature_logo']) and \
cd['type'].id in NO_FEATURE_TYPES:
This changed line is already present on beta, and the surrounding clean() method has several similar direct cleaned_data lookups. I agree the validation path should be hardened, but changing only this expression would leave the same failure mode nearby. I'll handle the method consistently in a separate beta-form follow-up rather than widening this API compatibility PR.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Adam Hernandez
@DeusExTaco commented on this pull request.
> + for feature_name_form in feature_names_formset: + if feature_name_form.is_valid() and \ + feature_name_form.cleaned_data and \ + feature_name_form not in removed_names: + cd = feature_name_form.cleaned_data + if 'id' in cd and cd['id']: + feature_revision = feature_name_form.save() + else: + feature_revision = feature_name_form.save(commit=False) + feature_revision.save_added_revision( + changeset=self.changeset, feature_revision=self)
I don't think this currently creates empty revisions. Django leaves cleaned_data empty for an unchanged extra form, and the existing condition requires non-empty cleaned_data before saving. A has_changed() guard would be redundant here, so I'm leaving this unchanged.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Adam Hernandez
@DeusExTaco commented on this pull request.
In apps/oi/views.py:
> + valid = form.is_valid() + for extra_form in instantiated_extra_forms.values(): + valid = extra_form.is_valid() and valid
I don't think the short-circuit issue applies here. extra_form.is_valid() is the left operand, so it is evaluated on every iteration even after valid becomes false. The current loop does validate every extra form; the suggested version would be a readability refactor rather than a behavior fix, so I'm leaving it unchanged.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
JochenGCD
@jochengcd commented on this pull request.
In apps/api_v2/serializers/stories.py:
> @@ -190,15 +191,18 @@ class Meta(StoryListSerializer.Meta):
)
def get_feature_object(self, obj):
Is this now behaving as characters do in the API ?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Adam Hernandez
@DeusExTaco commented on this pull request.
In apps/api_v2/serializers/stories.py:
> @@ -190,15 +191,18 @@ class Meta(StoryListSerializer.Meta):
)
def get_feature_object(self, obj):
Yes, that’s the intention. feature_object now follows the same pattern as characters: it returns the parent Feature ID, but the name from the specific FeatureNameDetail linked to the story. So if a story uses an alternate feature name, the API returns that alternate name with the canonical Feature ID. Deleted feature names and deleted parent features are excluded as well.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
JochenGCD
@jochengcd commented on this pull request.
> feature_logo = get_gcd_object(FeatureLogo, feature_logo_id) - if feature_logo.feature.all()[0].feature_type.id == 1: + if feature_logo.feature_name.all()[0].feature.feature_type.id == 1:
An empty relation would be a db error and should fail so that we get notified.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
JochenGCD
@jochengcd commented on this pull request.
> + if (cd['feature'] or cd['feature_name'] or cd['feature_logo']) and \
cd['type'].id in NO_FEATURE_TYPES:
If these checks would be a problem, we would already have errors in production. Type cannot be not set, the browser prevents a submit of the form.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
JochenGCD
@jochengcd commented on this pull request.
> + for related_name in ('story_credit_revisions',
+ 'story_character_revisions',
+ 'story_group_revisions'):
+ for revision in getattr(self, related_name).all():
+ for field in revision._get_single_value_fields():
+ setattr(revision, field,
+ getattr(revision.source, field))
+ revision.deleted = True
+ revision.save()
+ for field in revision._get_multi_value_fields():
+ getattr(revision, field).set(
+ getattr(revision.source, field).all())
correct, is fine, was discussed in earlier submit.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
JochenGCD
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.