Page MenuHomePhabricator

Add user right to delete single revision redirects, regardless of target, during page moves
Closed, ResolvedPublic

Description

Use case:
If a redirect is created to a section of a page, or a related topic, and then a full page is created for the topic, it cannot be moved to the redirect without moving the redirect out of the way

  • A -> B or A -> B#C
  • C, a new page on the subject, cannot be moved to A and automatically delete the redirect

Implementation:
A new right is added, delete-redirect, to allow users to overwrite all single-revision redirects. The deletion is logged separately as delete/delete_redir2

Event Timeline

Restricted Application added a project: User-DannyS712. · View Herald TranscriptNov 26 2019, 9:01 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Pppery renamed this task from Allow automatic deletion during page move of all single revision redirects, regardless of target to Add user right to automatically delete single revision redirects, regardless of target, during page moves.Nov 27 2019, 1:03 AM
Pppery awarded a token.
DannyS712 moved this task from Unsorted to Next on the User-DannyS712 board.Nov 27 2019, 1:24 AM

We've discussed this internally in the CPT group, and we don't see a big objection, and it seems like a nice benefit, especially for people who do a lot of page moving. We'd like to see you get some community discussion on this to shake out any possible bad ramifications before there's a patch ready to review.

This comment was removed by DannyS712.
DannyS712 added a comment.EditedDec 4 2019, 3:15 AM

We've discussed this internally in the CPT group, and we don't see a big objection, and it seems like a nice benefit, especially for people who do a lot of page moving. We'd like to see you get some community discussion on this to shake out any possible bad ramifications before there's a patch ready to review.

My patch wasn't going to assign this right to anyone by default, since the only non-controversial assignment would be sysop, which doesn't need this since it has delete rights.
I'm having the deletion logged differently (delete/delete_redir2 instead of delete/delete_redir), given that the deletion is inherently different.
Following the current behaviour for delete and move, the actual move is logged as a normal move/move

DannyS712 updated the task description. (Show Details)Dec 4 2019, 3:15 AM
This comment was removed by DannyS712.
DannyS712 updated the task description. (Show Details)Dec 14 2019, 12:15 AM

Question: this will introduce a variation of moving over an existing page that allows users without delete rights to overwrite any single revision redirect. Given that this deletion should not be possible outside the context of moving a page, it seems like the only way to do this via the api would be to add the functionality requested in T44886: No option to move over an existing non-redirect page in the API (adding on option to the move api to move over an existing page). That task was declined by @Anomie, saying

I'm going to decline this, just use action=delete followed by action=move.

Given the new feature introduced here, @Anomie would you be willing to reconsider declining that task?

Given the new feature introduced here, @Anomie would you be willing to reconsider declining that task?

No.

action=move already supports the existing case of moving over a single-revision redirect that points to the source of the move, i.e. moving A over B when B exists as a single-revision redirect to A. This new feature is just removing the "that points to the source of the move" condition, and can be added as such without going all the way to doing what was requested in T44886.

Given the new feature introduced here, @Anomie would you be willing to reconsider declining that task?

No.

action=move already supports the existing case of moving over a single-revision redirect that points to the source of the move, i.e. moving A over B when B exists as a single-revision redirect to A. This new feature is just removing the "that points to the source of the move" condition, and can be added as such without going all the way to doing what was requested in T44886.

If you look at the patch I provided, to ensure that the user intended to delete a different redirect and enforce the user right restrictions I used the same process as the current moving over an existing article (i.e. delete before the move, rather than as part of it) and so I don't believe that the current action=move supports this new behaviour.

I see no patch provided here.

You may need to review your implementation.

I see no patch provided here.

You may need to review your implementation.

Sorry; I've been working on this at https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/554365/, but haven't tagged the task until it was ready for review

DannyS712 renamed this task from Add user right to automatically delete single revision redirects, regardless of target, during page moves to Add user right to delete single revision redirects, regardless of target, during page moves.Dec 19 2019, 10:36 AM

Change 559622 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Add delete-redirect for deleting single-rev redirects during moves

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

@eprodromou can Platform Engineering please review the provided patch?

WDoranWMF moved this task from External Code Review In Progress to External Code Review Needed on the Core Platform Team Workboards (Clinic Duty Team) board.

I looked at this, but the comments I left on PS5 do not seem to have been addressed in PS6.

WDoranWMF moved this task from External Code Review In Progress to External Code Review Needed on the Core Platform Team Workboards (Clinic Duty Team) board.

I looked at this, but the comments I left on PS5 do not seem to have been addressed in PS6.

PS6 was just a rebase.
The comment on PS5 was regarding moving "delete by overwriting" logic out of the movepage special page and into the movepage class itself.

@daniel commented:

Since "automatically move before deletion" is a notion owned by the UI layer, it seems correct to handle the permission there as well. One could of course argue that this logic belongs into the MovePage class, but that should be a separate discussion and a different patch.

@Anomie commented:

Is it a notion owned by the UI layer? Remember that if we put it there then it has to be implemented multiple times: in SpecialMovepage, in ApiMove, and potentially at some point in the future in a MW REST API Handler as well. If we put it here, then we just have to have each of those UI layers call it in the right way.

but that should be a separate discussion and a different patch.

Why do it wrong only to go back and fix it soon after?

@DannyS712 commented:

If I submit a patch to move delete and move logic into the MovePage class (haven't looked at doing this yet, just asking, not committing) will it be reviewed?

I don't believe that this should be included as part of this task, but can look into doing it separately if CPT can commit to reviewing it?

daniel triaged this task as Medium priority.Apr 8 2020, 11:28 AM
Jony added a subscriber: Jony.Apr 9 2020, 6:42 AM

Change 559622 merged by jenkins-bot:
[mediawiki/core@master] Add delete-redirect for deleting single-rev redirects during moves

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

Pppery removed a subscriber: Pppery.Sep 4 2020, 4:07 PM

Are there any default (or wmf-specific) groups this will be added to, or will WMF communities need to request this be added to groups as needed?

Are there any default (or wmf-specific) groups this will be added to, or will WMF communities need to request this be added to groups as needed?

No groups have it yet. I was envisioning this as a right for enwiki's page mover group, and was planning to start a discussion for that at some point once the train reaches enwiki

We should consider granting to autoconfirmed by default, though I'm not sure it's the right course. Since it only allows deletion of single revision redirects, I doubt that misuse will lead to serious disruption, and with rate limits any problems should be caught before they get out of hand. That said, since revisions are deleted, any problems would have to be resolved by admins because non-sysops wouldn't be able to figure out where the redirect pointed before the page was deleted. For smaller projects with few admins, that might be a bottleneck to avoid.

@DannyS712: I'm not familiar with the api specifics, but could we log the original redirect target as part of the history or deletion log entry? If the deleted target was part of the publicly visible log history it could mitigate some of those problems. Something like: "User:Example moved the page Foo to Bar over a redirect targeting Biz." That way if editors need to clean something up they know to restore a redirect to Biz without requiring rights to view deleted history

DannyS712 added a comment.EditedSep 7 2020, 10:51 PM

We should consider granting to autoconfirmed by default, though I'm not sure it's the right course. Since it only allows deletion of single revision redirects, I doubt that misuse will lead to serious disruption, and with rate limits any problems should be caught before they get out of hand. That said, since revisions are deleted, any problems would have to be resolved by admins because non-sysops wouldn't be able to figure out where the redirect pointed before the page was deleted. For smaller projects with few admins, that might be a bottleneck to avoid.

@DannyS712: I'm not familiar with the api specifics, but could we log the original redirect target as part of the history or deletion log entry? If the deleted target was part of the publicly visible log history it could mitigate some of those problems. Something like: "User:Example moved the page Foo to Bar over a redirect targeting Biz." That way if editors need to clean something up they know to restore a redirect to Biz without requiring rights to view deleted history

That is not currently logged, and I'm not sure how to do so.
If I understand the history correctly, the automatic deletion of single-revision redirects that point to the title being moved was added to combat page move vandalism. That shouldn't be a concern here, which is why I didn't assign it to autoconfirmed by default

Are there any default (or wmf-specific) groups this will be added to, or will WMF communities need to request this be added to groups as needed?

No groups have it yet. I was envisioning this as a right for enwiki's page mover group, and was planning to start a discussion for that at some point once the train reaches enwiki

Notice posted at Wikipedia talk:Page mover

4nn1l2 added a subscriber: 4nn1l2.Oct 6 2020, 11:07 AM
DannyS712 closed this task as Resolved.Wed, Dec 30, 6:08 AM