Remove hack in the Invitations backend - #4370
Open
jlledom wants to merge 4 commits into
Open
Conversation
…ation The User model used an attr_accessor for :invitation instead of a proper has_one association. This was a Rails 3 workaround needed because the inverse association wasn't reliably set in memory when building a user through Invitation#make_user. Modern Rails handles has_one/belongs_to inverses correctly, so the attr_accessor is no longer needed. Removing it also means the invitation no longer needs to pass itself as a user attribute; the association is wired automatically when self.user is assigned on the invitation side. Assisted-by: Claude Code
The invitation signup tests verified user creation and activation but never checked that the invitation itself was marked as accepted. This is the key side-effect of the after_commit :accept_invitation callback, and the behavior most likely to regress after replacing the attr_accessor hack with a real has_one association. Assisted-by: Claude Code
2 new issues
|
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
| assert result.active? | ||
| assert authentication_strategy.error_message.blank? | ||
| assert invitation.reload.accepted? | ||
| end |
Contributor
Author
There was a problem hiding this comment.
I'm gonna ignore these, I don't see the problem on the duplication, these are tests.
| assert result.active? | ||
| assert authentication_strategy.error_message.blank? | ||
| assert invitation.reload.accepted? | ||
| end |
Dynamic finder methods (find_by_email, find_by_user_id) are deprecated in favour of find_by(attr:) syntax. Also caches account.invitations into a local variable to avoid calling the same method twice, addressing the reek DuplicateMethodCall warning flagged in code review. Assisted-by: Claude Code
rubocop requires a :dependent option on has_one/has_many associations. In this case the before_destroy callback already handles invitation destruction (with an abort guard if destruction fails), so adding a Rails-managed dependent option would conflict. Suppressing the cop with an inline comment explains the intentional design. Assisted-by: Claude Code
jlledom
changed the title
Invitations backend
jlledom
marked this pull request as ready for review
August 17, 2026 07:19
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
I found this horrible hack while working on #4369 and I couldn't help but fix it in this PR.
The hack comes from Rails 3 times, it seemed to try to fix a Rails bug when using association in the inverse way: Instead of creating the invite from the user, we want to create the user from the invite side. So we are creating the strong side from the weak side of the relation.
I didn't investigate further, so maybe this wasn't the reason of the hack, but anyway, using a has_one association works perfect, all tests pass, etc. So I think whatever the problem was in Rails 3, it's solved now.
You can check the original PR for further info: https://github.com/3scale/system/pull/3483/changes. About this PR, it's a pretty simple change, it should work without issues. The PR also includes the fix for Qlty comments and improves existing tests.
Which issue(s) this PR fixes
No Jira issue
Verification steps