Page MenuHomePhabricator

Introduce a new DeletePage service
Open, Needs TriagePublic5 Estimated Story Points

Description

See T288242 for an overview.

Event Timeline

Change 713560 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] Move deletion code from WikiPage to a new DeletePage command

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

Change 713654 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] Clean up DeletePage

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

Change 713876 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] More cleanup for DeletePage

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

The things above are up for review. There are still many things left to do, but those are partly blocked on the open patches for this task and for T288282.

Change 714048 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] Move more logic from WikiPage to DeletePage

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

Change 713560 merged by jenkins-bot:

[mediawiki/core@master] Move deletion code from WikiPage to a new DeletePage command

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

Change 714121 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] Make DeletePage a real service

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

Change 714167 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] Improve DeletePage tests

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

Change 714352 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] Deprecate and replace legacy hooks in DeletePage

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

Change 714383 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] Deprecate WikiPage methods replaced by DeletePage

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

Change 713654 merged by jenkins-bot:

[mediawiki/core@master] Clean up DeletePage

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

Change 713876 merged by jenkins-bot:

[mediawiki/core@master] More cleanup for DeletePage

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

Change 714048 merged by jenkins-bot:

[mediawiki/core@master] Move more logic from WikiPage to DeletePage

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

Daimona set the point value for this task to 5.Aug 31 2021, 11:23 AM

Change 714121 merged by jenkins-bot:

[mediawiki/core@master] Make DeletePage a real service

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

Change 714167 merged by jenkins-bot:

[mediawiki/core@master] Improve DeletePage tests

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

Change 720850 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@REL1_37] Deprecate and replace legacy hooks in DeletePage

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

Change 714352 merged by jenkins-bot:

[mediawiki/core@master] Deprecate and replace legacy hooks in DeletePage

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

Change 714383 merged by jenkins-bot:

[mediawiki/core@master] Deprecate WikiPage methods replaced by DeletePage

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

Change 721639 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] Use DeletePage in ApiDelete and DeletePageJob

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

Change 720850 merged by jenkins-bot:

[mediawiki/core@REL1_37] Deprecate and replace legacy hooks in DeletePage

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

Change 721667 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@REL1_37] Deprecate WikiPage methods replaced by DeletePage

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

Change 721667 merged by jenkins-bot:

[mediawiki/core@REL1_37] Deprecate WikiPage methods replaced by DeletePage

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

Change 724021 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] Check change tags permissions in DeletePage

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

Change 725917 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@REL1_37] Check change tags permissions in DeletePage

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

Change 724021 merged by jenkins-bot:

[mediawiki/core@master] Check change tags permissions in DeletePage

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

Change 725917 merged by jenkins-bot:

[mediawiki/core@REL1_37] Check change tags permissions in DeletePage

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

Change 721639 merged by jenkins-bot:

[mediawiki/core@master] Use DeletePage in ApiDelete and DeletePageJob

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

Daimona added subscribers: imaigwilo, dom_walden.

@dom_walden @imaigwilo We'd like to have a preliminary QA phase now. No new feature was introduced so far; we only rewrote the deletion code (backend). We switched ApiDelete to use the new code explicitly, so we'd like to ensure that there are no regressions with deletions performed via the API.

The UI is still using the old code (which is actually proxying calls to the new code internally), so there's no need to test it now. Once we get your green light that API deletions are still fully functional, we will migrate the UI as well and ask you to repeat the same tests for the UI (in this same task).

Here are some suggestions of things that you might want to test, amongst others:

  • Lacking the 'delete' permissions
  • Trying to delete a page with more than $wgDeleteRevisionsLimit revisions (with/without having the 'bigdelete' right)
  • Applying change tags to the deletion (with/without the 'applychangetags' permission)
  • Deleting pages in the File: namespace
  • Using the 'oldimage' API parameter
  • Trying both scheduled and immediate deletions (you should get a message that the deletion was scheduled if the page has more revisions than $wgDeleteRevisionsBatchSize + 1

Note that the logic of the API module itself is unchanged, so things like parameter validation should not be affected by our changes.

Thanks!

  • Lacking the 'delete' permissions
  • Applying change tags to the deletion (with/without the 'applychangetags' permission)
  • Deleting pages in the File: namespace

These three I believe I tested thoroughly, but we could use another QA pass.

Note that some of this requires configuration changes to fully test and hence won't be testable on Beta. You'll want your own local MW install so you can for instance make $wgDeleteRevisionsLimit something smaller, and be able to change the default user rights. In particular be aware that registered users have the applychangetags right by default, which you can disable with $wgGroupPermissions['user']['applychangetags'] = false;. Config changes have an immediate effect so you should be able test using Special:ApiSandbox, adjusting user rights as desired between requests. I'm not sure if any of this was obvious to either of you already so thought I'd share what I did!

Change 730197 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] Use DeletePage in FileDeleteForm and fix output of ApiDelete

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

Here are some suggestions of things that you might want to test, amongst others:

  • Lacking the 'delete' permissions
  • Trying to delete a page with more than $wgDeleteRevisionsLimit revisions (with/without having the 'bigdelete' right)
  • Applying change tags to the deletion (with/without the 'applychangetags' permission)
  • Deleting pages in the File: namespace
  • Using the 'oldimage' API parameter
  • Trying both scheduled and immediate deletions (you should get a message that the deletion was scheduled if the page has more revisions than $wgDeleteRevisionsBatchSize + 1

Thanks for the guidance. I have done some testing on beta (https://en.wikipedia.beta.wmflabs.org and https://commons.wikimedia.beta.wmflabs.org) I think covering all of these points and a few others.

I imagine there are more things to test, but I cannot think of anything at the moment and I am running out of focus. As this project goes on we will get a better idea of what and how to test this.

@Daimona Would you like this moved into Product sign-off or wait until the above patch (T288758#7419627) is merged?

Change 730775 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] Use DeletePage in DeleteAction

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

Thanks for the guidance. I have done some testing on beta (https://en.wikipedia.beta.wmflabs.org and https://commons.wikimedia.beta.wmflabs.org) I think covering all of these points and a few others.

I imagine there are more things to test, but I cannot think of anything at the moment and I am running out of focus. As this project goes on we will get a better idea of what and how to test this.

@Daimona Would you like this moved into Product sign-off or wait until the above patch (T288758#7419627) is merged?

Thanks! That patch is not strictly necessary for our work, but I think it might be helpful to have it merged, otherwise you may get confused while testing file deletion.

I'm moving this back to review since we also need to use DeletePage from the UI code, that is r730775. Once that patch is merged, I'll move it back to QA for testing deletion via action=delete.