Remove GlobalBlockingHooks::onGetUserPermissionsErrorsExpensive in favour of GlobalBlockingHooks::onGetUserBlock
Closed, ResolvedPublic3 Estimated Story Points
Actions

Assigned To

Authored By

	Tchanders
	Nov 11 2022, 5:41 PM

Description

Background

GlobalBlocking's handler for the GetUserPermissionsErrorsExpensive hook is no longer needed, now that global blocks are added via the GetUserBlock hook, since T257701: Add global blocks into CompositeBlocks rather than treating them separately.

Why is it no longer needed?

GetUserPermissionsErrorsExpensive is fired from PermissionManager::checkPermissionHooks.
GetUserBlock is eventually fired whenever from PermissionManager::checkUserBlock is called.
PermissionManager::checkPermissionHooks and PermissionManager::checkUserBlock are both called together from the PermissionManager, here: https://gerrit.wikimedia.org/g/mediawiki/core/+/f53279b7f5cad944ae3a53fdd4e8111acd34a7f4/includes/Permissions/PermissionManager.php#451

At the moment, if a globally blocked user tries to edit, their block is found twice, via the permission and the block checks:

two_block_messages.png (439×1 px, 96 KB)

Acceptance criteria

A globally blocked user is still unable to edit
A globally blocked user sees only one block message, if they only have a global block

Details

	Subject	Repo	Branch	Lines +/-
	Remove GlobalBlockingHooks::onGetUserPermissionsErrorsExpensive	mediawiki/extensions/GlobalBlocking	master	+0 -33

Customize query in gerrit

Related Objects

Mentioned In: T344463: Local block messages should override global ones
T323088: Global blocks should be displayed below local blocks
T257701: Add global blocks into CompositeBlocks rather than treating them separately
Mentioned Here: T257701: Add global blocks into CompositeBlocks rather than treating them separately

Event Timeline

Tchanders created this task.Nov 11 2022, 5:41 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 11 2022, 5:41 PM

Tchanders mentioned this in T257701: Add global blocks into CompositeBlocks rather than treating them separately.Nov 11 2022, 5:41 PM

dom_walden mentioned this in T323088: Global blocks should be displayed below local blocks.Nov 15 2022, 4:20 PM

TheresNoTime subscribed.Nov 15 2022, 5:18 PM

Tchanders moved this task from Untriaged to Triage/To be Estimated on the Anti-Harassment board.Nov 22 2022, 6:18 PM

ARamirez_WMF renamed this task from Remove GlobalBlockingHooks::onGetUserPermissionsErrorsExpensive in favour of GlobalBlockingHooks::onGetUserBlock to Investigate: Remove GlobalBlockingHooks::onGetUserPermissionsErrorsExpensive in favour of GlobalBlockingHooks::onGetUserBlock.Nov 22 2022, 6:51 PM

ARamirez_WMF moved this task from Triage/To be Estimated to AHaT Sprint 21: The Bruntal Hat on the Anti-Harassment board.

ARamirez_WMF edited projects, added Anti-Harassment ( AHaT Sprint 21: The Bruntal Hat); removed Anti-Harassment.

Tchanders claimed this task.Nov 23 2022, 2:05 PM

Tchanders moved this task from Ready 🎬 (ONLY IF YOU HAVE NO MORE CODE TO REVIEW) to In Progress 💪 on the Anti-Harassment ( AHaT Sprint 21: The Bruntal Hat) board.

Tchanders renamed this task from Investigate: Remove GlobalBlockingHooks::onGetUserPermissionsErrorsExpensive in favour of GlobalBlockingHooks::onGetUserBlock to Remove GlobalBlockingHooks::onGetUserPermissionsErrorsExpensive in favour of GlobalBlockingHooks::onGetUserBlock.Nov 28 2022, 4:01 PM

Tchanders set the point value for this task to 3.

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

[mediawiki/extensions/GlobalBlocking@master] Remove GlobalBlockingHooks::onGetUserPermissionsErrorsExpensive

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

gerritbot added a project: Patch-For-Review.Nov 28 2022, 4:17 PM

I investigated whether it was safe to remove this hook handler. I believe it is, based on the logic from the task description, so have written a patch for it. Reviewers should confirm that it is safe to remove this handler.

Tchanders moved this task from In Progress 💪 to Code Review 🔍 on the Anti-Harassment ( AHaT Sprint 21: The Bruntal Hat) board.Nov 28 2022, 4:18 PM

• AGueyte moved this task from Code Review 🔍 to QA/Testing 🐞 on the Anti-Harassment ( AHaT Sprint 21: The Bruntal Hat) board.Nov 29 2022, 3:12 PM

Change 861421 merged by jenkins-bot:

[mediawiki/extensions/GlobalBlocking@master] Remove GlobalBlockingHooks::onGetUserPermissionsErrorsExpensive

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

Maintenance_bot removed a project: Patch-For-Review.Nov 29 2022, 3:30 PM

ReleaseTaggerBot added a project: MW-1.40-notes (1.40.0-wmf.13; 2022-12-05).Nov 29 2022, 4:01 PM

@Tchanders I validated that I was receiving a single block message when trying to edit. I tested global block IP, range, XFF, local sitewide, and composite blocks as seen in the screenshots below. Please let me know if there is any other testing that you want me to do. If not, is it ok for me to move this to Done?

Test Environment: Local
Browser: Chrome

Global Block IP

T322941_GB_Edit_GB_IP_Chrome.png (587×2 px, 203 KB)

Global Block IP Range

Global Block IP XFF

Local Block Sitewide

Composite Block

T322941_GB_Edit_Composite_Chrome.png (578×2 px, 188 KB)

ARamirez_WMF edited projects, added Anti-Harassment (AHaT Sprint 22: Asian Conical Hat); removed Anti-Harassment ( AHaT Sprint 21: The Bruntal Hat).Dec 6 2022, 3:52 PM

ARamirez_WMF moved this task from Ready 🎬 (ONLY IF YOU HAVE NO MORE CODE TO REVIEW) to QA/Testing 🐞 on the Anti-Harassment (AHaT Sprint 22: Asian Conical Hat) board.

@GMikesell-WMF Thanks, I've moved this on

Tchanders closed this task as Resolved.Mar 21 2023, 2:12 PM

jrbs mentioned this in T344463: Local block messages should override global ones.Aug 23 2023, 10:50 PM

	F35826100: T322941_GB_Edit_Composite_Chrome.png
	Dec 1 2022, 1:10 AM

	F35826097: T322941_GB_Edit_Sitewide_Chrome.png
	Dec 1 2022, 1:10 AM

	F35826095: T322941_GB_Edit_XFF_Chrome.png
	Dec 1 2022, 1:10 AM

	F35826093: T322941_GB_Edit_RangeIP_Chrome.png
	Dec 1 2022, 1:10 AM

	F35826091: T322941_GB_Edit_GB_IP_Chrome.png
	Dec 1 2022, 1:10 AM

Remove GlobalBlockingHooks::onGetUserPermissionsErrorsExpensive in favour of GlobalBlockingHooks::onGetUserBlockClosed, ResolvedPublic3 Estimated Story PointsActions