Fix Identity#destroy callback ordering and enable user reactivation

[dilberryhoundog]

Title: Fix Identity#destroy callback ordering and enable user reactivation

Summary

This PR fixes a bug where Identity#destroy fails to properly deactivate users due to callback ordering, and includes an enhancement to enable user reactivation when rejoining an account.

The Bug

When an Identity is destroyed, the deactivate_users callback runs after dependent: :nullify because Rails adds association callbacks in declaration order. By the time deactivate_users iterates over users.find_each, the association is already empty (identity_id has been set to NULL).

Result: Users are orphaned with active: true, accesses intact, but identity_id: nil.

Reproduction

identity = Identity.create!(email_address: "test@example.com")
identity.join(account)
user = identity.users.first

identity.destroy!
user.reload

user.active        # => true  (BUG: should be false)
user.accesses.any? # => true  (BUG: should be empty)
user.identity_id   # => nil   (nullified, but deactivation never ran)

Historical Context

Looking at the git history, this appears to stem from the membership refactor:

Date	Commit	Change
Apr 22	7124835	No long transactions! - DHH removes transaction from deactivate
Nov 13	edf837f	Drop memberships - Identity now has has_many :users, dependent: :nullify
Nov 13	34d83aa	Fix tests - Added identity: nil to User#deactivate, transaction re-added
Dec 3	6e9381a	Fix race condition - Changed create! to find_or_create_by! in join

The identity: nil in User#deactivate was added as a quick test fix after dropping memberships, but it created a cascade:

1. Severing the identity link allows "duplicate" users (NULL ≠ NULL in unique constraint)
2. This led to the race condition fix using find_or_create_by!
3. But the callback ordering bug remained unaddressed

On Transactions

DHH's original reasoning for removing the transaction from deactivate (commit 7124835):

"If you intend to deactivate someone, and the process fails mid process, so you only delete some sessions, or some accesses, you are actually fine. The system is never left in an incomplete state. And that's really the only time we should be using transactions with sqlite3 -- to prevent actual data integrity issues."

This reasoning still applies - without identity: nil, all operations in deactivate are independently safe:

• accesses.destroy_all - partial completion is fine
• update! active: false - atomic
• close_remote_connections - side effect, doesn't affect data integrity

The transaction was re-added alongside identity: nil but isn't needed if we remove that assignment.

Changes

Bug Fix

identity.rb: Add prepend: true to ensure deactivation runs before nullification

-  before_destroy :deactivate_users
+  before_destroy :deactivate_users, prepend: true

Enhancement: User Reactivation

user.rb: Remove identity: nil and transaction wrapper in deactivate method - preserves identity link so rejoining users can be found. This also ensures Identity destroy works correctly and avoids unique constraint issues.

joinable.rb: Handle reactivation - if an inactive user with matching identity exists, reactivate them and re-grant board access

Behavior Change

Scenario	Before	After
Identity destroyed	Users left active with orphaned accesses	Users properly deactivated first
Admin removes user	User gets identity: nil, fresh start on rejoin	User keeps identity link, same record on rejoin
User rejoins via join code	New user record created	Existing record reactivated with history intact

[flavorjones]

@dilberryhoundog Thanks so much for opening this! I've got a few notes that I hope you're open to hearing.

We ideally want to add tests for either bug fixes or new features. The app's test coverage isn't currently great because we pivoted so much during product development; and that's resulted in some rough edges like the bug you found! Going forward we very much want to improve that situation, and prevent bugs from regressing.

Also, this PR seems like it contains two separate concerns: a bug fix, and a feature for user reactivation. We'd really prefer to get one pull request per change.

I've created #2017 for the Identity destruction bug fix, and added test coverage for it. You're credited as a co-author!

I really like the idea of allow user reactivation. Are you willing to add some tests for that features and continue this PR? 🙏

[flavorjones]

See previous note.

[dilberryhoundog]

@flavorjones Thank you for reviewing. Yes there was a cascade with the three files, that meant I couldn't propose the improvement without the fix. The most impactful was the deactivate method in user.rb, which is used by both the identity destroy path and the user destroy path. Also with User.deactivate @dhh highlighted (7124835) that lengthy transactions (in that method) could cause db lockup issues.

The answer was to remove the identity: nil statement to satisfy both pathways, but first we had to ensure the callback ordering was correct in identity, otherwise the identity destroy path caused problems, as the commit record attests.
This also allowed the transaction to be removed again (it had been added back) as per dhh's instructions. Doing all this meant that a user now just has its active? flag changed to false and accesses removed. So it made sense to add the enhancement to re-activate the user for the account in Joinable:join.rb. Trying to create a new user, when there is already a user with both account_id and identity_id available to reconnect to. Also somebody would expect when rejoining an account to regain access to all their existing comments and cards etc. It all just made sense as a congruent feature as both a fix and an improvement.

The current situation is that we have merged the identity destroy callback ordering fix in #2017
We have left in this PR to remove identity: nil in User.deactivate as the identity destroy path already does this and make the user destroy path more efficient and have expected behaviour.
Then we also have some functionality in joinable.join that completes this PR that allows us to recoonnect with the orphaned user, should the user want their access to the account again.

The testing situatuation you asked for me to address is as follow.

• You (thank you) created the identity deactivation test to test that pathway.
• Surprisingly the existing deactivate test in user_test.rb is not testing the identity: nil statement . So the existing test actually covers the full functionality of my proposed changes in user.deactivate as it is. So no changes are needed to this test.
• I have created a join reactivates a deactivated user test in identity/joinable_test.rb to test the new reactivation enhancement. I also checked that we haven't caused any existing tests to become fragile or fail.
• I have also created a create for existing identity with deactivated user test in join_codes_controller to test the Joinable.join call in the join_codes_controller, as the existing create for existing identity test didn't cover the functionality.

I have tried to stay within your testing methodology and patterns identified. I have run all the test locally (they passed). So I hope that helps.

Thankyou for considering this fix and improvement.