Conversation
Any current owner can cancel a pending transfer, not just the one who started it - cancel is just a third status transition alongside accept/decline, so it slots into the existing resolve! helper with no new resource-loading logic, which now wraps the read-and-update in a transaction so its row lock is held across both steps (stopping a concurrent accept/decline/cancel from also finding the transfer pending) and responds only after commit, so a commit-time callback failure surfaces as a clean 500 instead of racing an already-rendered response. The read ability for a pending transfer is likewise open to any owner of the school, not just the one who started it, since any of them may need to see it in order to decide whether to cancel it - the status endpoint now identifies the viewer by their owner role rather than by whether they happened to be the requester.
Works like the existing request email - sent from a callback that only fires when a transfer actually moves from pending to cancelled, so fixing up an old transfer to cancelled some other way won't trigger it. Since any owner can now cancel a transfer, not just the one who started it, the email attributes the cancellation to "the school owner" generically rather than naming a specific person - it has no reliable way to know which owner actually clicked cancel.
If the user-info API ever returns an empty body, the user-lookup code would crash instead of treating it as "no users found". The fix lives in the client itself, not just the caller that happened to hit it, so every caller is protected - including the job that sends the new cancellation email.
request_ownership_transfer interpolated the nominee's and requested owner's names directly with no fallback, so a missing name from the user-info lookup would render as a blank greeting or a blank sentence instead of degrading gracefully. Brings it in line with the same hardening the cancellation email already has: a generic label when a name isn't available, never a blank.
Test coverage93.7% line coverage reported by SimpleCov. |
| # head/render happen after the transaction returns, not inside it, so a | ||
| # commit-time callback failure (e.g. enqueuing a notification) surfaces as | ||
| # a clean 500 instead of racing an already-performed response. |
There was a problem hiding this comment.
I don't really understand this, maybe because it's quite jargony. What is a clean 500? What's it racing - isn't it single threaded?
| def pending_ownership_transfer | ||
| @school.ownership_transfers.lock.pending.first | ||
| end |
There was a problem hiding this comment.
A small suggestion to simplify this, if you use first! rather than first then a ActiveRecord::RecordNotFound will be raised if there are no transfers found.
ActiveRecord::RecordNotFound is already caught and turned into 404 responses by the api controllers so you can simplify resolve! by assuming the transfer always exists (removing the transfer.nil? check and the next if loaded.blank?)
| r.body = { userIds: user_ids } | ||
| end | ||
| return if response.body.blank? | ||
| return [] if response.body.blank? |
There was a problem hiding this comment.
For what requests does the User Info response return an empty body?
I just wanted to check it was equivalent to no users found, and not a different error state we should handle.
zetter-rpf
left a comment
zetter-rpf
left a comment
There was a problem hiding this comment.
Great, I've added a few comments but nothing major
Summary
Backend for #1771 lets a school owner cancel a pending ownership transfer.
What this does
Why
Covers the case where an owner changes their mind, or nominates the wrong person by mistake, without needing to wait for the original invite to be responded to.