Page MenuHomePhabricator

Update LogFormatter / BlockLogFormatter to support multiblocks
Closed, ResolvedPublic

Description

Modify the existing LogFormatter and/or BlockLogFormatter classes to support multiblocks.

When viewing Special:Log/block with $wgUseCodexSpecialBlock = true:

  • On old block log entries (except for unblock actions), there will be a single link labelled "manage blocks". This link will take you to Special:Block with the target field prefilled with the blocked username.
  • On new block log entries (except for unblock actions), there will be two links:
    • The first link will be labelled "remove block", and clicking this will take you to Special:Block, with URL parameters such that a modal box is displayed shortly after page load, asking you to confirm block removal. Confirming will remove the block referred to by the block log entry.
    • The second link will be labelled "manage blocks", and will take you to Special:Block with the target prefilled.

With $wgUseCodexSpecialBlock = false, there should be no change.

Log entries for the unblock action don't have these links, so there should be no change for them.

Previously this task also asked for updates to Special:Contributions. That has been split out to T384916.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

This proposal makes sense without "codexifying" it.

Change #1102669 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/core@master] block: Show a special log entry when blocking an already blocked target

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

Cparle updated the task description. (Show Details)

@MusikAnimal is it possible to instead direct the user to Special:Block with the block loaded, so they can remove the block from the Special:Block page?

MusikAnimal is it possible to instead direct the user to Special:Block with the block loaded, so they can remove the block from the Special:Block page?

Discussed on Slack, but yes a Special:Block?unblockId=123 is very doable, but we don't currently have something that works for no-JS users.

As discussed in standup on Dec 19, we plan to update LogFormatter so that instead of two links (1 link for Special:Unblock and 1 link for Special:Block), we want to move towards 1 "Manage block" link, which would pass the target and/or blockID into the Special:Block page, and pre-populate it with the target.

This will help us decrease traffic to Special:Unblock.

EDIT: It sounds like this proposal will not be worth the while from an engineering standpoint. See summary at T378150#10436068


I feel like this task needs to be resolved before T380092. T380092 is really just a copy edit change, and the same copy edits will need to be applied to the LogFormatter. So I will ask here:

As discussed in standup on Dec 19, we plan to update LogFormatter so that instead of two links (1 link for Special:Unblock and 1 link for Special:Block), we want to move towards 1 "Manage block" link, which would pass the target and/or blockID into the Special:Block page, and pre-populate it with the target.

This will help us decrease traffic to Special:Unblock.

When we have a list of individual blocks, such as Special:Log/block, would we not want to have both a "change block" and "remove block" links? The reason being, the admin already knows which block they want to act on, so we can do deep linking to save them from extra clicks at Special:Block.

So the changes to lists of blocks (such as Special:BlockList or Special:Log/block):

Old labelNew labelOld pathNew path
change blockchange blockSpecial:Block/UsernameSpecial:Block?id=123
unblockremove blockSpecial:Unblock/UsernameSpecial:Block?id=123&blockaction=unblock

Then, for messages about users such as at the very top of Special:Contribs, we'd want i.e. Username (talk | manage blocks) as you say (because in those situations there's no way to deep link):

Old labelNew labelOld pathNew path
blockmanage blocksSpecial:Block/UsernameSpecial:Block/Username
change blockchange blockSpecial:Block/UsernameSpecial:Block/Username
unblockmanage blocksSpecial:Unblock/UsernameSpecial:Block/Username

How does that sound?

On Special:Contributions, if there are zero blocks for a user, I would prefer to retain the label "block" in the interests of brevity. It's already very long, for me on Wikipedia it looks like

Results for Tim Starling (talk | block | block log | uploads | logs | deleted user contributions | user rights management | global block log | mass delete | global account | filter log)

In Special:Log/block, the blocks may not exist anymore when the log is viewed, so Special:Block?id=123 may lead to an error message. Maybe better to give the username as well, so that Special:Block can at least show the current blocks for the user. So "change block" would link to Special:Block/Username?id=123.

Getting the block ID at all in BlockLogFormatter is going to be a moderate nuisance. It's stored in a separate table (log_search), and we usually don't join on that table. LogPager::getQueryInfo() joins on it conditionally in a way that is not useful -- we would need a second join on the same table. Or a batch query done somewhere else. There are more queries in DatabaseLogEntry::newSelectQueryBuilder, DatabaseLogEntry::getSelectQueryData and ApiQueryLogEvents.

BlockLogFormatter is given a LogEntry, and the conventional way to construct one is using DatabaseLogEntry::newFromRow(). The usual flow in maybe 30 callers has no place for a batch query.

A batch query could be done lazily, but of course there is no context object where you would store the result.

It would be much simpler to copy the block ID into log_params. But there will be no ID-specific links for old log entries, we would retain the old links for those.

On Special:Contributions, if there are zero blocks for a user, I would prefer to retain the label "block" in the interests of brevity. It's already very long, for me on Wikipedia it looks like

Results for Tim Starling (talk | block | block log | uploads | logs | deleted user contributions | user rights management | global block log | mass delete | global account | filter log)

That seems fine to me. cc @JWheeler-WMF.

In Special:Log/block, the blocks may not exist anymore when the log is viewed, so Special:Block?id=123 may lead to an error message. Maybe better to give the username as well, so that Special:Block can at least show the current blocks for the user. So "change block" would link to Special:Block/Username?id=123.

Getting the block ID at all in BlockLogFormatter is going to be a moderate nuisance. It's stored in a separate table (log_search), and we usually don't join on that table. LogPager::getQueryInfo() joins on it conditionally in a way that is not useful -- we would need a second join on the same table. Or a batch query done somewhere else. There are more queries in DatabaseLogEntry::newSelectQueryBuilder, DatabaseLogEntry::getSelectQueryData and ApiQueryLogEvents.

BlockLogFormatter is given a LogEntry, and the conventional way to construct one is using DatabaseLogEntry::newFromRow(). The usual flow in maybe 30 callers has no place for a batch query.

A batch query could be done lazily, but of course there is no context object where you would store the result.

It would be much simpler to copy the block ID into log_params. But there will be no ID-specific links for old log entries, we would retain the old links for those.

Thanks for explaining! I guess since it'll be such a hassle, we won't bother with linking to specific blocks. The exception perhaps being the "snippet" of current blocks shown at Special:Contributions. That should ideally list all current blocks, similar to the task description. Could that be doable, since we now have the bt_count field with which we could set a LIMIT on the query ordered by timestamp, and safely know those N blocks are active?

I think there are quite a few different messages that we want to update, but they mostly all seem to come in these forms:

  • Contributions user links (the very top of Special:Contributions/Username)
    • This is a long list of links that appears like Results for Tim Starling (talk | block | block log | uploads | logs | deleted user contributions | user rights management | global block log | mass delete | global account | filter log)
    • Changes: If there are > 0 blocks, change the "block" link to "manage blocks"
  • Short user links
    • Appear like MusikAnimal (talk | contribs | block) or MusikAnimal (talk | contribs | unblock | change block)
    • Changes: for the blocked state, it would be MusikAnimal (talk | contribs | manage blocks). Unblocked state, no changes.
  • Action links
    • These are shown at the end of each line in block log entries, and under the "Expires" column at Special:BlockList
    • Appear like (unblock | change block)
    • Changes: (manage blocks) which simply links to Special:Block/Username

I still favor my idea at T378150#10428126, but per T378150#10435881 it sounds like that's going to be too much of an engineering challenge. If that is the case, let's stick with just manage blocks as Jack stated at T378150#10417143

Does the above sound correct?

Change #1108534 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/core@master] block: Update block action links in block log and contributions for multiblocks

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

Change #1102669 merged by jenkins-bot:

[mediawiki/core@master] block: Show a special log entry when blocking an already blocked target

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

  • Short user links
    • Appear like MusikAnimal (talk | contribs | block) or MusikAnimal (talk | contribs | unblock | change block)

The links from Linker::userToolLinks(), which look like this and appear on RecentChanges and various other places, don't do a block query. They just show "block" unconditionally.

I still favor my idea at T378150#10428126, but per T378150#10435881 it sounds like that's going to be too much of an engineering challenge. If that is the case, let's stick with just manage blocks as Jack stated at T378150#10417143

In the WIP patch I added the block ID to log_params and gave the links from Special:Log/block an id parameter. I think that solution is fine. It's just querying log_search that is not worth doing.

Old labelNew labelOld pathNew path
change blockchange blockSpecial:Block/UsernameSpecial:Block?id=123
unblockremove blockSpecial:Unblock/UsernameSpecial:Block?id=123&blockaction=unblock

I mentioned in a meeting with @Samwilson just now that while blockaction=unblock is fine, unblock=1 would serve the purpose well enough and would be more concise.

I think unblock=1 sounds good.

As for the log entry links (i.e. not Special:BlockList) — should we check for the current existence of the block, and so show the remove/unblock/change-block links as appropriate? i.e. not show unblock when there's nothing to unblock. Or maybe fall back to just a single 'manage blocks' link when no ID is available?

Change #1108534 merged by jenkins-bot:

[mediawiki/core@master] block: Update block action links in block log and contributions for multiblocks

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

dom_walden subscribed.

Acceptance criteria

When viewing Special:Log/block :

  • List each individual block as a separate log entry (this may already be the case)

I don't see any changes to the entries in Special:Log/block.

  • The "unblock" link should link to Special:Unblock/<username/IP>

I haven't seen such a link. I think it was changed to "remove block".

  • A "remove block" link should also be added, deep linking to Special:Unblock?id=123 (see T378144)

It actually links to: Special:Block/<target>?id=<id>&remove=1

  • The "change block" link should deep link to Special:Block?id=123 (see T378140)

Links to Special:Block/<target>&id=<id>

I also see "manage blocks" links which link to Special:Block/<target>

I think this is more-or-less consistent with what is written in T378150#10428126.

When viewing Special:Contribs/BlockedUser:

  • Message should instead say "There are currently N blocks on this user:"
  • List each active block in the same format as described above

I don't see any changes made to Special:Contributions, other than those noted in T378150#10436068.

Test environment: https://test.wikipedia.beta.wmflabs.org MediaWiki 1.44.0-alpha (7003ed2) 07:53, 24 January 2025.

Acceptance criteria

When viewing Special:Log/block :

  • List each individual block as a separate log entry (this may already be the case)

I don't see any changes to the entries in Special:Log/block.

Blocks already had separate log entries.

  • The "unblock" link should link to Special:Unblock/<username/IP>

I haven't seen such a link. I think it was changed to "remove block".

Yes, that change was proposed in T378150#10428126.

  • A "remove block" link should also be added, deep linking to Special:Unblock?id=123 (see T378144)

It actually links to: Special:Block/<target>?id=<id>&remove=1

There was some bikeshedding about that on Slack. For QA purposes, I don't think it matters as long as clicking on the link works as expected.

  • The "change block" link should deep link to Special:Block?id=123 (see T378140)

Links to Special:Block/<target>&id=<id>

I proposed that change at T378150#10435881. My theory is that clicking on such a link for a block that has expired or has been removed should still pre-fill the target.

I also see "manage blocks" links which link to Special:Block/<target>

I think this is more-or-less consistent with what is written in T378150#10428126.

I'd update the task description, but maybe @MusikAnimal owns it.

Since both ends are implemented now, the task description could just say what the links are actually meant to do, rather than what their URLs are.

When viewing Special:Contribs/BlockedUser:

  • Message should instead say "There are currently N blocks on this user:"
  • List each active block in the same format as described above

I don't see any changes made to Special:Contributions, other than those noted in T378150#10436068.

Fair point.

I updated the task description to say what I think I implemented, and I split out the Special:Contributions part to T384916.

Change #1165178 had a related patch set uploaded (by Reedy; author: Tim Starling):

[mediawiki/core@REL1_43] block: Update block action links in block log and contributions for multiblocks

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

Change #1165178 abandoned by Reedy:

[mediawiki/core@REL1_43] block: Update block action links in block log and contributions for multiblocks

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