DEV: extract join_group method from groups#add_members method (PR #13807)

We wanted to do a simple refactoring in groups#add_members method – we wanted to extract the code in the beginning of the method into the guardian. But looking into this, I’ve found out that actually all checks already extracted into guardians and the reason we have so much lines of checking code is that actually the groups#add_members method serve two different purposes:

  1. We use it when a user joins a public group: Screenshot 2021-07-21 at 13 03 30
  2. We use it when an admin wants to add users to a group:

It’s better to split this logic into two methods, instead of moving this mix deeper into the guardian, and this PR does this.

It’s not like these two operations are completely different, but when a user (probably unprivileged) joins a public group and an admin adds users to a group, it isn’t the same. Mixing it together makes code harder to follow and support. If I were to make some changes to joining logic, I need to read the code for bulk adding users to groups too. If I change the first logic, I risk breaking the second logic. Taking into account that the second logic here is for privileged users, it increases risk of security bugs.

Additionally, the join method can be more lightweight. It needs fewer checks on the server side. There is no need to return data from the server. It can be just idempotent PUT that returns just a status code. The profiler doesn’t show a very big increase in speed, though. Before:

After:

GitHub

Are you purposely swallowing the promise here? join() will return before findMembers is finished. I’d recommend adding return or using the one line version: }).then(() => this.findMembers());

Looks good :+1:

Mostly aesthetic but the begin and end are not necessary here. A function can have rescue without them.

Thoughtlessly copied this from neighbor methods. I’ve fixed it only in this method by now, but I’ll take a look and fix other methods a bit later too.

Why do we need to call findMembers after the user has joined the group?

This can be a one-liner

    return if already_in_group = group.users.exists?(id: current_user.id)

I think that a variable with explainable name is better than a comment that can easily become outdated. What do you think?

Since the variable is never used, there’s not point in declaring and instanciating it. Up to you if you want to keep the comment. I think it’s fine in that case as it makes the code a little bit clearer.

Sadly, the code around of this place relies on side-effects of this method. We need to call it to get UI updated. And, by the way, I’ve reverted the commit that make it return the promise instead of swallowing it (see @eviltrout comment above) because it caused timeout when running client side tests.

That’s quite weird - we should add a TODO to fix that because we shouldn’t be relying on swallowing a promise.

Fixed :ballot_box_with_check:

Yep, I agree, I’m going to address all these methods that call findMembers altogether. Will create a todo.