FIX: use active record update_attribute instead of mini sql.
#14367
Conversation
The "save" method will trigger the before_save callback "match_primary_group_changes" for User model. Else `flair_group_id` won't be removed from the user.
vinothkannans
changed the title
save! instead of mini sql.update_attribute instead of mini sql.
|
This pull request has been mentioned on Discourse Meta. There might be relevant details there: https://meta.discourse.org/t/user-flair-remains-after-user-is-removed-from-group/197293/25 |
spec/models/group_user_spec.rb
Outdated
| @@ -224,4 +224,18 @@ def levels | |||
| expect(group.group_users.find_by(user_id: user_2.id).first_unread_pm_at).to eq_time(10.minutes.ago) | |||
| end | |||
| end | |||
|
|
|||
| describe '#destroy!' do | |||
| it 'removes `primary_group_id`, `flair_group_id` and `title` for user' do | |||
There was a problem hiding this comment.
I'm not sure if this is correct because the GroupUser callbacks will only remove primary_group_id for the user. I think the relevant assertions should live in the test files of the model where the callbacks are present.
There was a problem hiding this comment.
I'm also confused now :) The bug we're trying to fix here is happening whenever the GroupUser's destroy method is called. Even if I write this in the group model's spec then I have to check it against the group_user.destroy! method only. I guess it's correct since I want to make sure group_user.destroy! is removing the values from user.flair_group_id and user.title.
There was a problem hiding this comment.
Sorry where are the callbacks being set for User#flair_group_id and User#title when group_user.destroy! is called?
There was a problem hiding this comment.
user.update_attribute(:primary_group_id, nil) method will trigger the before_save callback in user model which will run the match_primary_group_changes method. It removes these field values if possible.
There was a problem hiding this comment.
callback in
usermodel which will run thematch_primary_group_changesmethod. It removes these field values if possible.
In this case, I think those assertion should be on the User model and not be tested as a side effect of the GroupUser model.
There was a problem hiding this comment.
I'll split it up to two tests here. A callback on the User model is not a concern for the GroupUser model to test for because the GroupUser's model only concern is to run a callback that updates User#primary_group_id. What happens due to a change in User#primary_group_id is that of the User's model concern because it has a callback that is observing for that change.
There was a problem hiding this comment.
Let's assume I create two tests here. One test is checking the removal of primary_group_id. And another one in the user model is checking the removal of flair_group_id when the primary_group_id is changed.
In future, if someone used DB.exec method again here or used user.update_column(:primary_group_id, nil) method, then both the tests will pass but the bug I'm trying to fix in this PR will popup again.
There was a problem hiding this comment.
ahh ic what you mean now. This is certainty tricky and if I have a choice I would avoid all these nested callbacks we have on our models now. Anyway, let's just go with the simple change here first and we can revisit in the future.
There was a problem hiding this comment.
Yes. I even want to disable all other ways to remove/join a user in a group except group.remove(user) and group.add(user). Things getting complex since we have multiple ways to handle user's membership in a group.
There was a problem hiding this comment.
Yea that was my thought too. There should only be one way to add or remove a user from a group just like how we do it for PostCreator.
vinothkannans
changed the title
update_attribute instead of mini sql.update_attribute instead of mini sql.
The "save" method will trigger the before_save callback "match_primary_group_changes" for the User model. Else
flair_group_idwon't be removed from the user.