Validate persisted revisions before workflow transitions - #738
Validate persisted revisions before workflow transitions#738jhunterjActual wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
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.
For readability and future refactoring safety Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
Note that 2 of Gemini Code Assist's 3 feedback items were not implemented. Details in those comments. |
|
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
…On 8/6/2026 3:51 PM, 'jhunterjActual' via gcd-tech wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In apps/oi/views.py
<#738 (comment)>:
> +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
<#738?email_source=notifications&email_token=ADREI2AHQ42YJ5MKTQMHF3D5ITOUDA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIOBXG44DENRXGAY2M4TFMFZW63VKON2WE43DOJUWEZLEUVSXMZLOOSWGM33PORSXEX3DNRUWG2Y#discussion_r3731532106>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADREI2EK4BYTBXNGFIQJQI35ITOUDAVCNFSNUABDKJSXA33TNF2G64TZHM3DANRRGM2DSO2JONZXKZJ3GUYDQNBRHE4DEMZQUF3AE>.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/ADREI2DJ66H5LYDTOXULNBL5ITOUDA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIOBXG44DENRXGAY2M4TFMFZW63VKON2WE43DOJUWEZLEUVSXMZLOOSVGM33PORSXEX3JN5ZQ>
and Android
<https://github.com/notifications/mobile/android/ADREI2F34ZEBQIP2G3TSFK35ITOUDA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIOBXG44DENRXGAY2M4TFMFZW63VKON2WE43DOJUWEZLEUVSXMZLOOSXGM33PORSXEX3BNZSHE33JMQ>.
Download it today!
You are receiving this because you are subscribed to this
thread.Message ID:
***@***.***>
--
You received this message because you are subscribed to the Google
Groups "gcd-tech" group.
To unsubscribe from this group and stop receiving emails from it, send
an email to ***@***.***
To view this discussion visit
https://groups.google.com/d/msgid/gcd-tech/GrandComicsDatabase/gcd-django/pull/738/review/4877826701%40github.com
<https://groups.google.com/d/msgid/gcd-tech/GrandComicsDatabase/gcd-django/pull/738/review/4877826701%40github.com?utm_medium=email&utm_source=footer>.
|
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. |
|
Thanks, glad to hear.
- Don Milne
…On 8/7/2026 7:59 AM, 'jhunterjActual' via gcd-tech wrote:
*jhunterjActual* left a comment (GrandComicsDatabase/gcd-django#738)
<#738 (comment)>
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
<#738?email_source=notifications&email_token=ADREI2DO3DLZXU5VWS66KOD5IXACXA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMRRGY3TQNJZG4Y2M4TFMFZW63VHMNXW23LFNZ2KKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-5216785971>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADREI2DXMFT2AUMMY35YBND5IXACXAVCNFSNUABDKJSXA33TNF2G64TZHM3DANRRGM2DSO2JONZXKZJ3GUYDQNBRHE4DEMZQUF3AE>.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/ADREI2DX44V6AFI3BK6DHEL5IXACXA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMRRGY3TQNJZG4Y2M4TFMFZW63VHMNXW23LFNZ2KKZLWMVXHJKTGN5XXIZLSL5UW64Y>
and Android
<https://github.com/notifications/mobile/android/ADREI2AJ4MEYAN4POIXW4RD5IXACXA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMRRGY3TQNJZG4Y2M4TFMFZW63VHMNXW23LFNZ2KKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>.
Download it today!
You are receiving this because you commented.Message ID:
***@***.***>
--
You received this message because you are subscribed to the Google
Groups "gcd-tech" group.
To unsubscribe from this group and stop receiving emails from it, send
an email to ***@***.***
To view this discussion visit
https://groups.google.com/d/msgid/gcd-tech/GrandComicsDatabase/gcd-django/pull/738/c5216785971%40github.com
<https://groups.google.com/d/msgid/gcd-tech/GrandComicsDatabase/gcd-django/pull/738/c5216785971%40github.com?utm_medium=email&utm_source=footer>.
|
Summary
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 (for example) invalid mixed text and database object Features data to be submitted and approved.
User impact
Indexers receive linked validation errors and can correct the persisted data before submission. Editors cannot approve invalid data and are directed to send back to indexer for the required correction. Validation failures do not partially commit changes.
Validation
manage.py checkpassedgit diff --checkpassedCloses #572