Skip to content

Update roles when ownership transfer is accepted - #1046

Merged
mwtrew merged 4 commits into
mainfrom
new-owner-accepts-transfer-request
Oct 6, 2026
Merged

mwtrew merged 4 commits into
mainfrom
new-owner-accepts-transfer-request

Conversation

@mwtrew

@mwtrew mwtrew commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Status

What's changed?

  • On acceptance of transfer, removes the owner role (via soft deletion) for the old owner, and creates new owner role for the new owner.
  • In the latter case, a previously soft-deleted owner role will be restored if ownership is being transferred back to a user who was previously an owner.

As is the pattern elsewhere, we are archiving the old owner role rather than deleting it.

When promoting the nominee, we use `Role.unscoped[...]` to ensure that existing archived
owner roles don't cause problems in situations where ownership has been transferred back and
forth (and therefore the nominee has an existing, but archived, owner role).
@cla-bot cla-bot Bot added the cla-signed label Oct 5, 2026
@github-actions

github-actions Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

Test coverage

93.78% line coverage reported by SimpleCov.
Run: https://github.com/RaspberryPiFoundation/editor-api/actions/runs/37464273283

@raspberrypiherokubot
raspberrypiherokubot temporarily deployed to editor-api-p-new-owner--w4fg99 October 5, 2026 13:35 Inactive
@mwtrew
mwtrew marked this pull request as ready for review October 5, 2026 13:57

@zetter-rpf zetter-rpf left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one, approved with a suggestion to simplify by removing the rescues in the Accept class

This means there's no risk of a different transfer being loaded between the
controller and concept.

`requires_new` is used in Accept's transaction so it doesn't use the outer
transaction, which can result in surprising behaviour. See:
https://api.rubyonrails.org/v8.0/classes/ActiveRecord/Transactions/ClassMethods.html#module-ActiveRecord::Transactions::ClassMethods-label-Nested+transactions
@mwtrew
mwtrew temporarily deployed to editor-api-p-new-owner--w4fg99 October 6, 2026 10:33 Inactive
@mwtrew
mwtrew temporarily deployed to editor-api-p-new-owner--w4fg99 October 6, 2026 12:21 Inactive
@mwtrew
mwtrew temporarily deployed to editor-api-p-new-owner--w4fg99 October 6, 2026 12:25 Inactive
@mwtrew
mwtrew force-pushed the new-owner-accepts-transfer-request branch from 6a914f3 to d06f1e8 Compare October 6, 2026 12:32
@mwtrew
mwtrew temporarily deployed to editor-api-p-new-owner--w4fg99 October 6, 2026 12:32 Inactive
@mwtrew
mwtrew temporarily deployed to editor-api-p-new-owner--w4fg99 October 6, 2026 12:35 Inactive
@mwtrew
mwtrew merged commit c892d93 into main Oct 6, 2026
9 checks passed
@mwtrew
mwtrew deleted the new-owner-accepts-transfer-request branch October 6, 2026 12:39

This branch was previously deployed

1 inactive deployment
editor-api-p-new-owner--w4fg99 — aef2923a Deployed Oct 6, 2026 by mwtrew
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants