Repository navigation
Conversation
|
For transparency, Qwen3.8-27B reviewed (and edited) this code. |
Study never needed in GEWISWEB, so it was not included in ReportDB at the time. The board would like to have this information available, and to allow this to potentially become part of GH-146, I have now added the propagation logic. Existing rows are set to Unknown by the migration. They will be updated when the records are regenerated.
* feat(database): exchange action links for a claim before use I like security, so now everything uses the same scheme as the password resets already use. Before, a renewal or payment link was a token that was stored as it was mailed and stayed in the address of the page it opened. A click from a mailbox is a navigation from another origin, so that token was in the referrer of every request the page made, remained in the browser history, and was sent without the session cookie the form behind it needs. An action link is now a split token, of which the register stores the selector and a hash of the verifier. Following one exchanges it for a hash that is valid for one use and three minutes (we might want to increase that for this form), and the page that renews or restarts a checkout is served behind that hash, with the link recorded in the session. Because only the hash is stored, a link cannot be sent again. If that is required, a new link must be generated. Cherry-picked from GH-146. * refactor(database): drop the pending member updates This functionality was never implemented. Now that both applications are one it is also no longer necessary to keep around. Cherry-picked from GH-146. * feat(database): let the secretary resend payment link to prospective member Only a hash of an action link's token is stored now. An prospective member cannot recover from that themselves. The checkout expiry email is sent once, when the first session expires, and the checkout status page is only reached from Stripe, so both need a link that still works. The secretary can now manually resend a new payment link to a prospective member in case this is needed.
acaaeea to
9c5b446
Compare
For sake of transparency, show how the memberhsip of someone changed each year.
9c5b446 to
02389d6
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Email uniqueness can be bypassed, stale confirmations can overwrite newer data, and conversion updates are not atomic.
Review effort: Balanced
Findings: 2
Open (5)
Form binding bypasses email uniqueness validation · New Offer consumption is not atomic with membership conversion · New Stale confirmation permits duplicate or overwritten email addresses · New Missing server-side email format and length validation · New Non-adjacent membership overlaps are not fully detected · New
What changed in this PR
Adds member self-service details, graduate conversion workflows, secure action links, and membership consistency checks.
Changes:
- Adds self-service email, address, and mailing-list management.
- Adds graduate conversion/removal flows and scheduled membership checks.
- Adds migrations, translations, audit records, and extensive tests.
| File | Description |
|---|---|
translations/validators.nl.xlf |
Updates Dutch email validation key. |
translations/validators.en.xlf |
Updates English email validation key. |
translations/AutocompleteBundle.nl.xlf |
Adds Dutch autocomplete translations. |
translations/AutocompleteBundle.en.xlf |
Adds English autocomplete translations. |
tests/Service/Database/ActionLinkServiceTest.php |
Tests split-token validation and claims. |
tests/Service/Checker/MembershipTest.php |
Tests membership consistency logic. |
tests/Scheduler/MainScheduleTest.php |
Registers new scheduled commands. |
tests/Integration/Service/Checker/MembershipConsistencyTest.php |
Tests consistency reporting. |
tests/Integration/Service/Checker/GraduateConversionSweepTest.php |
Tests graduate conversion sweep. |
tests/Integration/MessageHandler/Database/GraduateRemovalRequestedHandlerTest.php |
Tests removal notifications. |
tests/Integration/MessageHandler/Database/EmailChangeEmailsTest.php |
Tests email-change messages. |
tests/Integration/EventListener/Database/MemberEmailChangeListenerTest.php |
Tests subscription migration and auditing. |
tests/Integration/Controller/User/MemberDetailsControllerTest.php |
Tests member self-service pages. |
tests/Integration/Controller/User/EmailChangeControllerTest.php |
Tests email confirmation flow. |
tests/Integration/Controller/Database/GraduateConversionTest.php |
Tests conversion decisions. |
tests/Entity/Database/EmailChangeLinkTest.php |
Tests email-change link lifecycle. |
templates/user/settings/details.html.twig |
Adds member details page. |
templates/user/settings/details-address.html.twig |
Adds address editor. |
templates/user/settings/details-address-remove.html.twig |
Adds address removal confirmation. |
templates/user/email-change.html.twig |
Adds email confirmation page. |
templates/partials/membership-history.html.twig |
Adds reusable membership history. |
templates/partials/member-sidebar.html.twig |
Links member details page. |
templates/database/member/show.html.twig |
Displays membership history. |
templates/database/member/attention-needed.html.twig |
Explains conversion exclusions. |
templates/database/mailing/list/form.html.twig |
Exposes self-service setting. |
templates/database/join/subscribe.html.twig |
Standardizes email wording. |
templates/database/join/renew-done.html.twig |
Standardizes email wording. |
templates/database/join/prospective-member/show.html.twig |
Standardizes email wording. |
templates/database/join/graduate.html.twig |
Adds graduate response form. |
templates/database/join/graduate-unavailable.html.twig |
Adds invalid-link page. |
templates/database/join/graduate-done.html.twig |
Adds acceptance result page. |
templates/database/join/graduate-declined.html.twig |
Adds decline result page. |
templates/database/join/checkout-status.html.twig |
Standardizes email wording. |
templates/database/email/member-registration.html.twig |
Standardizes email wording. |
templates/database/email/graduate-removal-requested.html.twig |
Adds secretary removal email. |
templates/database/email/graduate-conversion.html.twig |
Adds conversion offer email. |
templates/database/email/email-changed.html.twig |
Adds previous-address notification. |
templates/database/email/email-change-confirm.html.twig |
Adds confirmation email. |
templates/database/checker/membership-report.txt.twig |
Adds consistency report. |
templates/components/Database/ProspectiveMemberOverview.html.twig |
Standardizes email wording. |
templates/components/Database/MemberOverview.html.twig |
Standardizes email wording. |
src/ViewModel/Checker/MembershipError.php |
Models membership consistency errors. |
src/Validator/Database/UnusedEmailAddress.php |
Updates validation wording. |
src/Validator/Database/DeliverableEmailAddress.php |
Updates validation wording. |
src/Service/Database/Member.php |
Implements details and conversion operations. |
src/Service/Database/ActionLinkService.php |
Resolves new secure link types. |
src/Service/Checker/Renewal.php |
Sends graduate conversion offers. |
src/Service/Checker/Membership.php |
Checks membership chronology. |
src/Repository/Database/MailingListRepository.php |
Queries self-service lists. |
src/Repository/Database/GraduateConversionLinkRepository.php |
Queries conversion offers. |
src/Repository/Database/EmailChangeLinkRepository.php |
Manages email-change links. |
src/Repository/Database/AuditEmailChangeRepository.php |
Adds email audit repository. |
src/Repository/Database/AuditAddressChangeRepository.php |
Adds address audit repository. |
src/Repository/Database/ActionLinkRepository.php |
Resolves new action-link subclasses. |
src/Repository/Checker/MemberRepository.php |
Finds conversion candidates. |
src/MessageHandler/Database/GraduateRemovalRequestedHandler.php |
Notifies the secretary. |
src/MessageHandler/Database/EmailChangedNoticeEmailHandler.php |
Notifies the previous address. |
src/MessageHandler/Database/EmailChangeConfirmationEmailHandler.php |
Sends confirmation links. |
src/Message/Database/GraduateRemovalRequested.php |
Defines removal message. |
src/Message/Database/EmailChangedNoticeEmail.php |
Defines change-notice message. |
src/Message/Database/EmailChangeConfirmationEmail.php |
Defines confirmation message. |
src/Form/User/MemberEmailChangeType.php |
Adds email-change form. |
src/Form/Database/Registration/RegistrationData.php |
Updates validation wording. |
src/Form/Database/Registration/PersonalStepType.php |
Updates email label. |
src/Form/Database/MemberRenewalType.php |
Updates renewal fields. |
src/Form/Database/MemberListsType.php |
Supports scoped self-service lists. |
src/Form/Database/MemberGraduateConversionType.php |
Adds graduate conversion form. |
src/Form/Database/MemberEditType.php |
Updates email label. |
src/Form/Database/MailingListType.php |
Adds self-service configuration. |
src/Form/Database/MailingListLabel.php |
Enhances subscription labels. |
src/EventListener/Database/MemberEmailChangeListener.php |
Audits and migrates subscriptions. |
src/Entity/User/Enums/ApiPermissions.php |
Standardizes email wording. |
src/Entity/Database/RenewalLink.php |
Names renewal grace period. |
src/Entity/Database/Member.php |
Updates email error wording. |
src/Entity/Database/MailingList.php |
Stores self-service eligibility. |
src/Entity/Database/GraduateConversionLink.php |
Models conversion offers. |
src/Entity/Database/Enums/MembershipProblems.php |
Defines consistency problems. |
src/Entity/Database/Enums/MemberDetailAction.php |
Defines detail audit actions. |
src/Entity/Database/Enums/MailingListMemberOrigin.php |
Adds self-service audit origin. |
src/Entity/Database/Enums/GraduateConversionOutcome.php |
Defines conversion outcomes. |
src/Entity/Database/EmailChangeLink.php |
Models email confirmation links. |
src/Entity/Database/AuditEntry.php |
Registers new audit subclasses. |
src/Entity/Database/AuditEmailChange.php |
Records email changes. |
src/Entity/Database/AuditAddressChange.php |
Records address changes. |
src/Entity/Database/ActionLink.php |
Registers new link subclasses. |
src/DataFixtures/Member/MemberFixture.php |
Adds scheduled-flow fixtures. |
src/DataFixtures/Mailing/MailingListMembershipFixture.php |
Adds subscription scenarios. |
src/DataFixtures/Mailing/MailingListFixture.php |
Seeds self-service flags. |
src/Controller/User/MemberDetailsController.php |
Implements member details endpoints. |
src/Controller/User/EmailChangeController.php |
Implements email confirmation. |
src/Controller/Database/ProspectiveMemberController.php |
Implements graduate conversion. |
src/Command/Checker/CheckMembershipGraduateConversionCommand.php |
Schedules conversion offers. |
src/Command/Checker/CheckMembershipConsistencyCommand.php |
Schedules consistency checks. |
phpstan-baseline.neon |
Removes resolved suppressions. |
migrations/database/Version20260923140502.php |
Adds conversion outcomes. |
migrations/database/Version20260923140501.php |
Adds self-service and audit columns. |
config/routes.yaml |
Adds graduate routes. |
config/packages/messenger.yaml |
Routes new asynchronous messages. |
AGENTS.md |
Documents manual member-flow validation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| $member = $link->member; | ||
| $form = $this->createForm( | ||
| MemberGraduateConversionType::class, | ||
| $member, | ||
| ['conversion_link' => $link], | ||
| ); |
| $link->outcome = GraduateConversionOutcome::Accepted; | ||
| $link->used = true; | ||
| $this->actionLinkRepository->persist($link); |
There was a problem hiding this comment.
We could build this logic into membershipChange (but then choose to generalise the status whether the action link was used or superseded)
Or we can start a transaction
Or we can choose to ignore this if we are sure that our logic works and the server is reliable.
| if ( | ||
| null === $link | ||
| || $link->used | ||
| || $link->linkExpired() | ||
| || $link->member->lidnr !== $user->member->lidnr | ||
| ) { |
| 'label' => t('Email Address'), | ||
| 'help' => t('Use an address you keep after you stop studying.'), | ||
| 'constraints' => [new Assert\NotBlank()], |
| } | ||
| } | ||
|
|
||
| $previous = $membership; |
- Fix email uniqueness bypass in graduate conversion by checking against the original email before the form mutates it - Fix non-atomic conversion by applying membership change before consuming the conversion link - Fix stale email confirmation by validating the link's previous email and new email availability - Add missing email format and length validation to the graduate conversion form - Fix non-adjacent membership overlap detection by tracking the furthest end date - Fix pre-existing test failure in GraduateConversionTest
d2a6c4b to
02389d6
Compare
rinkp
left a comment
There was a problem hiding this comment.
-
Verified address add, edit, delete.
-
Verified email address change (flow works, maybe a bit too well resulting in duplicate emails)
-
Verified graduate conversion:
Did not receive automatic conversion emails, but they were in the database.
After cleaning that up +check:membership:conversion:graduateit seems to work
I ended up on a "This no longer works" after confirmation, but my browser has a tendency to renew in develop.
What to do with Supremum opt-in, opt-out?
The Membership model is not changed but this happens (I don't see why it works on upstream/main and not here).
| type: 'string', | ||
| nullable: true, | ||
| )] | ||
| public ?string $oldEmail = null; |
There was a problem hiding this comment.
We must think about how long to store this information.
| } | ||
|
|
||
| #[Route( | ||
| path: '/address/{type}/remove', |
There was a problem hiding this comment.
Do we allow a user to delete their last address themselves?
There was a problem hiding this comment.
After thinking about it for a little bit, I believe non-graduate members should be disallowed from deleting their last address.
Historically, the secretary has allowed graduates to remove their address from GEWIS systems. Formal members are granted certain permissions (entering into payment obligations for activities and paying on the day itself, self-service buying items at the bar and paying afterwards online) for which we may need to reach them in the future.
If the processed and dependent systems would be changed to reduce the need for a future formal contact moment, address deletions may be supported, but that seems unwise at the moment.
Of course, the malicious member may change their address into an empty or obviously fake address, but there is a difference between intentional behaviour and unintentional deletions of the address with the conseuqence of not being able to reach the member several months/years later.
|
|
||
| /** @var array<value-of<AttentionReasons>, MemberModel[]> $combined */ | ||
| $combined = []; | ||
| $askedThemselves = $this->graduateConversionLinkRepository->findMembersWithAnOpenOffer(); |
There was a problem hiding this comment.
Do we want to clarify what this really means?
| $askedThemselves = $this->graduateConversionLinkRepository->findMembersWithAnOpenOffer(); | |
| $membersOpenOffer = $this->graduateConversionLinkRepository->findMembersWithAnOpenOffer(); |
| $renewalAudit, | ||
| ); | ||
| $this->memberRepository->persist($member); | ||
| $this->supersedeGraduateConversions($member); |
There was a problem hiding this comment.
Copilot made me realise tat here the same thing is happening. We may have just renewed a member, but their action links are still valid. If there is an issue, that means that the renewal link is still valid even after renewal (although no longer functioning properly due to the information stored as properties of the graduate conversion link).
| $link->outcome = GraduateConversionOutcome::Accepted; | ||
| $link->used = true; | ||
| $this->actionLinkRepository->persist($link); |
There was a problem hiding this comment.
We could build this logic into membershipChange (but then choose to generalise the status whether the action link was used or superseded)
Or we can start a transaction
Or we can choose to ignore this if we are sure that our logic works and the server is reliable.
| return $this->applyMembershipChange( | ||
| $member, | ||
| MembershipTypes::Graduate, | ||
| ); |
There was a problem hiding this comment.
This changes the membership type as of today, instead of adding a new membership with graduate status at the end.
Two proposals (I believe they should be identical if we handle the links properly):
| return $this->applyMembershipChange( | |
| $member, | |
| MembershipTypes::Graduate, | |
| ); | |
| return $this->applyMembershipChange( | |
| $member, | |
| MembershipTypes::Graduate, | |
| $link->getCurrentExpiration(), | |
| ); |
| return $this->applyMembershipChange( | |
| $member, | |
| MembershipTypes::Graduate, | |
| ); | |
| return $this->applyMembershipChange( | |
| $member, | |
| MembershipTypes::Graduate, | |
| $member->getMembershipEndDate(), | |
| ); |
There was a problem hiding this comment.
Confirmed that this happens in practice.
| // Two records answering to one address cannot both be reached, so an address that is taken is refused. This | ||
| // says only that it is taken, not by whom. | ||
| if ( | ||
| $this->memberService->emailBelongsToSomeoneElse( |
There was a problem hiding this comment.
Here we are creating a problem.
I believe it is possible to pass this check for two users at once. Member 8000 and 8001 can both request a change link into newmail@example.org. Both can then accept this change (the email change controller does not verify this again).
Test cases do not cover this.
Proposal to keep it simple: as soon as a second person requests changing their email into newmail@example.orgthe first link is invalidated.
Alternatively, 8001 cannot request changing their email into newmail@example.org until the link of 8000 has expired.
There was a problem hiding this comment.
Second alternative is to also check for duplicated upon accepting, but that seems slightly harder to implement in the controller.
There was a problem hiding this comment.
Uniqueness of the email address is not enforced on the member table. For null values that is not a problem. For non-null values this is problematic for some behaviour, e.g. if we see unknown list memberships we try to match it to a user.
In some cases (e.g. password resets) it is fine.
| <p class="text-muted mb-2">{{ 'Not on file.'|trans }}</p> | ||
| <a class="btn btn-sm btn-outline-gewis-primary" | ||
| href="{{ path('user_settings_details_address', {'type': row.type.value}) }}"> | ||
| <span class="fas fa-plus"></span> {{ 'Add'|trans }} |
There was a problem hiding this comment.
The address form was not changed, but perhaps it is nice to set a default value for postal region there. 90-ish percent will be NETHERLANDS I assume


Description
Members can now change what the register holds about them: their email address,
their three addresses and the mailing lists that are theirs to manage, on a My
details page beside the other settings and behind sudo like them. What only the
secretary may change is shown there read-only, so everything on file is in one
place. Changing an email address takes effect only once a link sent to the new
address is followed while signed in, and the address that was replaced is told
that it happened; the subscriptions held under the old address are carried over
on the flush that writes it, wherever it was written from.
Whoever's membership is about to run out and who is not installed in a body is
now written to 45 days ahead and asked whether they want to stay on as a
graduate, because a student registered with a TU/e address stops being reachable
once they leave. They can accept, decline, or decline and ask the secretary to
remove their data. The secretary's bulk conversion stays for the people the
sweep cannot reach, and the two now settle one ending exactly once.
Both member pages also list the memberships somebody has held, and a weekly
check reports memberships that overlap or run backwards.
Along the way: action links (renewal, payment, and the two new kinds) carry a
split token of which only a hash is stored and are handed in for a single-use
claim, the way the password reset already worked; the dormant "pending member
updates" flow is dropped; and the messages the translation extractor could not
see, which
--cleanhad been deleting on every run, are written where it readsthem and translated.
Worth knowing when this deploys: every renewal and payment link currently in
somebody's mailbox stops working, four migrations run, the self-service flag is
off for every mailing list until the secretary turns it on, and two commands
join the schedule (
check:membership:conversion:graduateevery half hour,check:membership:consistencyweekly).Screenshots of the three new pages still to be added.
Related issues/external references
Fixes GH-118
Fixes GH-46
Fixes GH-66
Types of changes