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:
- We use it when a user joins a public group:
- 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.
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: