FIX: use active record `update_attribute` instead of mini sql. (PR #14367)

The “save” method will trigger the before_save callback “match_primary_group_changes” for the User model. Else flair_group_id won’t be removed from the user.

GitHub

The title of this pull request changed from “DEV: use active record save! instead of mini sql.” to "DEV: use active record update_attribute instead of mini sql.

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

Will you be able to provide context on why this line is no longer required as it is not clear at the moment.

Will you be able to provide some context on why this is no longer required?

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.

I’m also confused now :slight_smile: 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.

The title of this pull request changed from “DEV: use active record update_attribute instead of mini sql.” to "FIX: use active record update_attribute instead of mini sql.

Sorry where are the callbacks being set for User#flair_group_id and User#title when group_user.destroy! is called?

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.

callback in user model which will run the match_primary_group_changes method. 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.

I understand what you’re saying. Still, I want to make sure the before_save method is being triggered when run the destroy method for group_user. Let me check.

Do you mean the after_destroy :remove_primary_group callback ?

No, I mean the before_save callback of the user model. https://github.com/discourse/discourse/pull/14367/commits/b81d8ba9bd676f44a3b7afeb4f7f8028dce22868 I hope this is a correct approach here.

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.

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.

Now 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.

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.

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.