Page MenuHomePhabricator

Add Hook for PageTriage after specific actions taken, to allow access from gadgets [high priority]
Closed, ResolvedPublic3 Estimate Story Points

Description

As followup for T207237: Page Curation Tools to add userspace CSD Log/PROD Log functionality, we could add a hook that fires on PageTriage's actions (deletion/mark as reviewed/unreview/etc) that allows external tools, gadgets and extensions to respond to PageTriage actions in a consolidated way.

As an example, Twinkle can utilize this hook to add the information to their CSD logs. Another example is potentially creating a tool to batch logs, respond to certain types of actions and alert users. These are tools that are not prioritized and are out of scope for the current work, but the adding of this hook will enable these powerful utilizations for extending PageTriage work.

Details

Related Gerrit Patches:
mediawiki/extensions/PageTriage : masterFire delete hook when any delete action is taken
mediawiki/extensions/PageTriage : masterPrevent multiple bindings of render()
mediawiki/extensions/PageTriage : masterAdd JS hooks so that other scripts/gadgets can integrate with PageTriage
mediawiki/extensions/PageTriage : masterActionQueue: Allow for invoking multiple actions
mediawiki/extensions/PageTriage : masterAdd an asynchronous ActionQueue for external tools

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ifried set the point value for this task to 3.Aug 13 2019, 10:37 PM
Mooeypoo renamed this task from Placeholder: Add Hook for T207237 [more info added later] to Placeholder: Add Hook for PageTriage after specific actions taken, to allow access from gadgets.Aug 14 2019, 10:01 PM
Mooeypoo renamed this task from Placeholder: Add Hook for PageTriage after specific actions taken, to allow access from gadgets to Add Hook for PageTriage after specific actions taken, to allow access from gadgets.
Mooeypoo updated the task description. (Show Details)
kostajh moved this task from Inbox to Triaged but Future on the Growth-Team board.Aug 16 2019, 3:02 PM
kostajh added a subscriber: kostajh.

Seems like a good idea, but not something that Growth-Team will be able to work on anytime soon.

Change 530977 had a related patch set uploaded (by MusikAnimal; owner: MusikAnimal):
[mediawiki/extensions/PageTriage@master] Add JS hooks so that other scripts/gadgets can integrate with PageTriage

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

MusikAnimal removed MusikAnimal as the assignee of this task.Sep 3 2019, 9:23 PM
MusikAnimal added a subscriber: MusikAnimal.

One of the biggest issues here is that we call the hook just before we refresh the page, and that the use cases that this hook is for are working asynchronously, which means refreshing immediately after call is not going to help anything

After conversation with the team, here's the new plan:

  • Create an mw.hook that fires when PageTriage JavaScript finishes loading. That hook should provide an entrypoint for an array that expects promises. We can provide some expected parameters into the expected promises, like the action taken, data, etc.
  • Store the array of promises.
  • Before we refresh the page on the actions that were requested, we call all promises (can use a $.when on the stored array) and only after they're all resolved or rejected, we refresh the page.

This will allow other gadgets to hook into PageTrage while still allowing for async responses before the page is refreshed.

MusikAnimal removed MusikAnimal as the assignee of this task.Sep 5 2019, 8:38 PM

Change 534942 had a related patch set uploaded (by Mooeypoo; owner: Mooeypoo):
[mediawiki/extensions/PageTriage@master] WIP: Add an asynchronous ActionQueue for external tools

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

Mooeypoo claimed this task.Sep 8 2019, 7:26 AM

Change 534942 merged by jenkins-bot:
[mediawiki/extensions/PageTriage@master] Add an asynchronous ActionQueue for external tools

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

The new ActionQueue system was merged (many thanks Moriel!), so I will now make use of that in https://gerrit.wikimedia.org/r/530977

Change 535935 had a related patch set uploaded (by Mooeypoo; owner: Mooeypoo):
[mediawiki/extensions/PageTriage@master] ActionQueue: Allow for invoking multiple actions

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

Change 535935 merged by jenkins-bot:
[mediawiki/extensions/PageTriage@master] ActionQueue: Allow for invoking multiple actions

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

Ready for QA! To test, I put this in my Common.js (and on the beta cluster you could just use Special:MyPage/common.js):

$( document ).ready( function () {
	mw.hook( 'ext.pageTriage.toolbar.ready' ).add( function ( queue ) {
		queue.add( 'articleinfo', function ( data ) {
			console.log( 'pagetriage.articleinfo' );
			console.log( data );
		} );
		queue.add( 'delete', function ( data ) {
			console.log( 'pagetriage.delete' );
			console.log( data );
		} );
		queue.add( 'mark', function ( data ) {
			console.log( 'pagetriage.mark' );
			console.log( data );
		} );
		queue.add( 'tags', function ( data ) {
			console.log( 'pagetriage.tags' );
			console.log( data );
		} )
	} );
} );

Note you'll want to use the "preserve console log" feature of your browser's developer tools, because the "delete" and "tags" actions will refresh the page. Or you could just put a debugger in the above code.

Err, Jenkins is currently not cooperating. Placing this "in development" until it actually gets merged!

Change 530977 merged by jenkins-bot:
[mediawiki/extensions/PageTriage@master] Add JS hooks so that other scripts/gadgets can integrate with PageTriage

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

@MusikAnimal @Mooeypoo There is an odd behaviour I cannot understand. After opening the "Page info" flyout the articleinfo hook fires, which is fine. But afterwards, each time I mark the page reviewed/unreviewed (without refreshing), the articleinfo hook also fires. First time once, second time twice, third time 4 times, then 8, etc.

I don't think the doc is correct about the mark action adding a note. I don't think we add notes to review/unreview actions anymore.

Actions which fire a hook:

  • Opening "Page info" flyout
  • Marking a page reviewed or unreviewed
  • Adding a tag to a page
  • Marking a page for speedy deletion
  • Marking a page for proposed deletion

Actions which don't fire a hook:

  • Marking article as "Article for deletion" and (possibly, can't test on beta atm) User page as "Miscellany for deletion"
  • Sending WikiLove
  • "Add a message for the creator"

Is this ok? I would have thought we should fire the delete hook for afd and mfd.

@MusikAnimal @Mooeypoo There is an odd behaviour I cannot understand. After opening the "Page info" flyout the articleinfo hook fires, which is fine. But afterwards, each time I mark the page reviewed/unreviewed (without refreshing), the articleinfo hook also fires. First time once, second time twice, third time 4 times, then 8, etc.

Oh noes, that sound like a problem with Promise chaining. Ha.

I think (off the top of my head) that we might be attaching .then each time to the same promise, and then running the same promise. This will just keep adding .then to the promises -- which means it keeps running the previous ones again and again.

I didn't think about this, and also, I think we might be missing what happens when a promise reruns without refresh... we need to discuss and see how we follow up.

Mooeypoo added a subscriber: Catrope.EditedSep 23 2019, 11:08 PM

Thanks to the amazing debug (and git blame) skills of @Catrope we found the problem.

In articleinfo.js this happens:

		this.model.unbind( 'change:patrol_status', function () {
			that.render();
		} );
		this.model.bind( 'change:patrol_status', function () {
			that.render();
		} );

The problem is that this isn't a way to unbind the function (it would unbind if we have a reference to it rather than a function again) which means we are adding that function (this.render()) over and over again, making it be invoked over and over again.

This always happened, but the action queue exposed it.

The solution is to have a class var that remembers the state; I'm working on a quick patch.

Change 538719 had a related patch set uploaded (by Mooeypoo; owner: Mooeypoo):
[mediawiki/extensions/PageTriage@master] Prevent multiple bindings of render()

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

Bringing back to review for this patch.

Change 538719 merged by jenkins-bot:
[mediawiki/extensions/PageTriage@master] Prevent multiple bindings of render()

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

This was merged, and will be available on beta in a few minutes.

dom_walden added a comment.EditedOct 2 2019, 5:55 PM

Which hooks get fired for which actions:

Actionarticleinfomarktagsdelete
Opening "Page info" flyoutx
Reviewed or unreviewedx[1]x
Adding Tagxx
Speedy deletionx
Proposed deletionx
Article for deletion
Miscellany for deletion
WikiLove
"Add a message for the creator"
  1. Only after first opening the "Page info" flyout

For reference:

  • Adding a tag marks a page as reviewed (unless overridden on wiki)
  • Doing Speedy or Proposed deletion unreviews a page (I think by default)
  • Doing AfD or MfD reviews a page
ifried renamed this task from Add Hook for PageTriage after specific actions taken, to allow access from gadgets to Add Hook for PageTriage after specific actions taken, to allow access from gadgets [high priority].Oct 16 2019, 9:34 PM
HMonroy claimed this task.Oct 21 2019, 5:30 PM
HMonroy moved this task from Ready to In Development on the Community-Tech (Kanban-Q2-2019-20) board.

Change 545666 had a related patch set uploaded (by HMonroy; owner: HMonroy):
[mediawiki/extensions/PageTriage@master] Fire delete hook when any delete action is taken

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

Per our conversation during stand up, we need to trigger the delete hook when any of the delete actions is taken. This change was submitted in patch https://gerrit.wikimedia.org/r/545666

@HMonroy Just checking, because I didn't explain it very clearly in T230455#5541869, have you also investigated the weird behaviour where the articleinfo hook gets fired when you review/unreview, but only after first opening the "Page info" flyout?

@dom_walden Thank you for checking, the articleinfo hook has been removed. It should not be triggered for any action.

aezell added a subscriber: aezell.Oct 28 2019, 8:49 PM

Thanks y'all for working to get this one right.

The delete hook should now be fired for any type of deletion. The data it gives you is just whatever we have stored for the article model, so for AfD for instance all the info is in data.tags.articlesfordeletion (where data is what the handler of the hook is given). With speedy it's for example data.tags.g7 (and also data.tags.g11, etc.), and similar for PROD. This is sufficient for clients to determine what type of deletion it is, I think.

Change 545666 merged by jenkins-bot:
[mediawiki/extensions/PageTriage@master] Fire delete hook when any delete action is taken

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

@HMonroy When there is no "Articles for deletion" log for today's date (e.g. Wikipedia:Articles for deletion/Log/2019 October 30) and I attempt the AfD action, I see the Log page for today has not been created yet! popup. If I click "OK", the AfD action continues. It adds the {{afd}} template to the article and posts on the creator's user_talk page (although it does not create the deletion page Wikipedia:Articles for deletion/$article_name).

This contrasts to before when clicking OK canceled the action.

I am comparing https://en.wikipedia.beta.wmflabs.org with https://test.wikipedia.org. I think the only difference is that the latter does not have this change.

I don't know how likely something like this will occur in production. I guess the Articles for deletion log gets created automatically there. On the other hand, there may be other circumstances where the patroller will be unable to add to the deletion log. For instance, they might be blocked from the Wikipedia namespace (although perhaps also unlikely for someone with patrol rights).

HMonroy added a subscriber: MaxSem.Oct 30 2019, 4:41 PM

@dom_walden the AfD action behavior is related to https://phabricator.wikimedia.org/T231357. @MaxSem is Dom's concern likely in production?

Not particularly likely, but there could be other issues, like log page failing to load due to bad connection, etc.

@dom_walden we reviewed your concern during standup and since it's not very likely to happen (it has not been reported by any user) @ifried decided that it is safe to leave it as is for now. This possible case has been there since the beginning. Thank you!

@HMonroy When there is no "Articles for deletion" log for today's date (e.g. Wikipedia:Articles for deletion/Log/2019 October 30) and I attempt the AfD action, I see the Log page for today has not been created yet! popup. If I click "OK", the AfD action continues. It adds the {{afd}} template to the article and posts on the creator's user_talk page (although it does not create the deletion page Wikipedia:Articles for deletion/$article_name).
This contrasts to before when clicking OK canceled the action.
I am comparing https://en.wikipedia.beta.wmflabs.org with https://test.wikipedia.org. I think the only difference is that the latter does not have this change.
I don't know how likely something like this will occur in production. I guess the Articles for deletion log gets created automatically there. On the other hand, there may be other circumstances where the patroller will be unable to add to the deletion log. For instance, they might be blocked from the Wikipedia namespace (although perhaps also unlikely for someone with patrol rights).

dom_walden added a comment.EditedNov 13 2019, 7:56 PM

@dom_walden we reviewed your concern during standup and since it's not very likely to happen (it has not been reported by any user) @ifried decided that it is safe to leave it as is for now. This possible case has been there since the beginning. Thank you!

OK, thanks.

@HMonroy I am seeing something else now I'm afraid, which I didn't see before. Occasionally, afd tags get added twice to a page. See, for example, https://test.wikipedia.org/wiki/Dsjfklsdjl (for the first and third nomination). I don't know if you can reproduce.

In these cases, I see two calls made to api.php?action=pagetriagetagging. Less often, I also see two calls to api.php?action=edit&title=User_talk:$creator&....

I tend to also see errors popping up briefly, such as "Failed to notify author" or "Failed to apply tag".

Another issue. In the AfD form if I don't enter anything in the "Add details" section and press "Mark for deletion", I get a validation error ("Tag afd is missing required parameter"). However, if I click OK the AfD action continues.

@dom_walden we reviewed your concern during standup and since it's not very likely to happen (it has not been reported by any user) @ifried decided that it is safe to leave it as is for now. This possible case has been there since the beginning. Thank you!

OK, thanks.
@HMonroy I am seeing something else now I'm afraid, which I didn't see before. Occasionally, afd tags get added twice to a page. See, for example, https://test.wikipedia.org/wiki/Dsjfklsdjl (for the first and third nomination). I don't know if you can reproduce.
In these cases, I see two calls made to api.php?action=pagetriagetagging. Less often, I also see two calls to api.php?action=edit&title=User_talk:$creator&....
I tend to also see errors popping up briefly, such as "Failed to notify author" or "Failed to apply tag".
Another issue. In the AfD form if I don't enter anything in the "Add details" section and press "Mark for deletion", I get a validation error ("Tag afd is missing required parameter"). However, if I click OK the AfD action continues.

@dom_walden I was able to replicate:

  1. The afd tag being added twice to a page. It actually gets added as many times as the page is "mark for deletion" by the same user. The other nominations (2nd and 3rd) would also get added multiple times when adding the afd tag with different logged in users. Here is a screenshot of what my page looks like in my local env:
  2. "Failed to apply tag" errors
  3. "Tag afd is missing required parameter" error when the "Add details" section is empty and I click on the button.

But I was not able to replicate:

  1. "Failed to notify author" error

I was able to replicate these issues in master as well. I briefly reviewed them with @MaxSem. Max said that these use cases have been there since we first started working on this project and that they are not related to our recent changes. Can you corroborate this @MaxSem ?

That sounds like API responses are getting cached?

Which hooks get fired for which actions (now, on testwiki):

Actionarticleinfomarktagsdelete
Opening "Page info" flyout
Reviewed or unreviewedx
Adding Tagxx
Speedy deletionx
Proposed deletionx
Article for deletionxx
Miscellany for deletionxx
WikiLove
"Add a message for the creator"

I have raised bugs T239712 and T239714 for the issues discussed in T230455#5661483 and T230455#5673920.

I have not raised a bug for T230455#5619120.

ifried closed this task as Resolved.Dec 16 2019, 11:08 PM
ifried moved this task from Product sign-off to Done on the Community-Tech (Kanban-Q2-2019-20) board.

This is now on production, so I'm marking this work as Done.

Where can I find information regarding who in the community is working on this further. This is one of the most important things for me regarding use of the deletion tools (I will not use the page curation tools without CSD and PROD log functionality added). I was disappointed to see a bare minimum done with the expectation that volunteers would have to finish it, but if so let me know who I should be talking to.