[GrandComicsDatabase/gcd-django] Validate persisted revisions before workflow transitions (PR #738)
jhunterjActual
Summary
- Validate persisted issue and story revisions before submission and immediately before approval.
- Reuse the existing forms and formsets as the validation source of truth, including database-object Features values.
- Keep invalid submissions in the indexer's workflow and block invalid approvals with guidance using the translated Send Back to Indexer action label.
- Defer non-critical notification email until after the database transaction commits so an unavailable mail service cannot roll back a valid workflow action.
Root cause
Revision data can be changed through migrations or other non-form code paths and then submitted without passing through the normal edit-form validation. Submission and approval previously trusted that persisted data, allowing invalid mixed legacy-text/database-object Features data to reach a workflow transition. During UAT, synchronous notification failures also rolled back otherwise valid transitions.
User impact
Indexers receive linked validation errors and can correct the persisted data before submission. Editors cannot approve invalid data and are directed to use Send Back to Indexer with the required correction. Validation failures do not partially commit changes, and notification outages no longer prevent valid submit, send-back, or approval actions.
Validation
- Focused tests: 10 passed
- Flake8 passed for all changed Python files
- Changed templates compiled successfully
- Docker-configured
manage.py checkpassed git diff --checkpassed- Manual UAT passed for valid Features submission, invalid persisted data, send-back, correction, and approval
Closes #572
You can view, comment on, or merge this pull request online at:
https://github.com/GrandComicsDatabase/gcd-django/pull/738
Commit Summary
- f34e64b Validate persisted revisions before workflow transitions
File Changes
(8 files)
- A apps/oi/action_labels.py (14)
- A apps/oi/submission_validation.py (125)
- M apps/oi/templatetags/editing.py (15)
- A apps/oi/tests/test_submission_validation.py (256)
- M apps/oi/views.py (131)
- M templates/oi/bits/compare_actions.html (8)
- M templates/oi/edit/changeset.html (2)
- M templates/oi/edit/include_frame.html (4)
Patch Links:
- https://github.com/GrandComicsDatabase/gcd-django/pull/738.patch
- https://github.com/GrandComicsDatabase/gcd-django/pull/738.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 a validation mechanism for persisted revisions before changeset workflow transitions, along with synchronized workflow action labels across buttons and instructions. It adds validation logic, corresponding unit tests, and updates views and templates to use the new labels and validate changesets before submission or approval. Feedback on these changes highlights three key areas for improvement: handling potential None values for min_num and max_num in formsets to avoid Django validation errors, adding a defensive guard in _send_notification to prevent AttributeError when user is None, and refactoring the formset validation loop to avoid relying on subtle non-short-circuiting behavior of the and operator.
In apps/oi/submission_validation.py:
> + data['%s-MIN_NUM_FORMS' % prefix] = str(formset.min_num) + data['%s-MAX_NUM_FORMS' % prefix] = str(formset.max_num)
In Django, formset.min_num and formset.max_num can be None. If either is None, str(formset.min_num) or str(formset.max_num) will produce the string 'None'. When submitted to Django's ManagementForm, this will cause a validation error because 'None' is not a valid integer. It is safer to use an empty string '' when they are None so that Django's IntegerField cleans them correctly as None.
- data['%s-MIN_NUM_FORMS' % prefix] = str(formset.min_num) - data['%s-MAX_NUM_FORMS' % prefix] = str(formset.max_num) + data['%s-MIN_NUM_FORMS' % prefix] = '' if formset.min_num is None else str(formset.min_num) + data['%s-MAX_NUM_FORMS' % prefix] = '' if formset.max_num is None else str(formset.max_num)
In apps/oi/views.py:
> +def _send_notification(user, subject, body): + """Send non-critical email after commit and log delivery failures.""" + def deliver(): + try: + user.email_user(subject, body, settings.EMAIL_INDEXING) + except Exception: + # Email delivery must not turn a completed workflow action into a + # user-facing error, but operators still need the traceback. + logger.exception( + 'Failed to send OI notification to user %s.', user.pk) + + transaction.on_commit(deliver)
In the discuss workflow, if an indexer puts an OPEN changeset into discussion, changeset.approver can be None. This results in _send_notification being called with user=None, which triggers an AttributeError when attempting to call user.email_user(...). Although the exception is caught and logged, we should add a defensive guard to return early if user is None to avoid scheduling a failing on_commit callback and logging unnecessary tracebacks.
-def _send_notification(user, subject, body): - """Send non-critical email after commit and log delivery failures.""" - def deliver(): - try: - user.email_user(subject, body, settings.EMAIL_INDEXING) - except Exception: - # Email delivery must not turn a completed workflow action into a - # user-facing error, but operators still need the traceback. - logger.exception( - 'Failed to send OI notification to user %s.', user.pk) - - transaction.on_commit(deliver) +def _send_notification(user, subject, body): + """Send non-critical email after commit and log delivery failures.""" + if user is None: + return + + def deliver(): + try: + user.email_user(subject, body, settings.EMAIL_INDEXING) + except Exception: + # Email delivery must not turn a completed workflow action into a + # user-facing error, but operators still need the traceback. + logger.exception( + 'Failed to send OI notification to user %s.', user.pk) + + transaction.on_commit(deliver)
In apps/oi/submission_validation.py:
> + valid = form.is_valid() + for formset in extra_forms.values(): + if formset is not None: + valid = formset.is_valid() and valid
The loop uses valid = formset.is_valid() and valid to evaluate all formsets. Relying on the left-hand side of and to prevent short-circuiting is subtle and can easily be broken by future refactoring (e.g., if someone changes it to valid = valid and formset.is_valid()). It is safer and more readable to explicitly check the validity of each formset and update the flag.
- valid = form.is_valid() - for formset in extra_forms.values(): - if formset is not None: - valid = formset.is_valid() and valid + valid = form.is_valid() + for formset in extra_forms.values(): + if formset is not None: + if not formset.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.
jhunterjActual
@jhunterjActual pushed 1 commit.
- c7fa6fd Update apps/oi/submission_validation.py
—
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.
jhunterjActual
@jhunterjActual commented on this pull request.
In apps/oi/submission_validation.py:
> + data['%s-MIN_NUM_FORMS' % prefix] = str(formset.min_num) + data['%s-MAX_NUM_FORMS' % prefix] = str(formset.max_num)
Django 5.2’s formset_factory() converts None to numeric defaults before constructing the formset class. All formsets used by this validation path come from Django’s formset factories, so _add_formset_data(data, formset): (on submission_validation.py line 58) receives integers.
Additionally, the suggested replacement creates two lines that would fail the project’s Flake8 line-length check.
—
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.
jhunterjActual
@jhunterjActual commented on this pull request.
In apps/oi/views.py:
> +def _send_notification(user, subject, body): + """Send non-critical email after commit and log delivery failures.""" + def deliver(): + try: + user.email_user(subject, body, settings.EMAIL_INDEXING) + except Exception: + # Email delivery must not turn a completed workflow action into a + # user-facing error, but operators still need the traceback. + logger.exception( + 'Failed to send OI notification to user %s.', user.pk) + + transaction.on_commit(deliver)
The described path is unreachable. discuss(self, commenter, notes='') (from apps/oi/models.py line 823) explicitly rejects any changeset without an approver before _send_notification(changeset.approver, subject, email_body) (from apps/oi/views.py line 1253) is reached.
Silently accepting None in _send_notification() would also conceal future caller errors instead of preserving the existing invariant that a recipient is required.
—
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.
jhunterjActual
jhunterjActual left a comment (GrandComicsDatabase/gcd-django#738)Note that 2 of Gemini Code Assist's 3 feedback items were not implemented. Details in those comments.
—
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/oi/views.py:
> @@ -221,6 +229,20 @@ ############################################################################## +def _send_notification(user, subject, body): + """Send non-critical email after commit and log delivery failures.""" + def deliver(): + try: + user.email_user(subject, body, settings.EMAIL_INDEXING) + except Exception: + # Email delivery must not turn a completed workflow action into a + # user-facing error, but operators still need the traceback. + logger.exception(
What does this logging do ?
Generally this addresses a non-issue. Further, at least when a change is rejected, the indexer needs to get an email about it, which now might fail silently. Same for some other emails. Why the change ?
—
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.
jhunterjActual
jhunterjActual left a comment (GrandComicsDatabase/gcd-django#738)I probably do not understand this, but would "...explicitly rejects any changeset without an approver..." result in a failure if someone commented on a changeset directly from the Pending list, that does not yet have an approver assigned? We often have folks, some not editors, who make such comments. - Don Milne
Ordinary Add Comment activity isn't affected. It uses a separate path that leaves the changeset in its existing state and does not require an approver. On an unassigned pending changeset, it records the comment, notifies the indexer and prior commenters, and skips the approver notification because none exists.
—
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 commented.
jhunterjActual
@jhunterjActual commented on this pull request.
In apps/oi/views.py:
> @@ -221,6 +229,20 @@ ############################################################################## +def _send_notification(user, subject, body): + """Send non-critical email after commit and log delivery failures.""" + def deliver(): + try: + user.email_user(subject, body, settings.EMAIL_INDEXING) + except Exception: + # Email delivery must not turn a completed workflow action into a + # user-facing error, but operators still need the traceback. + logger.exception(
The change came from my UAT, where SMTP connection failures (I don't have an SMTP server running) caused valid workflow actions to return errors and, with ATOMIC_REQUESTS, risked rolling back the state transition. Not quite a non-issue, but not one that comes up in the happy path.
Yes, on the happy path, emails should get sent to indexers when the state of their changeset changes. But if an email cannot be sent, IMO the workflow should be able to continue anyway, and the changesets could be processed by people logging in. You are right that this changes required notifications into best-effort notifications.
This might be the wrong scope for it; I just included it here because it's where I had the problem. I could remove the email refactor from this PR if we want to address mail failure separately with an explicit delivery policy, retry/outbox mechanism, and monitoring (or just assume happy path).
—
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 commented.
JochenGCD
@jochengcd commented on this pull request.
In apps/oi/views.py:
> @@ -221,6 +229,20 @@ ############################################################################## +def _send_notification(user, subject, body): + """Send non-critical email after commit and log delivery failures.""" + def deliver(): + try: + user.email_user(subject, body, settings.EMAIL_INDEXING) + except Exception: + # Email delivery must not turn a completed workflow action into a + # user-facing error, but operators still need the traceback. + logger.exception(
For dev-purposes please use one of the dev-backends provided by django, e.g.
https://docs.djangoproject.com/en/5.2/topics/email/#console-backend
we could add that to settings_local.py of the docker setup
I don't see why we should catch mail-sending errors. If mails cannot be send, there would be severe issues with the server, which needs actions. Silencing these errors would further delay that.
Please remove this part of the pull request.
—
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 commented.
JochenGCD
@jochengcd commented on this pull request.
On apps/oi/submission_validation.py:
We so far do not split out oi-functionality into different files then what we have. It is not ideal and needs reconsideration, but that would be a separate effort. Goes also for action_labels.py. Please integrate into views.py.
In apps/oi/submission_validation.py:
> + valid = form.is_valid() + for formset in extra_forms.values(): + if formset is not None: + if not formset.is_valid(): + valid = False + + if valid: + return [] + return _validation_messages(form, extra_forms) + + +def validate_changeset_revisions(changeset, request): + """Return invalid active issue/story revisions and their errors.""" + invalid = [] + revisions = list(changeset.issuerevisions.filter(deleted=False)) + revisions.extend(changeset.storyrevisions.filter(deleted=False))
Thanks for tackling this.
While this is valid and wanted for revisions edited in a changeset, I don't think we want to force indexers to work on sequences they did not touch, i.e. existing data that would not validate.
We can use the changed status of a revision, if compare_changes was already run, not quite sure if it is when this is called.
—
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 commented.
jhunterjActual
@jhunterjActual commented on this pull request.
In apps/oi/submission_validation.py:
> + valid = form.is_valid() + for formset in extra_forms.values(): + if formset is not None: + if not formset.is_valid(): + valid = False + + if valid: + return [] + return _validation_messages(form, extra_forms) + + +def validate_changeset_revisions(changeset, request): + """Return invalid active issue/story revisions and their errors.""" + invalid = [] + revisions = list(changeset.issuerevisions.filter(deleted=False)) + revisions.extend(changeset.storyrevisions.filter(deleted=False))
We currently make indexers work on sections they did not touch. I make edits to, say, sequence titles or first lines she have to fix problems with the character list and groups.
Or if you make an edit for groups with characters who aren't members, your have to deal with earlier shortcuts.
—
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 commented.
JochenGCD
@jochengcd commented on this pull request.
In apps/oi/submission_validation.py:
> + valid = form.is_valid() + for formset in extra_forms.values(): + if formset is not None: + if not formset.is_valid(): + valid = False + + if valid: + return [] + return _validation_messages(form, extra_forms) + + +def validate_changeset_revisions(changeset, request): + """Return invalid active issue/story revisions and their errors.""" + invalid = [] + revisions = list(changeset.issuerevisions.filter(deleted=False)) + revisions.extend(changeset.storyrevisions.filter(deleted=False))
On sections, yes, but not sequences.
—
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 commented.
jhunterjActual
@jhunterjActual commented on this pull request.
In apps/oi/submission_validation.py:
> + valid = form.is_valid() + for formset in extra_forms.values(): + if formset is not None: + if not formset.is_valid(): + valid = False + + if valid: + return [] + return _validation_messages(form, extra_forms) + + +def validate_changeset_revisions(changeset, request): + """Return invalid active issue/story revisions and their errors.""" + invalid = [] + revisions = list(changeset.issuerevisions.filter(deleted=False)) + revisions.extend(changeset.storyrevisions.filter(deleted=False))
Understood. Will refactor.
—
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 commented.
jhunterjActual
@jhunterjActual commented on this pull request.
On apps/oi/submission_validation.py:
Possibly a newbie question, but: What's the issue in adding files for this? Does it impact something elsewhere?
It's easy enough to refactor, just unexpected.
—
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 commented.
JochenGCD
@jochengcd commented on this pull request.
On apps/oi/submission_validation.py:
views.py/models.py is django recommendation for organising code. If, we would have dirs views/models with further files. It is about where to expect the code.
—
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 commented.
jhunterjActual
@jhunterjActual pushed 1 commit.
- c9ec8d0 Remove best-effort workflow email handling
—
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.
jhunterjActual
@jhunterjActual commented on this pull request.
In apps/oi/views.py:
> @@ -221,6 +229,20 @@ ############################################################################## +def _send_notification(user, subject, body): + """Send non-critical email after commit and log delivery failures.""" + def deliver(): + try: + user.email_user(subject, body, settings.EMAIL_INDEXING) + except Exception: + # Email delivery must not turn a completed workflow action into a + # user-facing error, but operators still need the traceback. + logger.exception(
Commit c9ec8d0 removes this part.
—
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 commented.
jhunterjActual
@jhunterjActual commented on this pull request.
In apps/oi/views.py:
> @@ -221,6 +229,20 @@ ############################################################################## +def _send_notification(user, subject, body): + """Send non-critical email after commit and log delivery failures.""" + def deliver(): + try: + user.email_user(subject, body, settings.EMAIL_INDEXING) + except Exception: + # Email delivery must not turn a completed workflow action into a + # user-facing error, but operators still need the traceback. + logger.exception(
And I've updated the PR description to remove references to email handling changes.
—
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 commented.
jhunterjActual
@jhunterjActual pushed 1 commit.
- 16bf32b Consolidate OI workflow helpers in views
—
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.
jhunterjActual
@jhunterjActual commented on this pull request.
On apps/oi/submission_validation.py:
Commit 16bf32b has the refactoring
—
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 commented.
jhunterjActual
@jhunterjActual pushed 1 commit.
- cabb14d Skip unchanged revisions during transition validation
—
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.
jhunterjActual
@jhunterjActual commented on this pull request.
In apps/oi/submission_validation.py:
> + valid = form.is_valid() + for formset in extra_forms.values(): + if formset is not None: + if not formset.is_valid(): + valid = False + + if valid: + return [] + return _validation_messages(form, extra_forms) + + +def validate_changeset_revisions(changeset, request): + """Return invalid active issue/story revisions and their errors.""" + invalid = [] + revisions = list(changeset.issuerevisions.filter(deleted=False)) + revisions.extend(changeset.storyrevisions.filter(deleted=False))
Refactored in cabb14d
—
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 commented.