Codecov Report

Attention: Patch coverage is89.61039% with16 lines in your changes missing coverage. Please review.

Project coverage is 87.63%. Comparing base(61c0870) to head(27637ae).
Report is 3963 commits behind head on main.

@@            Coverage Diff             @@##             main    #8419      +/-   ##==========================================+ Coverage   87.62%   87.63%   +0.01%==========================================  Files         272      272                Lines       26333    26426      +93     ==========================================+ Hits        23073    23158      +85- Misses       3260     3268       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report?Share it here.

I don't love that this ends up essentially prop-drilling the rate limiter into a bunch of new places, but I don't see an alternative other than prop-drilling the whole app struct through, which would be worse.

I think the root cause of this is that the model layer is currently sending emails, which IMHO is a bit questionable, especially because we're using this code path in a couple of places in the test suite too. It might be better to decouple these things first before introducing the rate limiting.

I think the root cause of this is that the model layer is currently sending emails, which IMHO is a bit questionable, especially because we're using this code path in a couple of places in the test suite too. It might be better to decouple these things first before introducing the rate limiting.

Yeah, I went back and forth on this yesterday — decoupling the e-mail sending out of the model layer is better from an architectural perspective, but it means theNewUser::create_or_update API is easier to misuse as it stands, since it now comes with an implicit need to figure out if the e-mail address needs verification after and action that.

The e-mail upsert code could be extracted into something that returns that state, but being in the same transaction as the user upsert is obviously a nice quality that would be bad to lose.

I'm sorta wondering if we end up with something like this (treat names as placeholders):

let new_user =NewUser::new(...);let updater = new_user.create_or_update(&conn)?;// returns some intermediate type holding a transactionlet verification_required = updater.set_email(&email)?;// needs to be optional since not all updates might touch the e-mail?updater.commit()?;// consumes updater// Has to be after the commit;if verification_required{// send verification e-mail}

Another, simpler option would obviously be to return something else fromcreate_or_update than just theUser — a struct enclosing theUser along with abool field indicating if verification is required.

Now I've typed this out, I probably have a slight preference for the second option (return something enclosingUser and whatever other state we need), but I'm interested in what you think.

☔ The latest upstream changes (presumably77d55e9) made this pull request unmergeable. Pleaseresolve the merge conflicts.