442045 - Bugzilla::User::flatten_group_membership should be using the fast code from User::groups

Status: RESOLVED INVALID

(Bugzilla :: User Accounts, enhancement)

Description

[Max Kanat-Alexander]

For some reason flatten_group_membership has its own code, when really it should just be using the code that's already in User::groups.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

Here we go, easy as pie. :-)

Comment 2

[Frédéric Buclin]

Comment on attachment 326921 [details] [diff] [review]
v1

Wow, this change is completely wrong. You misunderstood what flatten_group_membership() does. groups() returns all groups you belong to thanks to your direct membership to some given groups and then letting group inheritence giving you indirect membership. flatten_group_membership() does exactly the opposite! Given a group, it tells you in which groups you must belong to in order to indirectly belong to the given group thanks to group inheritence. To make this clear:

Assume all members of the admin group are automatically in the editbugs group, and that members of the editbugs group are automatically in the canconfirm group. If you explicitly belong to the editbugs group only, then $user->groups() will return both editbugs and canconfirm, because you indirectly belong to the canconfirm group thanks to group inheritence. Now, flatten_group_membership($editbugs_gid) will return both admin and editbugs groups, because those are the groups you must belong to in order to be (directly or indirectly) in the editbugs group.

It's the same distinction as in bugs blocking and bugs depending on another bug.

Comment 3

[Max Kanat-Alexander]

Ah, okay. That subroutine should clearly be in Bugzilla::Group then, and not Bugzilla::User.

Comment 4

[Frédéric Buclin]

(In reply to comment #3)
> That subroutine should clearly be in Bugzilla::Group then, and not
> Bugzilla::User.

Exactly, I agree!