Group Assignments Summary (PR #70)

New feature: group-assignments-summary

GitHub

The title of this pull request changed from “Dev” to "Group Assignments Summary

Why is :group_assignments being added as a before_action? That means the group_assignments function will be called on every request handled by this controller

Rather than wrapping this all in a huge if statement, you could do

raise Discourse::InvalidAccess.new if !SiteSetting.assign_enabled?

(Also, maybe we should use Discourse::NotFound, since 404 is a better error code for this situation)

There is lots of repetition in the if and else part of this statement. Can we move some stuff outside the if statement?

Why are we using the group_name parameter to describe a user? That is kind of confusing.

When requesting the topics for a single user, do we need to use this endpoint? Or could we re-use the existing per-user endpoint?

What does this parameter do? Can we name it something more descriptive?

What happens if a group has lots (e.g. 50+) members. Does this still work?

We don’t really want to be making ajax requests while rendering components. Let’s simplify this to match the per-user behaviour.

If current_user.can_assign is true, show the tab. If not, then hide it.

ooh, this was added by mistake. will remove it in next commit😅

We need to use this endpoint as existing one doesn’t allow getting data of other users unless current_user is admin

Changed as specified😀

Sure, it’s now done as specified in the new commit😀

I just removed it, as it wasn’t useful

Yes, in the new commit it works with 50+ users too.

Sure, it’s now done as specified in the new commit😀

Let’s try to avoid another AJAX request to load the assignments count. We should be able to include it in the group serializer

Something like add_to_serializer(:group, :assignments_count) in plugin.rb

Oh I think that is probably a mistake. The assign plugin was originally for staff-only, but now we made it so you can customize the groups. So instead of checking admin, can you change that route to check for current_user.can_assign?

Looks good :+1:

We just need to make sure it is secure. This should only be included for users where scope.can_assign? is true. (see here for a similar implementation)

A lot of this code seems similar to def assigned above. Can we share any logic between them?