FIX: Allow only groups with flairs to be selected (PR #13744)

It used the same permission check as for primary groups which is wrong because not all groups that can be primary have a flair.

GitHub

Maybe they want to use a flair of an automatic group?

Is there a reason to use be_falsey here since the method will always return a boolean? be_falsey is only useful when false or nil can be returned.

      user.update!(groups: [group])

I don’t think this line will affect the outcome of the assertions below. My suggestion is to just remove it.

I don’t think this line is required too since we don’t leak state between tests.

      group.update!(flair_icon: nil)
      user.update!(groups: [group])

Minor but instead of creating an AR object here, can we instead only pluck the required columns?

be_falsey and be_truthy are used throughout the whole file.

O icic. Consistency is fine but do take note that some of the code in our codebase has been written a long time ago so it may not necessarily make sense in today’s context. Anyway, it isn’t a big problem here so I’ll resolve this :slight_smile:

Sorry I know I said to pluck the columns previously but I realized the logic can be done in SQL land.

Group.where(id: group_id.to_i).where("flair_icon IS NOT NULL OR flair_upload_id IS NOT NULL").exists?

If you do not mind, I would like to keep this as it is as I find it easier to comprehend. There is going to be a similar query either way (‘SELECT 1’ or ‘SELECT flair_icon, flair_upload_id’), so this looks like a micro-optimization to me.

Moreover, your suggestion is not equivalent to the initial code. IS NOT NULL returns true for empty string, but present? returns false.

I don’t think those columns will ever have a blank string as its value but I understand your concern :+1:

They will have a blank string everytime an admin adds and then removes the flair.