feat(contribution): rewrite fee cycles to payment emails in unified modal - #921
Merged
Conversation
ExtraToast
marked this pull request as draft
September 1, 2026 09:52
This was referenced Sep 1, 2026
ExtraToast
marked this pull request as ready for review
September 1, 2026 11:44
Contributor
Author
Code reviewTwo axes, run in parallel against 11 findings. 8 fixed, 2 deferred with issues raised, 1 rejected. Fixed
Deferred
Rejected
Verification
|
ExtraToast
force-pushed
the
feat/payment-emails
branch
2 times, most recently
from
September 2, 2026 07:40
10c52b7 to
ab4602a
Compare
ExtraToast
force-pushed
the
feat/payment-emails
branch
from
September 2, 2026 07:59
ab4602a to
4e0deb5
Compare
The fee cycle asked every unpaid member of a period in one operation and named itself after a concept nobody uses. It is gone. In its place, **Send payment emails** over the members ticked in the manager. One send, two statements. Each row shows which email that member gets, chosen by their direct-debit flag: a contribution reminder asks for a transfer by a date, an incasso notification announces what will be taken and when. The flag is a default rather than a rule — a mandate that failed this morning is chased by transfer, and no flag knows that yet — so the row is a dropdown, and switching it flags the row, moves the counts and re-reads its Last sent. Recipients are the selection, not the period. That is what makes the two warnings meaningful again: a member who has already paid, and one who held no membership during the period, are left out by default and can be ticked back in. Honorary members, deleted accounts and members with no address are shown with their reason and never written to, whatever the request says — a tick box on those would be a promise the tool cannot keep. Last sent is read per kind. A member reminded in September and since moved onto direct debit has never been pre-notified, and pooling the two would claim otherwise. Both dates travel on every row so the column re-reads without a round trip when a row is switched. Each date is required exactly when it reaches somebody. An all-transfer batch never types a debit date; the field says why it is empty. Amounts are still never typed. The treasurer changes a fee *type* and the amount follows from the period, live. A fee type — or a chosen email — naming somebody the send does not write to refuses the whole send with the ids at fault, rather than leaving the operator believing they changed something they did not. `NOT_MEMBER_IN_PERIOD` is a new reason code so the membership dialogs keep `NO_ACTIVE_MEMBERSHIP` meaning "no spell running now". `INCASSO_MISMATCH` goes back to deprecated: routing by the flag replaced it. The records, the email builders, the jobs and the migrations are unchanged. What changed is the operation over them.
…w still reads as one Ten columns in a dialog sized for five. Every cell was compressed to the point of unreadability, worst on the two pickers, where the whole point of the column is the words in it. The scaffold's width becomes a prop rather than a constant, so the dialogs with five columns keep the 1200 they were drawn for and this one takes 1700. The columns themselves declare no widths: they take what their content needs, which is the only thing that knows how long "Incasso notification" is. Automatic layout compresses whatever it can to fit, and left alone it lands on the pickers — so those ask for their content width and the wrappable columns give way instead. Forcibly including a warned row also erased the warning. The status chip and the row tint both read the effective disposition, which is INCLUDED once the box is ticked, so the row became indistinguishable from one that was never flagged — at exactly the moment the flag matters most, because the operator is overruling it. The tint now follows the row's own disposition, and the chip says Forced, in amber, with the alert icon. Both facts, one chip: it is going out, and it is going out because somebody said so. The note keeps the reason in amber beside it.
Send queued a hundred emails on one click. The table above it carries nine columns and a row per selected member, so its totals were not something an operator could take off it by eye before committing — and the emails are not recallable once the jobs are queued. Send now opens a summary and sends nothing. It states how many of each email and the dates they carry, then a line for every override the operator made: forcibly included despite a warning, moved off the email their flag chose, charged a fee type other than the one that applies, and already sent this same email for this period. Those four are the ones worth a second look, because each is a place the operator overruled something, and the last is how sending twice in a week is caught. Back returns to the table with every choice intact. The refusal path closes the summary rather than reporting behind it, because a 409 names rows and the summary cannot show them. `summarise` is a pure function over the rows and the three override maps, so the counts are asserted directly rather than read out of a rendered dialog.
Four things, all copy and layout. The counts and the dates become chips, which is what the table above them already uses for the same facts — a date sitting as a bare line of body text did not read as a value at all. The four override lines were loose amber sentences in an unstyled list. They are one warning alert now, headed "Worth checking before you send", because they are one thing to check rather than four unrelated remarks. "3 selected members are not written to" was about the writing rather than about the member. It says "will not have an email sent to them". The chip above the table matches at chip length: "3 get no email". The closing line loses the detail about records and queues, which is implementation the operator cannot act on: "Sending the emails will happen immediately and cannot be undone."
…ick box is the selection The api's copy of the same rule is named in the comment beside it.
Who the batch writes to, what each of them gets, and what is about to go out, asked one at a time. The Send-to box is the selection, so Forcibly include goes.
…ion's shape BulkFieldRejected reports a rule that needed the database as 400 in the same errors[] envelope bean validation and BulkSelectionProblemDetailsAdvice already produce, so a client has one handler for every refusal in a flow. Three codes join the bulk vocabulary for selections that do not add up.
…say per member Per ADR-003 the structural half of the payment-email rules sits on the request: neither date may have passed, and the tick-back-in list and the two override maps carry at most as many members as the selection does.
Four things the send swallowed are refusals naming the ids: an id that was never a user, an id named twice, somebody ticked back in the send does not write to and somebody ticked back in who is not in the selection. Each date the request states is checked against the period it bills for, so a mistyped year is caught while chasing the last unpaid members shortly after the period ends still works.
…ode over http Those field names are what a client routes a refusal on, so they are asserted at the endpoint rather than left to be discovered.
Per ADR-026 the browser composes the operator's sentence from the code, so the api's message is now fixed per code and interpolates nothing rather than shipping the same prose twice. The unknown-users sentence, whose copy the browser still takes from the api, moves to BulkUserSelection so it exists once. Per ADR-008 the prose around it names the act rather than the widget.
The send already refuses an unknown id, but the planner dropped it, so the count the treasurer read under-reported a stale selection until Send. The plan now carries unknownUserIds, the preview answers with them, and the send reads them instead of working the strays out again. The planner still writes nothing.
A 409 re-reads the plan, and re-seeding threw away every tick, fee type and email kind on the way. It now puts each of them back where the member is still in the new plan and still reachable; a member the refusal named keeps the plan's own answer, because the api has just contradicted what was chosen for them. Two more the same shape: a date the request strips no longer blocks Send, since the api never sees it; and having had this email before is history rather than an override the treasurer made, so it no longer counts as one while staying in the summary. The wizard also counts the ids the plan says are nobody.
The fee-changed test was spelled out in three places and is now isReCharged; routeRefusal knew field-to-step twice and now derives the date test from the same table; the sticky table header was verbatim in both step files and is now a mixin. The scaffold's maxWidth prop, whose only caller was deleted with the old dialog, goes with the width it defaulted to inlined. bulkDisposition cites the flow doc where it actually lives.
…w owns `PaymentEmailSteps.anHonoraryMember` registered "an honorary member in the selection", which `BulkContributionSteps` already owned. Cucumber resolves glue at runtime, so the compiler said nothing and the collision only surfaced in CI. It did not fail the two scenarios that used the step. Preparing the glue throws on a duplicate, so `BulkContributionSteps` registered nothing at all, and the steps every feature borrows from it — "the request succeeds", "the request is forbidden", "the request is refused as invalid" — became undefined everywhere. One collision, 33 failures across the suite. The step is named for this flow now, so it cannot collide again.
`TestHelper.createContributionPeriod` returns an existing period when one already has those dates, and every scenario in this feature asks for the same ones. So the thirteen scenarios share a period, and a row written by an earlier one is still there when a later one looks. Two assertions read the whole period and expected it empty. They passed alone and failed in a suite, reporting seven ids from scenarios that had already finished — one of them twice, which was the chasing scenario's second ask. Both now answer only for the members their own scenario created. The step is renamed to say so, because "nothing is recorded for the period" was the claim that was wrong.
Seven things, all from reading the screen. The fees step shows the direct-debit flag beside the email it chose. The column said which email a member gets without saying why, so the one fact the choice rests on was the one fact missing. The two dates sit in a column each. A flex row stretches its children to the tallest, so a message under one date grew the other to match; a grid column does not. The review page is a table with headers rather than a flex row per member. Its columns were sized by their content, so "Incasso notification" and "Contribution reminder" pushed the fee and the amount to different places on every line. The confirmation names people. "2 ticked back in despite a warning" was jargon for a tick box that no longer exists, and a count with nobody attached is not something a treasurer can check. It now reads "2 members are included despite a warning" and lists them, each with the warning that was overruled. The step warnings move below the table. Appearing above it pushed everything down as the treasurer worked, so the row they had just changed jumped out from under the cursor. Em-dashes are gone from the copy, and the stepper header carries no shadow of its own, which is how the membership form's reads.
…ections "1 member gets a different email than their direct-debit flag says", followed by a name, told the reader that something was wrong and left them to work out which way round. The two directions are not the same mistake: sending a reminder to somebody on direct debit asks for money that is taken anyway, and sending a notification to somebody without a mandate announces a debit that will never happen. Each line now names the member and both halves of what is true of them. "Viktor Petrov pays by direct debit but is getting a contribution reminder". "Emma Dokter is charged the alumni fee instead of the full-year fee that applies". The row's own note says the consequence rather than repeating that something was switched. `switchedDescription` and `reChargedDescription` compose those sentences, and the summary carries them, so the step and the confirmation cannot come to word the same fact differently. "7 of the selected members are left alone" becomes "7 selected members get no email", which is the thing the treasurer is checking.
The feature was an integration test in Gherkin. Eighteen scenarios, twenty of whose steps read "the request succeeds", "the request is refused as a conflict", or named a field and an error code. The features README forbids exactly that: "No transport in the feature files. Status codes, cookies, headers, paths and SQL live in step definitions." Worse, the refusals it asserted are asserted better one layer down. `BulkContributionEmailControllerIT` covers all eight with their field and code, in milliseconds, against a real database. Repeating them through a compose stack bought nothing and made a status code look like a rule the association had agreed to. ADR-001 gives the test: a system test earns its place only when the failure it catches requires the real stack, and the assertion rather than the setup is what requires it. What survives that is what a member actually received — seven scenarios, asserting the email in the inbox, the amount and the reason it states, that a member owing nothing is written to at all, and that chasing twice leaves two asks. The step definitions read the mail outbox instead of a status code. The send's response is still recorded, because a send that answers 200 while delivering nothing is the failure most worth seeing.
ExtraToast
force-pushed
the
feat/payment-emails
branch
from
September 2, 2026 08:21
4e0deb5 to
f28c32b
Compare
ExtraToast
changed the title
ExtraToast
changed the title
This was referenced Sep 2, 2026
Six scenarios failed in CI with "expected an email ... within 5000ms" and nothing had arrived. The send was fine; the search was wrong. `/test-support/emails` matches a subject exactly. Both payment emails end in the academic year the period works out to — "Please pay your Blueshell contribution (2026/2027)" — so a constant naming only the stable half matched nothing. The search now filters by recipient and matches the subject as a fragment, which is the half that does not depend on when the scenario ran. Two things hardened while here, both untestable from this machine and so worth removing as variables: The amount is asserted without its currency symbol. The builder test reads markdown, but a scenario reads rendered HTML, where a € may arrive as an entity; the digits are what the assertion is about. Its Dutch notation is built rather than formatted, so a JVM default locale cannot turn 40,00 into 40.00. And the failure now quotes what the send answered, with the subjects that inbox does hold. A refused send arrives here as an absent email, so every cause read as "no email came" — which is what made the first diagnosis take a CI round trip.
… once Run against a real stack for the first time, two payment-email scenarios failed. Neither was the code under test. `InMemoryEmailClient` held its captured emails in a plain `ArrayList`. Email jobs run async, so a send that queues a reminder and a notification together has two threads calling `add` at the same moment, and one write is lost. The job table recorded two successes and the outbox held one email, which reads as an email that was never sent. It is a `CopyOnWriteArrayList` now. Nothing had caught it because no test until this one asserted on two emails from a single send. The second failure was mine. Creating a member sends them an activation email, so an honorary member's inbox is never empty and asserting that it was could not pass. The scenario asks what it meant to ask: that no *payment* email reached them. The suite now passes 64 of 64, twice in a row, against the CI stack.
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
1 participant
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.
Closes #936, closes #939, closes #940, closes #941.
The fee cycle landed one commit ago and was the wrong shape. It asked every unpaid member of
a period in one operation, which sounded tidy and meant the treasurer could not decide who
was asked. And it named itself after a concept that exists nowhere else — not in the
association, not in the board's vocabulary, not on any other page.
Why
Two things were wrong with it, and they are the same thing.
It was over a period, not a selection. The row checkboxes in the manager had no bearing
on it: open the cycle and every unpaid member of the period was in, and the only way out was
a hard exclusion the api decided. That was defended as making "has everybody been asked
exactly once" answerable, which it does, at the cost of the question the treasurer actually
asks — these forty, now.
Because the flag decided the side, nothing could be warned about. With the partition
choosing the statement, being on the wrong one was impossible by construction, so
INCASSO_MISMATCHwas deprecated on arrival. The warnings that survived — already paid,honorary, no address — were the ones the api could rule on alone. There was no mechanism for
"I know, send it anyway", because under a partition there was nothing to overrule.
Neither problem is fixed by renaming it. The operation had to be over the selection again.
What this achieves
One menu entry, Send payment emails, over the members ticked in the manager. It needs a
period and a selection, like Mark paid beside it.
Each row states which email that member gets — a contribution reminder asking for a
transfer by a date, or an incasso notification announcing what will be taken and when.
The member's
incassoflag chooses it. That is now a default rather than a rule: the cell isa dropdown, and a row moved off its flag's choice says
⚠ Switched — pays by direct debitunder its note, moves the counts above the table, and re-reads its Last sent for the email it
is now getting.
Chips above the table count each kind and follow it live, so a batch that is accidentally all
one kind is visible before it goes out rather than after.
The warnings come back with somewhere to go. A member who has already paid, or who held no
membership during the period, is left out by default and can be ticked back in — a
contribution recorded in error and a membership backdated by hand are both real. Honorary
members, deleted accounts and members with no address on file are shown with their reason and
never written to, whatever the request says; a tick box on those would be a promise the tool
cannot keep.
How
ContributionEmailPlannerdecides the whole selection in one read and is read again by thesend, so the table and what goes out cannot disagree.
BulkContributionEmailUseCaseswritesboth record kinds in one transaction and reports them separately — one total would hide
either half being zero.
Last sent is read per kind. A member reminded in September and since moved onto direct
debit has never been pre-notified, and pooling the two would claim otherwise. Both dates
travel on every row so the column re-reads without a round trip when a row is switched.
Each date is required exactly when it reaches somebody. An all-transfer batch never types
a debit date; the field stays visible and says why it is empty, so the form does not move
under the operator. The api enforces the same rule, keyed off what the send turned out to
contain rather than off the request shape — which is why both dates are optional on the wire
and a missing one is a 400 naming which.
Amounts are still never typed. The treasurer changes a fee type and the amount follows from
the period, live in the browser. A fee type — or a chosen email — naming somebody the send
does not write to refuses the whole send with the ids at fault, rather than applying the rest
and leaving the operator believing they changed something they did not.
The records, the email builders, the jobs and the migrations are untouched. What changed is
the operation over them.
Worth a reviewer's attention
NOT_MEMBER_IN_PERIODis a new reason code rather than a reuse.NO_ACTIVE_MEMBERSHIPalready means "no spell running right now" to the End and Start membership dialogs, and this
flow means "held nothing during the year being billed". A member can be both — a current
member who was not here in 2024 — so one code with two meanings would have said the opposite
of the truth on one of the two screens.
INCASSO_MISMATCHgoes back to deprecated. Routing by the flag replaced it, and the rownow carries the switch note instead. The value stays one more release for a client reading a
stored one.
Every endpoint requires write on both records, because one send writes both. The board
role carries each, so this is not a narrowing — but it does mean the read endpoints ask for
more than they strictly use.
payment-emails.featurehas not been executed. It compiles and the steps are written,but running it needs the full compose stack, and the one on this machine was serving another
branch. It wants a run before this merges.
Verification
:services:api:test:services:api:integrationTestuser-manager-payment-emails.spec.tsyarn typecheck·yarn lint:tests:system:compileTestKotlinBulkContributionEmailControllerITasserts the table-and-send agreement directly: it readsthe table, derives the included members from its rows, and asserts the send wrote to exactly
those.
One run of the frontend suite showed eight
JobManager.test.tsfailures; they pass inisolation and the rerun was clean at 845/845. That is #804, not this branch.
Diff breakdown —
█added░removed, scaled to the largest row.