It used the same permission check as for primary groups which is wrong because not all groups that can be primary have a flair.
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
nil can be returned.
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.
Minor but instead of creating an AR object here, can we instead only pluck the required columns?
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
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
They will have a blank string everytime an admin adds and then removes the flair.