Implement custom registration transfer logic while merging#14101
Implement custom registration transfer logic while merging#14101danieljames-dj wants to merge 2 commits intothewca:mainfrom
Conversation
| # 2. Handle conflicting registrations (where both users registered for the same competition): | ||
| # - If both are accepted: Raise error. | ||
| # - If only incoming is accepted: Swap them so the master account gets the accepted one. | ||
| # - Otherwise (incoming is not accepted): Keep the master's and the other stays in the old account. | ||
| registrations.where(competition_id: to_user_competition_ids).find_each do |registration| | ||
| other_registration = new_user.registrations.find_by(competition_id: registration.competition_id) | ||
| if registration.accepted? && other_registration.accepted? | ||
| raise "Both users have accepted registrations for #{registration.competition.name} (#{registration.competition_id})" | ||
| elsif registration.accepted? | ||
| # The master account has a non-accepted registration, but the old account has an accepted one. | ||
| # We swap them: the master gets the accepted one, and the old account keeps the unaccepted one. | ||
| # We use update_columns to bypass validations and avoid intermediate unique index violations. | ||
| other_registration.update_columns(user_id: nil) | ||
| registration.update_columns(user_id: new_user.id) | ||
| other_registration.update_columns(user_id: id) | ||
| end | ||
| end |
There was a problem hiding this comment.
Why do you focus on the status accepted so much? I have not thought through all corner-cases, so maybe I'm missing something, but at least from a high-level standpoint it should be enough to:
- Update all non-conflicting registrations (you're already doing that above)
- For all conflicting registrations:
- Update either one to
rejected - Leave the other on
accepted
- Update either one to
There was a problem hiding this comment.
I think instead of coding shenanigans, you should take a step back and think about what is the desired goal here from a business logic perspective.
The scenario you're trying to solve/fix when the same (phyiscal) person registers twice for the same competition. What if, for example, both registrations have been paid for via Stripe? In that case, we must definitely keep both registrations in our database, to keep the payment history. But which one (the old, to-be-ignored user or the new, after-merge-winner user) should be the one that actually counts as "going to the competition"?
- What if both registrations have a different selection of events?
- What if one has an admin comment and the other does not?
- etc...
These questions should be solved from a non-code business logic perspective first, and then you can find a clever way to do this in code.
There was a problem hiding this comment.
Hm looks like my solution is actually not solving the full problem, it's not considering the cases that you mentioned. I'll mark this PR as draft and post in the group for discussion.
There was a problem hiding this comment.
Note: I'm not posting this in group now, but I'll think through it again later and try if I can think of a solution.
The problem here is that we need to select one of the two (or even more) registrations. In my solution, the reason why I considered current registration as 'master' registration is because the results and events will be more accurate with current registration.
Reason why I was not happy with two accepted registration was because there are chances that one of the twins will have the WCA ID, and the delegate might wrongly assume they are same, and click 'merge'.
Does this answer your questions or do you still see a gap?
I understand there can be admin comment on the non-accepted registration, but should we care about it because that is going to stay unaccepted, as we are swapping user?
| elsif registration.accepted? | ||
| # The master account has a non-accepted registration, but the old account has an accepted one. | ||
| # We swap them: the master gets the accepted one, and the old account keeps the unaccepted one. | ||
| # We use update_columns to bypass validations and avoid intermediate unique index violations. |
There was a problem hiding this comment.
This "trick" with update_columns does not work because the uniqueness validation exists on a pure database level. update_columns only bypasses Rails model validations, but the SQL UNIQUE index will still complain.
2 participants
This change is because currently if we try to merge two users having registrations at same competition, it fails.