Page MenuHomePhabricator

Watchlist Expiry: Add support to Protect, Delete, Undelete, Upload, Rollback, & Move APIs [small]
Closed, ResolvedPublic

Description

As a Watchlist Expiry user, I want support added to Protect, Delete, Undelete, Rollback, Upload and Move APIs, so that I can access the feature while using these functionalities.

Background: This is split out from T248407. ApiProtect, ApiDelete, ApiUndelete, ApiUpload, and ApiRollback ll use ApiBase::setWatch() to watch pages, so adding expiry support to all of them should involve the same changes. Protect and Delete both already have an expiry parameter, so for all APIs we can instead use a parameter called watchlistexpiry. It should have ApiBase::PARAM_TYPE of "expiry", to be introduced with T248508.

Resources:

Acceptance Criteria:

  • Add watchlist expiry support to Protect API
  • Add watchlist expiry support to Delete API
  • Add watchlist expiry support to Undelete API
  • Add watchlist expiry support to Upload API
  • Add watchlist expiry support to Rollback API
  • Add watchlist expiry support to Move API

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
MusikAnimal updated the task description. (Show Details)Mar 25 2020, 8:50 PM
Anomie moved this task from Unsorted to Needs Code on the MediaWiki-API board.Mar 25 2020, 9:10 PM
Anomie removed a project: Platform Engineering.
ifried renamed this task from Add watchlist expiry support to Protect, Delete, Rollback and Move APIs to Watchlist Expiry: Add support to Protect, Delete, Rollback and Move APIs.Mar 26 2020, 4:21 PM
ifried updated the task description. (Show Details)
ifried updated the task description. (Show Details)
ifried updated the task description. (Show Details)Mar 26 2020, 4:26 PM
ifried updated the task description. (Show Details)Mar 26 2020, 5:23 PM
ifried renamed this task from Watchlist Expiry: Add support to Protect, Delete, Rollback and Move APIs to Watchlist Expiry: Add support to Protect, Delete, & Rollback.Mar 26 2020, 5:29 PM
ifried updated the task description. (Show Details)
ARamirez_WMF renamed this task from Watchlist Expiry: Add support to Protect, Delete, & Rollback to Watchlist Expiry: Add support to Protect, Delete, & Rollback [small].Mar 26 2020, 5:30 PM
ifried updated the task description. (Show Details)Mar 26 2020, 5:32 PM
ifried moved this task from To Be Estimated/Discussed to Estimated on the Community-Tech board.
MusikAnimal renamed this task from Watchlist Expiry: Add support to Protect, Delete, & Rollback [small] to Watchlist Expiry: Add support to Protect, Delete, & Rollback APIs [small].Apr 28 2020, 1:20 AM
MusikAnimal renamed this task from Watchlist Expiry: Add support to Protect, Delete, & Rollback APIs [small] to Watchlist Expiry: Add support to Protect, Delete, Undelete, & Rollback APIs [small].May 22 2020, 7:45 PM
MusikAnimal updated the task description. (Show Details)
MusikAnimal renamed this task from Watchlist Expiry: Add support to Protect, Delete, Undelete, & Rollback APIs [small] to Watchlist Expiry: Add support to Protect, Delete, Undelete, Upload & Rollback APIs [small].May 22 2020, 8:01 PM
MusikAnimal updated the task description. (Show Details)

Change 602486 had a related patch set uploaded (by MusikAnimal; owner: MusikAnimal):
[mediawiki/core@master] [WIP][FEEDBACK REQUESTED] Add watchlist expiry support to some APIs

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

ifried renamed this task from Watchlist Expiry: Add support to Protect, Delete, Undelete, Upload & Rollback APIs [small] to Watchlist Expiry: Add support to Protect, Delete, Undelete, Upload, Rollback, & Move APIs [small].Jul 13 2020, 9:22 PM
ifried updated the task description. (Show Details)
ifried added a subscriber: ifried.Jul 13 2020, 9:26 PM

@MusikAnimal Hello! We have added the Move API into the acceptance criteria. We believe you already did the work; is that correct? If not, we'll create a separate ticket. Thanks!

MusikAnimal added a comment.EditedJul 13 2020, 9:57 PM

@MusikAnimal Hello! We have added the Move API into the acceptance criteria. We believe you already did the work; is that correct? If not, we'll create a separate ticket. Thanks!

Yes, https://gerrit.wikimedia.org/r/c/mediawiki/core/+/602486 adds expiry support to the Move API. That is, if you choose to watch when moving a page, it will allow you to specify an expiry. If you do specify to watch, but don't specify an expiry, it does not copy over the existing expiry (if one exists), which I think is what we want? That may be worthy of a separate task.

Change 602486 merged by jenkins-bot:
[mediawiki/core@master] Add watchlist expiry support to applicable APIs

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

Restricted Application edited projects, added Community-Tech; removed Community-Tech (Kanban-2020-21-Q1). · View Herald TranscriptJul 14 2020, 7:21 PM

When a page is already watched, if you make an API request (e.g. Edit, Protect) with the watchlist[1] parameter set to "preferences" or "nochange" and you set an expiry parameter, the expiry parameter of the watched page is updated.

For "preferences" this might make sense, because when a page is already watched "preferences" behaves the same as "watch". (Looking back, this appears to be how it has behaved for a long time, at least as far back as 2010ish.)

But I don't know if it makes sense for "nochange".

To be fair, if the user didn't want to change the expiry, I don't know why they would explicitly set an expiry when making the API request.

@MusikAnimal @ifried @Prtksxna I don't know if any of you have an opinion about this?

Ref:

  1. the watchlist parameter can have the values: nochange, preferences, unwatch or watch.
MusikAnimal added a comment.EditedJul 15 2020, 8:08 PM

Further to T248512#6309234, I'll explain the different options for the watchlist API parameter:

  • watch -- watch the page, obviously
  • unwatch -- unwatch the page
  • nochange -- leave the watch state unchanged. If it was already watched, it should remain watched. If it was unwatched, it should remain unwatched.
  • preferences -- defer to the preference for whatever action you're taking (e.g. the "Add pages and files I edit to my watchlist" preference is what is checked when editing pages). To put it simply, the value for the preference can either be watch or nochange (see definition above)

The question here is what behaviour should change, if any, in regards to a supplied watchlist expiry. Consider:

  1. I manually watch the page Foobar (either with an expiry or not)
  2. I use the API to edit the Foobar page and I set the watchlist parameter to be nochange. However I also supply a value for watchlistexpiry.
  3. We know we want to keep watching Foobar, since the user supplied "nochange" and they were already watching the page. However, should the expiry be changed, since they gave a watchlistexpiry value? Or should that also be left unchanged?

So in a nutshell: Should nochange mean no change to expiry, as well as the watched state? As Dom says: if the user didn't want to change the expiry, I don't know why they would explicitly set an expiry when making the API request. That is my sentiment.


Technical notes -- Most APIs end up calling WatchAction::doWatchOrUnwatch(). This only makes database changes if one of these two conditions are true:

  • The watch state is to be changed (I told the API to watch the page, and I wasn't watching it before)
  • An expiry is supplied. Doesn't matter if it matches the old expiry (if one existed)

The second part should maybe be fixed; that is, an UPDATE should only happen if there is a change to the expiry, so as to avoid unnecessary write transactions. That doesn't answer the question Dom brought up, but it's something worth mentioning.

At any rate, if our answer to Dom's question is NO (do NOT change expiries if nochange is used for the watchlist parameter), then it will require some work (see also ApiWatchlistTrait::getWatchlistValue(), the value of which gets passed to WatchAction::doWatchOrUnwatch). By rough estimate is a "medium".

JJMC89 added a subscriber: JJMC89.EditedJul 16 2020, 2:45 AM

The question here is what behaviour should change, if any, in regards to a supplied watchlist expiry. Consider:

  1. I manually watch the page Foobar (either with an expiry or not)
  2. I use the API to edit the Foobar page and I set the watchlist parameter to be nochange. However I also supply a value for watchlistexpiry.
  3. We know we want to keep watching Foobar, since the user supplied "nochange" and they were already watching the page. However, should the expiry be changed, since they gave a watchlistexpiry value? Or should that also be left unchanged?

So in a nutshell: Should nochange mean no change to expiry, as well as the watched state?

I would expect watchlistexpiry to return an appropriate warning and not change my watchlist (including expiry) for nochange, unwatch, and preferences:nochange. As in nochange means nothing about my watchlist should chage.
Another way to put it would be that watchlistexpiry is only acceptable/actionable for watch and preferences:watch.
This would be for all APIs, not just the ones in the task's title.

As Dom says: if the user didn't want to change the expiry, I don't know why they would explicitly set an expiry when making the API request. That is my sentiment.

Maybe someone sets watchlistexpiry=1 week for a session and only sets watclist=watch on the relevant requests when the relevant page actually needs to be watched. (Not that I would do this, but someone could.)

FYI, I raised T258187, but I don't think it is related to these changes.

There is also a similar (perhaps related) bug/behaviour:

  1. I am temporarily watching Page_A
  2. I use the API to move it to Page_B
  3. For the watchlist parameter I choose watch, nochange or preferences (where my preferences are set to watch)
  4. I do not set an explicit expiry value
  • Page_A is still temporarily watched with the original expiry value
  • Page_B is permanently watched

@MusikAnimal @ifried @Prtksxna What do we think? I think both pages should be temporarily watched with the original expiry value.

There is also a similar (perhaps related) bug/behaviour:

  1. I am temporarily watching Page_A
  2. I use the API to move it to Page_B
  3. For the watchlist parameter I choose watch, nochange or preferences (where my preferences are set to watch)
  4. I do not set an explicit expiry value
  • Page_A is still temporarily watched with the original expiry value
  • Page_B is permanently watched

What do we think? I think both pages should be temporarily watched with the original expiry value.

That will be handled in T257259 I believe (though we should document that in the ticket)

Change 613177 had a related patch set uploaded (by Samwilson; owner: MusikAnimal):
[mediawiki/core@REL1_35] Add watchlist expiry support to applicable APIs

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

Change 613177 merged by jenkins-bot:
[mediawiki/core@REL1_35] Add watchlist expiry support to applicable APIs

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

Behaviour of the different watchlist parameters:

  • watch:
    • Value of watchlistexpiry parameter:
      • Not set: Page watched, preserves the expiry value if that is already set (apart from T257259)
      • Temporary value: Watched temporarily, expiry value is updated
      • "Infinite" value: Watched permanently
  • unwatch: unwatches the page (apart from T258187)
  • preferences:
    • If page already watched: behaves the same as watch
    • If page not already watched: behaves the same as watch or unwatch depending on users' preferences
  • nochange: Does not change anything; preserves expiry date if applicable (apart from T257259)
    • Unless page is already watched and expiry value is set (as described in T248512#6309234)

I verified this on beta (MediaWiki 1.36.0-alpha (fbfa8f1) 12:47, 16 July 2020). I systematically tested all the combinations of watch parameters and page watch states (permanent, temporary, unwatched) for each affected API.

I also ran the script on testwiki (MediaWiki 1.35.0-wmf.41 (32c7656) 11:16, 16 July 2020) for regression purposes.

I did not test what happened to other users' watches when a page was moved/deleted etc.

...

Technical notes -- Most APIs end up calling WatchAction::doWatchOrUnwatch(). This only makes database changes if one of these two conditions are true:

  • The watch state is to be changed (I told the API to watch the page, and I wasn't watching it before)
  • An expiry is supplied. Doesn't matter if it matches the old expiry (if one existed)

The second part should maybe be fixed; that is, an UPDATE should only happen if there is a change to the expiry, so as to avoid unnecessary write transactions. That doesn't answer the question Dom brought up, but it's something worth mentioning.

Addressed with T258649

ifried closed this task as Resolved.Jul 31 2020, 6:38 PM
ifried moved this task from Product sign-off to Done on the Community-Tech (Kanban-2020-21-Q1) board.

Thank you for the above analysis, @dom_walden! I have conducted some basic tests on what happens to a temporarily watched item when the page is deleted. If the user select to watch the page via the checkbox (when deleting the page), it preserves the watch status with the half star (see screenshot example below), and the page can still be found in Edit:Watchlist. However, when I tried to restore the page, I saw the half star again but didn't see the clock icon in the watchlist. I'm also not sure what happens if another user deletes the page than the one watching the page. I have written some of the findings for deletion of pages in T259379, which we can approach separately. As for move, we are covering the support for move in T257259. Finally, as Leon wrote, some additional work will be covered in T258649. For these reasons, I'm marking this ticket as Done.

Change 624782 had a related patch set uploaded (by Ammarpad; owner: Ammarpad):
[mediawiki/core@master] Remove requirement for ApiWatchlistTrait to be in ApiBase.

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

Change 624782 merged by jenkins-bot:
[mediawiki/core@master] Remove requirement for ApiWatchlistTrait to be in ApiBase.

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

Change 626526 had a related patch set uploaded (by Dmaza; owner: Ammarpad):
[mediawiki/core@REL1_35] Remove requirement for ApiWatchlistTrait to be in ApiBase.

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

Change 626526 merged by jenkins-bot:
[mediawiki/core@REL1_35] Remove requirement for ApiWatchlistTrait to be in ApiBase.

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