FIX: Remove superfluous topic allowed users on group invite (PR #14656)

When inviting a group to a topic, there may be members of the group already in the topic as topic allowed users. These can be safely removed from the topic, because they are implicitly allowed in the topic based on their group membership.

Also, this prevents issues with group SMTP emails, which rely on the topic_allowed_users of the topic to send to and cc’s for emails, and if there are members of the group as topic_allowed_users then that complicates things and causes odd behaviour.


Need to exclude the OP of the topic from this behaviour.

I don’t think we need the distinct here since there is a unique index on topic_id,user_id so users are unique for a given topic.

    allowed_users_to_remove.find_each do |allowed_user|

Just to be safer since we never know what kind of PMs exist in the wild.

I’m thinking we should either run this in a defer queue or in Sidekiq since the operations here can be deferred.

Just wanted to highlight that this will create a small post action saying that the user left or a user has been removed which seems weird since neither is true.

I think it’ll be more efficient if we do topic_allowed_users.topic_id = :topic_id instead so that PG doesn’t need to join the entire table against topic_allowed_groups before filtering rows based on the :topic_id. Can you see if this changes the query plan for the planner?

Not sure if we need this since extra_user2 == tau that has been fabricated above.

I think it’ll be better to assert that topic.allowed_users includes the right users as your assertion will still pass if we accidentally delete all topic.allowed_users for the given topic.

Yep I will try that out.

Also I think we can fabricate another user that is not part of the group to make sure the users’ topic_allowed_user record is not deleted.

Well they were removed automatically, I thought we would want some sort of log for it? I thought it would be confusing for people to be removed from the topic silently when the group is added.

I was thinking this initially but then the UI will not update the list of participants of the topic after the group is invited which could be confusing.

Good point, will change.

Just to clarify, do we need to use the usernames here instead of just using the user ids?

Ah yes good call, will fix.

I will change to extra_user2 = tau.user, because topic.allowed_users actually returns an array of users whereas topic.topic_allowed_users returns the array of topic_allowed_user records :slight_smile:

Yea this is certainly tricky but I guess we can live with it until people complain.

Ah sorry this was leftover from when I was passing usernames directly to remove_allowed_user, will update.

O this is true, any delay can be confusing.