Page MenuHomePhabricator

Share logic between Special:UserRights and Special:GlobalGroupMembership
Closed, ResolvedPublic

Description

Background

Special:UserRights and Special:GlobalGroupMembership are very similar pages, that allow users to add, remove, or change expiry times for another user's groups.

The PHP classes SpecialUserRights and SpecialGlobalGroupMembership share a lot of code in common, and it is often necessary to update both in parallel when introducing or updating a feature. This is a maintenance burden.

It should be possible for these classes to share a lot of the code, so that it only needs updating in one place.

Technical notes

The execute methods and the methods for form building and processing are very similar.

Form building is done by building HTML directly rather than via component libraries. This should be updated, once the code is shared - T117884.

The main differences between the pages are:

  • UserRights has logic to handle which rights the performer can change, whereas GlobalGroupPermissions checks a single right
  • GlobalGroupPermissions handles automatic global groups
  • UserRights handles cross-wiki target users
  • The pages use different services for handling local/global groups, e.g. UserGroupManager vs GlobalGroupLookup
  • UserRights expects to be extendable (e.g. canProcessExpiries) - however nothing extends it
  • The pages run different hooks from doSaveUserGroups
  • The pages use different log types

Acceptance criteria

  • Logic is shared between SpecialUserRights and SpecialGlobalGroupMembership where possible
  • There are no functional changes to either page

Details

Related Changes in Gerrit:
SubjectRepoBranchLines +/-
mediawiki/coremaster+9 -33
mediawiki/coremaster+4 -50
mediawiki/extensions/CentralAuthmaster+1 -8
mediawiki/extensions/CentralAuthmaster+19 -24
mediawiki/coremaster+17 -7
mediawiki/extensions/CentralAuthmaster+2 -21
mediawiki/coremaster+49 -28
mediawiki/extensions/CentralAuthmaster+48 -31
mediawiki/coremaster+39 -22
mediawiki/coremaster+45 -27
mediawiki/extensions/CentralAuthmaster+14 -34
mediawiki/extensions/CentralAuthmaster+23 -90
mediawiki/extensions/CentralAuthmaster+3 -10
mediawiki/coremaster+26 -37
mediawiki/coremaster+17 -14
mediawiki/coremaster+15 -22
mediawiki/coremaster+31 -30
mediawiki/extensions/CentralAuthmaster+43 -101
mediawiki/extensions/CentralAuthmaster+9 -48
mediawiki/extensions/CentralAuthmaster+50 -135
mediawiki/extensions/CentralAuthmaster+57 -75
mediawiki/extensions/CentralAuthmaster+318 -0
mediawiki/coremaster+134 -226
mediawiki/coremaster+58 -79
mediawiki/coremaster+249 -71
mediawiki/coremaster+73 -116
mediawiki/coremaster+407 -0
mediawiki/extensions/CentralAuthmaster+32 -302
mediawiki/extensions/CentralAuthmaster+649 -0
mediawiki/coremaster+47 -7
mediawiki/coremaster+42 -32
mediawiki/coremaster+101 -8
mediawiki/coremaster+26 -155
mediawiki/coremaster+741 -0
Show related patches Customize query in gerrit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

I wonder if they could extend a common abstract special page, rather than one extending the other.

It should be done carefully to support the pages being different, but ideally it should be difficult for the special pages to accidentally diverge from each other (as Special:Contributions and Special:DeletedContributions did, for example - they were copy/pastes at some point, then the search filters for Special:Contributions got a lot more complex and left Special:DeletedContributions behind).

They are very similar as special pages. Both need a target form, a way to display the groups to users who can't change them, a form for changing groups, group-specific permission checks, logging, displaying log extracts.

That would make sense as well, I just wanted to provide some historical context, particularly since you mentioned that SpecialUserRights look kind-of extendable but nothing extends it.

Ideally there would still be a helper class for performing the operation, so that the API code doesn't have to call the special page code, but maybe that is better done at another time.

I think the best starting point would be to introduce an abstract base class, where we could put code from both special pages incrementally, patch by patch.

This way, we could start by sharing those pieces of code that are identical or almost identical and eventually leaving in subclasses only the differences between their logic. Later, based on that, there may be created some helper classes to be used by API (again, possibly as a base class and two concrete ones).

I'll create and claim a task for merging the UI code into a new base class, so that we could track progress on it more easily. The UI is almost the same in both, so it serves as a good starting point IMO.

That would make sense as well, I just wanted to provide some historical context, particularly since you mentioned that SpecialUserRights look kind-of extendable but nothing extends it.

Thank you yes, that helped clear up what was going on!

Ideally there would still be a helper class for performing the operation, so that the API code doesn't have to call the special page code, but maybe that is better done at another time.

Agreed.

Change #1194889 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/core@master] Introduce UserGroupAssignmentService

https://gerrit.wikimedia.org/r/1194889

Change #1194937 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/core@master] Use UserGroupAssignmentService for saving user groups

https://gerrit.wikimedia.org/r/1194937

Change #1195167 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/core@master] UserGroupAssignmentService: More checks for assigning/having groups

https://gerrit.wikimedia.org/r/1195167

Change #1195190 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/core@master] Introduce MultiFormatUserIdentityLookup

https://gerrit.wikimedia.org/r/1195190

Change #1195201 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/core@master] Use MultiFormatUserIdentityLookup to get user whose groups to change

https://gerrit.wikimedia.org/r/1195201

Change #1195690 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/extensions/CentralAuth@master] Create GlobalGroupAssignmentService

https://gerrit.wikimedia.org/r/1195690

Change #1195691 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/extensions/CentralAuth@master] Use GlobalGroupAssignmentService

https://gerrit.wikimedia.org/r/1195691

Change #1196011 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/extensions/CentralAuth@master] Create CentralAuthUserLookup service

https://gerrit.wikimedia.org/r/1196011

Change #1196012 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/extensions/CentralAuth@master] Use CentralAuthUserLookup for fetching target of global groups change

https://gerrit.wikimedia.org/r/1196012

Change #1196036 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/core@master] UserRights: Read the form in the parent class

https://gerrit.wikimedia.org/r/1196036

Change #1194889 merged by jenkins-bot:

[mediawiki/core@master] Introduce UserGroupAssignmentService

https://gerrit.wikimedia.org/r/1194889

Change #1194937 merged by jenkins-bot:

[mediawiki/core@master] Use UserGroupAssignmentService for saving user groups

https://gerrit.wikimedia.org/r/1194937

Change #1196138 had a related patch set uploaded (by Tchanders; author: Tchanders):

[mediawiki/core@master] Use UserGroupAssignmentService to check whether a user can change rights

https://gerrit.wikimedia.org/r/1196138

Change #1195167 merged by jenkins-bot:

[mediawiki/core@master] UserGroupAssignmentService: More checks for assigning/having groups

https://gerrit.wikimedia.org/r/1195167

Change #1196457 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/core@master] UserRights: Reorder the code in ::execute method

https://gerrit.wikimedia.org/r/1196457

Change #1196458 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/core@master] UserRights: Prepare relevant state in ::execute

https://gerrit.wikimedia.org/r/1196458

Change #1196138 merged by jenkins-bot:

[mediawiki/core@master] Use UserGroupAssignmentService to check whether a user can change rights

https://gerrit.wikimedia.org/r/1196138

Change #1196642 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/extensions/CentralAuth@master] S:GlobalGroupMembership: Reorder code in ::execute method

https://gerrit.wikimedia.org/r/1196642

Change #1196800 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/core@master] UserGroupAssignmentService: Skip contradicting changes

https://gerrit.wikimedia.org/r/1196800

Change #1195690 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] Create GlobalGroupAssignmentService

https://gerrit.wikimedia.org/r/1195690

Change #1195691 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] Use GlobalGroupAssignmentService for saving groups

https://gerrit.wikimedia.org/r/1195691

Change #1196874 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/extensions/CentralAuth@master] S:GlobalGroupMembership: Prepare relevant state in ::execute

https://gerrit.wikimedia.org/r/1196874

Change #1196879 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/extensions/CentralAuth@master] S:GlobalGroupMembership: Use parent class method to read form state

https://gerrit.wikimedia.org/r/1196879

Change #1196800 merged by jenkins-bot:

[mediawiki/core@master] UserGroupAssignmentService: Skip contradicting changes

https://gerrit.wikimedia.org/r/1196800

Change #1196887 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/core@master] UserGroupsSpecialPage: Simplify the variety of target name types

https://gerrit.wikimedia.org/r/1196887

Change #1195190 merged by jenkins-bot:

[mediawiki/core@master] Introduce MultiFormatUserIdentityLookup

https://gerrit.wikimedia.org/r/1195190

Change #1196915 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/core@master] UserGroupsSpecialPage: Drop target param in more methods

https://gerrit.wikimedia.org/r/1196915

Change #1197128 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/core@master] UserGroupsSpecialPage: Further reduce usage of the target parameters

https://gerrit.wikimedia.org/r/1197128

Change #1197129 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/core@master] UserGroupsSpecialPage: Drop target from showLogFragment signature

https://gerrit.wikimedia.org/r/1197129

Change #1197203 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/extensions/CentralAuth@master] S:GlobalGroupMembership: Remove use of UserGroupsSpecialPageTarget

https://gerrit.wikimedia.org/r/1197203

Change #1195201 merged by jenkins-bot:

[mediawiki/core@master] Use MultiFormatUserIdentityLookup to get user whose groups to change

https://gerrit.wikimedia.org/r/1195201

Change #1197535 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/extensions/CentralAuth@master] Remove usages of SpecialGlobalGroupMembership as a service

https://gerrit.wikimedia.org/r/1197535

Change #1197536 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/extensions/CentralAuth@master] SpecialGlobalGroupMembership: Update constructor signature

https://gerrit.wikimedia.org/r/1197536

Change #1196036 merged by jenkins-bot:

[mediawiki/core@master] UserRights: Read the form in the parent class

https://gerrit.wikimedia.org/r/1196036

Change #1196457 merged by jenkins-bot:

[mediawiki/core@master] UserRights: Reorder the code in ::execute method

https://gerrit.wikimedia.org/r/1196457

Change #1196458 merged by jenkins-bot:

[mediawiki/core@master] UserRights: Prepare relevant state in ::execute

https://gerrit.wikimedia.org/r/1196458

Change #1196011 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] Create CentralAuthUserHelper service

https://gerrit.wikimedia.org/r/1196011

Change #1196012 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] Use CentralAuthUserHelper for fetching target of global groups change

https://gerrit.wikimedia.org/r/1196012

Change #1196642 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] S:GlobalGroupMembership: Reorder code in ::execute method

https://gerrit.wikimedia.org/r/1196642

Change #1196887 merged by jenkins-bot:

[mediawiki/core@master] UserGroupsSpecialPage: Simplify the variety of target name types

https://gerrit.wikimedia.org/r/1196887

Change #1196874 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] S:GlobalGroupMembership: Prepare relevant state in ::execute

https://gerrit.wikimedia.org/r/1196874

Change #1196879 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] S:GlobalGroupMembership: Use parent class method to read form state

https://gerrit.wikimedia.org/r/1196879

Change #1197661 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/core@master] UserRights: Move initialization to a separate method

https://gerrit.wikimedia.org/r/1197661

Change #1197662 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/core@master] UserRights: Move success message generation to parent class

https://gerrit.wikimedia.org/r/1197662

Change #1196915 merged by jenkins-bot:

[mediawiki/core@master] UserGroupsSpecialPage: Drop target param in more methods

https://gerrit.wikimedia.org/r/1196915

Change #1197128 merged by jenkins-bot:

[mediawiki/core@master] UserGroupsSpecialPage: Further reduce usage of the target parameters

https://gerrit.wikimedia.org/r/1197128

Change #1197129 merged by jenkins-bot:

[mediawiki/core@master] UserGroupsSpecialPage: Drop target from showLogFragment signature

https://gerrit.wikimedia.org/r/1197129

Change #1197535 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] Remove usages of SpecialGlobalGroupMembership as a service

https://gerrit.wikimedia.org/r/1197535

Change #1197536 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] SpecialGlobalGroupMembership: Update constructor signature

https://gerrit.wikimedia.org/r/1197536

Change #1198015 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/core@master] UserRights: Move conflict check logic to parent class

https://gerrit.wikimedia.org/r/1198015

Change #1197661 merged by jenkins-bot:

[mediawiki/core@master] UserRights: Move initialization to a separate method

https://gerrit.wikimedia.org/r/1197661

Change #1197662 merged by jenkins-bot:

[mediawiki/core@master] UserRights: Move success message generation to parent class

https://gerrit.wikimedia.org/r/1197662

Change #1198031 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/extensions/CentralAuth@master] S:GlobalGroupMembership: Move initialization to a separate method

https://gerrit.wikimedia.org/r/1198031

Change #1198046 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/extensions/CentralAuth@master] S:GlobalGroupMembership: Use parent methods for success message

https://gerrit.wikimedia.org/r/1198046

Change #1198015 merged by jenkins-bot:

[mediawiki/core@master] UserRights: Move conflict check logic to parent class

https://gerrit.wikimedia.org/r/1198015

Change #1198031 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] S:GlobalGroupMembership: Move initialization to a separate method

https://gerrit.wikimedia.org/r/1198031

Change #1198046 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] S:GlobalGroupMembership: Use parent methods for success message

https://gerrit.wikimedia.org/r/1198046

Change #1198063 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/extensions/CentralAuth@master] S:GlobalGroupMembership: Defer conflict checking to the parent class

https://gerrit.wikimedia.org/r/1198063

Change #1198072 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/core@master] UserGroupsSpecialPage: Add method for setting changeable groups

https://gerrit.wikimedia.org/r/1198072

Change #1198073 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/extensions/CentralAuth@master] S:GlobalGroupMembership: Use ::setChangeableGroups

https://gerrit.wikimedia.org/r/1198073

Change #1198205 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/core@master] Remove UserGroupsSpecialPageTarget

https://gerrit.wikimedia.org/r/1198205

Change #1198208 had a related patch set uploaded (by Mszwarc; author: Mszwarc):

[mediawiki/core@master] UserGroupsSpecialPage: Drop unneeded trivial getters

https://gerrit.wikimedia.org/r/1198208

I think I've finished working on this. The classes SpecialUserRights and SpecialGlobalGroupMembership were refactored, with a good portion of their code being extracted to a new base class, UserGroupsSpecialPage. Additionally, UserGroupAssignmentService and GlobalGroupAssignmentService were created to house the code responsible for actual operations on the groups the user is in.

After the changes, the special pages are focused just on displaying and processing the input from UI and no longer called as services. At a conceptual level, the special pages and the assignment services still do share the same ideas and the same high-level approach to handling things. However, editing user groups for local and global users is done using different access patterns and with objects of different interfaces, which reduces the possiblity to merge the code. I think the current state of the relevant classes is a good point to stop at and continue other work. Going further with sharing the logic may require deeper refactorings around the CentralAuthUser class and creation of additional supporting services, which will resemble the core's ones.

At the moment, UserGroupsAssignmentService and GlobalGroupsAssignmentService don't share any common base class. With a bit of work, it may be possible to create a minimal common base class for them. I'm not planning to do this as a part of this task, but the need for such a place for common logic may arise during our further works on the group restriction enforcement project.

I'm moving this task on our sprint board to the "Needs review" column, so that the remaining patches may get reviewed and the overall state of the code after the refactoring may also be looked at and reviewed by someone.

Change #1198063 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] S:GlobalGroupMembership: Defer conflict checking to the parent class

https://gerrit.wikimedia.org/r/1198063

Change #1198072 merged by jenkins-bot:

[mediawiki/core@master] UserGroupsSpecialPage: Add method for setting changeable groups

https://gerrit.wikimedia.org/r/1198072

Change #1198073 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] S:GlobalGroupMembership: Use ::setChangeableGroups

https://gerrit.wikimedia.org/r/1198073

Change #1198205 merged by jenkins-bot:

[mediawiki/core@master] Remove UserGroupsSpecialPageTarget

https://gerrit.wikimedia.org/r/1198205

Change #1198208 merged by jenkins-bot:

[mediawiki/core@master] UserGroupsSpecialPage: Drop unneeded trivial getters

https://gerrit.wikimedia.org/r/1198208

mszwarc removed a project: Patch-For-Review.

Everything has been reviewed, moving to done.