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

@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!

In T248512#6302724, @ifried wrote:

@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

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.

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".

In T248512#6309989, @MusikAnimal wrote:

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.

In T248512#6312688, @dom_walden wrote:

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 (rMW32c7656d407b) 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.

In T248512#6309989, @MusikAnimal wrote:

...

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

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