Correctly transfer 'last_updated_by' for annotations and scores when merging users #5894
Conversation
WalkthroughThe changes in this pull request focus on the Changes
Sequence Diagram(s)sequenceDiagram
participant UserA as User A
participant UserB as User B
participant Annotation as Annotation
participant Score as Score
UserA->>UserB: Initiate merge
UserB->>Annotation: Retrieve annotations
Annotation->>UserB: Get last_updated_by_id
alt last_updated_by_id matches UserA
Annotation->>UserB: Update last_updated_by to UserB
end
UserB->>Score: Retrieve scores
Score->>UserB: Get last_updated_by_id
alt last_updated_by_id matches UserA
Score->>UserB: Update last_updated_by to UserB
end
UserB->>UserA: Complete merge
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- app/models/user.rb (1 hunks)
- test/models/user_test.rb (1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
app/models/user.rb
[warning] 433-434: app/models/user.rb#L433-L434
Added lines #L433 - L434 were not covered by tests
🔇 Additional comments (2)
app/models/user.rb (1)
433-434: 🛠️ Refactor suggestionVerify annotation ownership transfer logic.
The changes correctly handle both aspects of annotation transfer:
- Transferring owned annotations to the target user
- Updating
last_updated_byreferences to point to the target userHowever, there's a potential race condition since the updates are performed separately.
Let's verify the annotation relationships and constraints:
Consider combining both updates into a single query to prevent potential race conditions:
- annotations.each { |a| a.update!(user: other) } - Annotation.where(last_updated_by_id: id).find_each { |a| a.update!(last_updated_by: other) } + Annotation.where(user_id: id).or(Annotation.where(last_updated_by_id: id)) + .update_all( + "user_id = CASE WHEN user_id = #{id} THEN #{other.id} ELSE user_id END, + last_updated_by_id = CASE WHEN last_updated_by_id = #{id} THEN #{other.id} ELSE last_updated_by_id END" + )🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 433-434: app/models/user.rb#L433-L434
Added lines #L433 - L434 were not covered by teststest/models/user_test.rb (1)
849-873: LGTM! Well-structured test case that verifies the last_updated_by transfer.The test effectively covers both scenarios:
- Annotations owned by the merging user
- Annotations owned by others but last updated by the merging user
The assertions properly verify that the last_updated_by field is correctly transferred in both cases.
jorg-vr
changed the title
This pull request fixes the merge users script for transferring the last_updated_by status on non owned submissions. The same fix is also applied for scores.