Credit the acting admin on updated_by across user account actions - #2023
Conversation
Several admin actions write the user record (bumping updated_at) but left updated_by pointing at whoever last edited the account — the same wrong attribution fixed for the welcome invite in #2022. Lock/unlock, manual email confirmation, and sending reset-password instructions all go through the controller; email-change and manual-confirm services already accepted current_user but never applied it. Set updated_by to the actor in each path. The Devise confirmation flows only save when they regenerate a token, so the services persist the attribution themselves when it's left dirty. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The welcome/setup flow bumps the user's updated_at but no admin is acting, so leaving updated_by credited whoever created or invited the account. Null it out since the actor is the account owner, not a staff member. Trim the attribution comments added in the prior commit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@jmilljr24 I merged the welcome instructions, and think we should do these too. You good w it? |
| if @user.update(password_params) | ||
| # The invited user sets their own password here — no admin is acting, so clear | ||
| # updated_by rather than leaving it crediting whoever created/invited the account. | ||
| if @user.update(password_params.merge(updated_by: nil)) |
There was a problem hiding this comment.
I'm missing why would wouldn't want to know that the user updated their account and/or if we don't care about that, we now clear out which admin last updated the account by setting nil.
There was a problem hiding this comment.
@jmilljr24 yeah i went back and forth on this one. the button is accessible publicly, so it could've been pressed by the user, or, it could've been pressed by the admin on their behalf. i'm going to update it so if there is current_user, use that, otherwise credit it to the user. then we handle my concern and yours!
There was a problem hiding this comment.
🤖 From Claude: Done in c49402d — updated_by: current_user || @user, so a signed-in user is credited and otherwise the account owner is.
The welcome page is public — the invited user may set their own password, or an admin may do it on their behalf. Setting updated_by to nil (prior commit) lost the admin attribution Justin flagged. Use current_user when present, otherwise the account owner, so we never fall back to whoever last edited the account. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
current_user isn't necessarily an admin — reword to reflect it's whoever is signed in. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🤖 suggested review level: 3 Read 📖 small, contained attribution fixes with existing test coverage
What is the goal of this PR and why is this important?
Follow-up to #2022. Several actions write the user record (bumping
updated_at) but leftupdated_bycrediting whoever last edited the account instead of who took the action — the same wrong attribution Rachel reported for the welcome invite.Admin actions — credit the acting admin:
users_controller#toggle_lock_status— lock/unlockusers_controller#confirm_email— manual confirmusers_controller#send_reset_password_instructionsUserServices::ProcessEmailChange— already tookcurrent_user, never applied itUserServices::ProcessEmailManualConfirm— resend + confirm; samePublic setup page —
welcome_controller#update(invited user sets their password):current_user || @user) — never whoever last edited the account.How did you approach the change?
updated_byat each write path (TDD: failing request/service specs first).send_confirmation_instructionsonly saves when it regenerates a token, so the two services persist the attribution themselves (save(validate: false) if changed?) — a no-op when Devise already saved.Anything else to add?
Deliberately out of scope:
BulkInviteService(runs from rake/console, nocurrent_user) and thepeople_controllernested-user autosave path.