Page MenuHomePhabricator

Fix ApiEditPage to return watchlist expiry if present when watchlist param is `nochange`
Closed, ResolvedPublic5 Estimated Story PointsOct 7 2020

Description

As a Watchlist Expiry user, I want watchlistexpiry to return the value in the database (if no changes are being made to the watchlist), so as to ensure that watchlist data is being processed properly.

How to reproduce

  1. Watch any page temporarily
  2. Use the API as below to run an edit without making changes to the watchlist (watchlist=nochange)

The data returned from the api should include the previously selected expiry and instead it returns as infinity

https://test.wikipedia.org/wiki/Special:ApiSandbox#action=edit&format=json&title=Test123454&text=test&watchlist=nochange&token=&formatversion=2

Acceptance criteria:
watchlistexpiry should return the value in the database if no changes are being made to the watchlist

Visual Example:

Event Timeline

dmaza created this task.Aug 21 2020, 9:46 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 21 2020, 9:46 PM
ifried updated the task description. (Show Details)Aug 21 2020, 10:13 PM
Restricted Application added a project: Platform Engineering. · View Herald TranscriptAug 23 2020, 1:47 AM
Ammarpad claimed this task.Aug 24 2020, 7:23 AM
ARamirez_WMF set the point value for this task to 5.Aug 24 2020, 5:29 PM
ifried moved this task from Estimated to Kanban-2020-21-Q1 on the Community-Tech board.
dmaza added a comment.Aug 24 2020, 7:53 PM

I'm also noticing that the API result for this action includes watched: as part of the result only if you are watching the page. I'm unsure if this is the status quo everywhere or if it needs to be addressed.

In my opinion, API results should be consistent except for things like watchlistexpiry that sits behind a feature flag.

MusikAnimal added a subscriber: MusikAnimal.EditedAug 25 2020, 2:02 AM

I'm also noticing that the API result for this action includes watched: as part of the result only if you are watching the page. I'm unsure if this is the status quo everywhere or if it needs to be addressed.

In my opinion, API results should be consistent except for things like watchlistexpiry that sits behind a feature flag.

I guess I did that with 6a898fae. The idea was to make it consistent with action=watch, but I should have done the same for the other APIs too. I originally tried to refactor all the API responses so that it's always consistent, but that turned into a rabbit hole. So for now, we could just remove the $r['watched'] = $status->isOK(); bit from ApiEditPage if we want to. Longer-term I think it'd nice if ApiWatchlistTrait handled this stuff for us, returning watched and watchlistexpiry accordingly whenever a change to the watch status is requested.

eprodromou added a subscriber: eprodromou.

Seems like this is under control. Let us know if there's anything we can do to help out.

Change 622708 had a related patch set uploaded (by Ammarpad; owner: Ammarpad):
[mediawiki/core@master] API: Show existing watchlist expiry if status is not being changed.

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

ARamirez_WMF set Due Date to Oct 7 2020, 4:00 AM.Sep 25 2020, 9:30 PM
ARamirez_WMF changed the subtype of this task from "Task" to "Deadline".Sep 25 2020, 9:32 PM

Change 622708 merged by jenkins-bot:
[mediawiki/core@master] ApiEditPage: Show existing watchlist expiry if status is not being changed.

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

Change 630798 had a related patch set uploaded (by MusikAnimal; owner: Ammarpad):
[mediawiki/core@REL1_35] ApiEditPage: Show existing watchlist expiry if status is not being changed.

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

Change 631024 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Revert "ApiEditPage: Show existing watchlist expiry if status is not being changed."

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

Change 631024 merged by jenkins-bot:
[mediawiki/core@master] Revert "ApiEditPage: Show existing watchlist expiry if status is not being changed."

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

Could we solicit some help to get LiquidThreads fixed?

The page needs rewrite. But any significant rewrite even for supported extension would really take time. But LiquidThreads is essentially dead and ought to be undeployed T187487.

I initially intended to use the service locator in the trait, but there's was some objection. The services is already being used in the ApiEditPage to fetch RevisionLookup and I am not sure why we should not use that to fetch a single WatchedItemStore. At any time when LiquidThread is rewritten or the extension gets undeployed, the ApiEditPage can inject both the WatchedItemStore, RevisionLookup and other dependencies that it's currently getting from service locator, such as ContentHandlerFactory,

Change 631222 had a related patch set uploaded (by Ammarpad; owner: Ammarpad):
[mediawiki/core@master] Revert "Revert "ApiEditPage: Show existing watchlist expiry if status is not being changed.""

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

MusikAnimal added a comment.EditedSep 30 2020, 5:54 PM

The page needs rewrite. But any significant rewrite even for supported extension would really take time. But LiquidThreads is essentially dead and ought to be undeployed T187487.

I initially intended to use the service locator in the trait, but there's was some objection. The services is already being used in the ApiEditPage to fetch RevisionLookup and I am not sure why we should not use that to fetch a single WatchedItemStore. At any time when LiquidThread is rewritten or the extension gets undeployed, the ApiEditPage can inject both the WatchedItemStore, RevisionLookup and other dependencies that it's currently getting from service locator, such as ContentHandlerFactory,

Baby steps :) I don't think we should continue using the service locator if we don't have to, especially if it's just to accommodate an unmaintained extension. Hacking LiquidThreads seems acceptable to me. What do you think @dmaza ? See LiquidThreads patch at https://gerrit.wikimedia.org/r/c/mediawiki/extensions/LiquidThreads/+/631219/

I don't think we should continue using the service locator if we don't have to, especially if it's just to accommodate an unmaintained extension. Hacking LiquidThreads seems acceptable to me. What do you think dmaza ? See LiquidThreads patch at https://gerrit.wikimedia.org/r/c/mediawiki/extensions/LiquidThreads/+/631219/

Disregard. I've +2'd https://gerrit.wikimedia.org/r/c/mediawiki/core/+/631222 which switches to using the service container. I think ApiEditPage as a whole could be converted to using DI at a later time, since there are multiple uses of the service container, as Ammarpad points out. For now, let's just be done :) Thanks for your patience, Ammarpad!

Change 631222 merged by jenkins-bot:
[mediawiki/core@master] Revert "Revert "ApiEditPage: Show existing watchlist expiry if status is not being changed.""

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

Change 630798 merged by jenkins-bot:
[mediawiki/core@REL1_35] ApiEditPage: Show existing watchlist expiry if status is not being changed.

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

dom_walden added subscribers: ifried, dom_walden.
  1. Watch any page temporarily
  2. Use the API as below to run an edit without making changes to the watchlist (watchlist=nochange)

For example, with Test123454 watched for 1 week. Submitting an API request with watchlist=nochange and no expiry set:

{
    "edit": {
        "result": "Success",
        "pageid": 229559,
        "title": "Test123454",
        "contentmodel": "wikitext",
        "nochange": true,
        "watched": true,
        "watchlistexpiry": "2020-10-09T07:46:28Z"
    }
}

(Which is the correct watch expiry.)

If the same page is watched permanently. Submitting an API request with watchlist=nochange and no expiry set:

{
    "edit": {
        "result": "Success",
        "pageid": 229559,
        "title": "Test123454",
        "contentmodel": "wikitext",
        "oldrevid": 453697,
        "newrevid": 453698,
        "newtimestamp": "2020-10-02T09:22:38Z",
        "watched": true
    }
}

Note it does not return a watchlistexpiry. @ifried @MusikAnimal Perhaps it should state that watchlistexpiry: infinite, like we did previously.

This also happens if I choose to watch and set the expiry to "infinite" (e.g. https://en.wikipedia.beta.wmflabs.org/wiki/Special:ApiSandbox#action=edit&format=json&title=Test123454&watchlist=watch&watchlistexpiry=infinite)

Test environments:

Change 632290 had a related patch set uploaded (by Ammarpad; owner: Ammarpad):
[mediawiki/core@master] ApiEditPage: Inject dependencies

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

If the same page is watched permanently. Submitting an API request with watchlist=nochange and no expiry set:

{
    "edit": {
        "result": "Success",
        "pageid": 229559,
        "title": "Test123454",
        "contentmodel": "wikitext",
        "oldrevid": 453697,
        "newrevid": 453698,
        "newtimestamp": "2020-10-02T09:22:38Z",
        "watched": true
    }
}

Note it does not return a watchlistexpiry. @ifried @MusikAnimal Perhaps it should state that watchlistexpiry: infinite, like we did previously.

This also happens if I choose to watch and set the expiry to "infinite" (e.g. https://en.wikipedia.beta.wmflabs.org/wiki/Special:ApiSandbox#action=edit&format=json&title=Test123454&watchlist=watch&watchlistexpiry=infinite)

I thought about that in code review, but I thought the old behaviour was to show nothing unless there is a non-indefinite expiry. But I see testwiki (which still doesn't have this patch) indeed returns "watchlistexpiry": "infinity". I think it's an easy fix to bring this back, if we want it. Pinging @Ammarpad for input.

dmaza added a comment.Oct 5 2020, 11:57 PM

... I thought the old behaviour was to show nothing unless there is a non-indefinite expiry...

I was under that impression too. It would make sense to keep this consistent across API(s). IIRC at least watch API will not return an expiry param unless you are passing one in.

In my view, if there's no expiry, then 'watchlistexpiry' should not appear in the result, that simply signifies that there's no expiry as the name suggests. That'd also be consistent with the conventional 'watch' action. (If you're not watching, the 'watched' status will not be included in the result at all).

The previous code in ApiEditPage (wmf.10) was adding 'watchlistexpiry' based on the whole feature being turned on, even if watchlistexpiry was not specifically given. It did no harm, but it created inconsistency with the 'watch' parameter as I said above. The 'watched' behavior looked sensible to me , that's why I changed it for them to be consistent. ApiEditPage and ApiBock now behave the same way.

I have looked at the other modules that allow watchexpiry. Six core modules: ApiMove, ApiDelete, ApiRollback., ApiUndelete, ApiUpload and ApiProtect. Although these modules have watchexpiry support, but the result of that action is not included in their result responses at all. So no module is saying "watchlistexpiry": "infinity" now in master.

I believe there should be more consistency though. If watchlistexpiry should be included unconditionally, then all modules that supports expiry should do the same thing. Ideally watch should also be removed from the conditional, so that when the action is not to watch, the result should say "watched: false".

Restricted Application edited projects, added Community-Tech; removed Community-Tech (Kanban-2020-21-Q2). · View Herald TranscriptOct 20 2020, 8:21 PM
ifried closed this task as Resolved.EditedFri, Nov 6, 5:54 PM

This work has been released. If developers would like to clean up when "watchlistexpiry" is mentioned to match the standard behavior of APIs, they can certainly do so, but such work can be done in a separate ticket. Since this work is complete and released, I'm marking it as Done.