465844 - Bugzilla::User->get_userlist() should accept an optional group as argument and only return members of this group

Status: NEW

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Frédéric Buclin]

Bugzilla::FlagType->grant_list currently calls Bugzilla::User->get_userlist to get all active and visible users, and then creates User objects one by one to check who is allowed to grant/deny a flag. This code couldn't be slower! We should modify get_userlist() to 1) return a list of user objects, and 2) to accept a group to restrict the query to members of this group only. This would let grant_list() directly pass the group to restrict the query to, instead of calling can_set_flag() for each visible user.

Comment 1

[arbingersys]

(In reply to comment #0)
Hi, I'm poking around at this bug, and I'm wondering if returning a list of User objects from get_userlist() is the best approach. I recently submitted a patch for the following bug that also modifies get_userlist(),

https://bugzilla.mozilla.org/show_bug.cgi?id=465818

and it seems to me that returning User objects instead of the simple data it currently returns makes get_userlist() unnecessarily bulky and less flexible, especially in regard to its usage elsewhere in the code.

Of course at this point I don't know if the patch I submitted above is on the right track, but my take is this: if you simply modify get_userlist() to take an optional group ID param, and only return those users, you eliminate the need to call can_set_flag() at all, and reduce processing to the bare minimum -- in grant_list(), you just instantiate the User objects  you get back and return them.

Thanks, James

Comment 2

[Max Kanat-Alexander]

You could just instantiate a Bugzilla::Group and use one of the members_ functions on it.

Comment 3

[miketosh]

(In reply to comment #2)
> You could just instantiate a Bugzilla::Group and use one of the members_
> functions on it.

There is no members_inherited yet.  I have part of my [rejected] patch for 287332, which could fill that role. :-)