Conversation
…mers — closes #309 Track A2 of epic #307, guardrail G4 of docs/CANONICAL_IDENTITY_RFC.md: audit every consumer of approval_status before #310's reconciler starts writing 'merged' rows. Audited and confirmed safe (exact-match/whitelist filters): badge generation, sync --type approvals, church_teams_export's force-resend categorization, [vaysf_churches]/[vaysf_badges] shortcodes, admin dashboard stats, process-token handling (already rejects any non-pending approval), and validation-issue sync (never branches on approval_status). Fixed two real issues found during the audit: - Admin participants list, admin approvals list, and the [vaysf_participants] shortcode all fell through their status switch/case to the "pending" bucket for an unrecognized status, so a tombstoned duplicate would have displayed as awaiting action. Added an explicit 'merged' case + a .status-merged style everywhere the pattern appears. - The admin Approvals page's "Resend Email" action unconditionally reset approval_status back to 'pending' and minted a fresh token/email for any row, including a merged one — undoing the reconciler's tombstone and resurrecting a retired duplicate. Now hidden for merged rows in the UI and independently rejected by the action handler. No plugin schema change needed: approval_status is already a plain VARCHAR(50), and the REST endpoints sanitize it with sanitize_text_field() rather than validating against an enum. 6 new mock tests pin the middleware-side tolerance; touched PHP passes php -l. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0186mjKMbV8qy3jRQxNg83S3
3 tasks
The resend guard added for #309 checked only the approval row's status. But apply-aliases (#310) tombstones the participant *before* its approval rows, so a partially-failed reconciliation leaves the participant 'merged' while its approval is still 'pending' — the approval-only guard would happily resend exactly that row, reset it to 'pending', and mail a fresh pastor token for a retired duplicate identity. Both the list query and the resend lookup now select p.approval_status, and both the "Resend Email" button and the action=resend handler treat a merged participant as blocking, independent of the approval row's own status. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0186mjKMbV8qy3jRQxNg83S3
Owner
Author
|
Superseded by #367, which implements #308–#310 as a single coherent change. Closing unmerged — the work continues on The merged-status audit from this PR is carried forward in #367, including the resend guard. One item from this PR remains open there: Generated by Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Track A2 of epic #307 — guardrail G4 of
docs/CANONICAL_IDENTITY_RFC.md— closes #309.sf_participants.approval_status/sf_approvals.approval_statusare plain VARCHAR(50), so the reconciliation command (#310) can tombstone a retired duplicate row withapproval_status = 'merged'with zero schema changes. This PR is the audit #309 asks for: verify every consumer ofapproval_statustolerates that unfamiliar value before #310 starts writing it, and fix anywhere a consumer would crash or misrender.Audit checklist
Safe as-is (exact-match or whitelist filters — verified with new regression tests where the logic lives in Python):
middleware/badges/runner.py) — double-filtered server- and client-side against{"approved"}, for both the bulk run and the targeted--chm-idpath.sync --type approvals(middleware/sync/manager.py) — exact!= APPROVAL_STATUS["APPROVED"]skip (targeted) and a server-sideapproval_status=approvedfilter (bulk fetch).church_teams_export.py's force-resend categorization (_handle_force_resend) — an explicit whitelist ofpending/pending_approval/reapproval_required/validated.[vaysf_churches]/[vaysf_badges]shortcodes and the admin dashboard stat tiles — exactapproval_status = 'approved'/'denied'SQL matches.process-tokenhandling (class-vaysf-rest-approvals.php) — already rejects any non-'pending'approval, includingmerged, with a generic "already processed" error, so a stale pastor-approval email token can't resurrect a tombstoned row.sync/participants.py) — keyed byparticipant_id, never branches onapproval_status; and once middleware: person alias map + resolution at sync choke point #308's alias resolution is live, normal sync never revisits a stale/mergedchm_idin the first place.Fixed — misrendered "Merged" as "Pending": the admin participants list, admin approvals list, and the
[vaysf_participants]shortcode (found during this audit — shares the same status-class helper, not explicitly named in the issue but a real consumer) all fell through theirswitch/casestatus styling to thependingbucket for any unrecognized status. Added an explicitmergedcase (+ a.status-mergedstyle) toget_status_class()inincludes/shortcodes.php,vaysf_format_approval_status()inincludes/functions.php, and the inline switches inadmin/class-vaysf-admin-participants.phpandadmin/class-vaysf-admin-approvals.php.Fixed — functional hazard, not just cosmetic: the admin Approvals page's "Resend Email" action (
admin/class-vaysf-admin-approvals.php→vaysf_resend_approval_email()) unconditionally resetapproval_statusback to'pending'and minted a fresh token/email for any row, including amergedone. Clicking Resend on a tombstoned duplicate would have undone the reconciler's tombstone (RFC §4.3 step 3 — "so an old email token can't resurrect it") and mailed the pastor a stale approval link. The button is now hidden formergedrows, and theaction=resendhandler independently rejects the request so a direct URL hit can't bypass the UI guard.No plugin schema migration needed anywhere in this audit.
Test plan
test_sync_manager.py(targeted + bulk approval sync skip merged),test_badges.py(bulk + targeted badge generation skip merged),test_church_teams_export.py(force-resend excludes merged, including a targeted--resend-chm-idhit).pytest tests/): 928 passed (922 pre-existing + 6 new), no regressions.php -lon all 4 touched PHP files: no syntax errors. No PHPUnit harness exists yet for the plugin (tracked separately), so the PHP fixes are verified by lint plus manual review of the audited call paths.🤖 Generated with Claude Code
https://claude.ai/code/session_0186mjKMbV8qy3jRQxNg83S3
Generated by Claude Code