Page MenuHomePhabricator

Test integrating Android app with Reading List Service
Closed, ResolvedPublic3 Story Points

Description

Begin testing the Reading List Service in the Android app:

https://github.com/wikimedia/restbase/pull/887

This probably requires setting up vagrant to test, see @Tgr for more information

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Tgr added a comment.Oct 10 2017, 4:27 PM

If you use vagrant, cherry-pick c380785 and parents, provision the readinglists role, and apply the RESTBase patches from the PR.
If you don't, install the ReadingLists extension in the usual way, and apply the relevant part of the vagrant role manually.

Tgr added a comment.Oct 23 2017, 6:31 PM

In case you haven't seen the comments in T168986: RL is available on beta and labs now:

cooltey claimed this task.Oct 25 2017, 6:02 PM

You should be using the https://readinglists.wmflabs.org url, since this is the site on which you're logged in to test the reading lists.
One quick note about CSRF tokens: the API will give you a token that looks like 160341316df9190ce65fbce73830a2cb49f12890+\\, but the last two slashes are actually an escaped single slash, so the token that you'll pass into your next request should be 160341316df9190ce65fbce73830a2cb49f12890+\

@Dbrant Thank you so much! This little slash cost me a lot of time 😿

cooltey added a comment.EditedOct 26 2017, 6:23 PM

Have some questions after doing the test on the https://readinglists.wmflabs.org/api/rest_v1/#/Reading_lists (using the "Try it now" tool)

  • Cannot add the same entry if the entry has been deleted.
    1. Add an entry to list id:8: United_States ( returns 200)
    2. Check the entries list id:8: United_States` exists in the list
    3. Delete the United_States entry from list id:8
    4. Check the entries list id:8: United_States does not exist
    5. Add an entry to list id:8: United_States —> return 500 and shows content exists
    6. Check entries list id:8 again: United_States does not exist
  • Get an Integer value of name , and it happens in the following API. Should we get "name": "Planets" instead of "name": 0?
    1. /data/lists/
    2. /data/lists/page/{project}/{title}
  • Not sure how to use it, it returns 500 with the message List has been deleted
    1. /data/lists/changes/since/{date}
Tgr added a comment.Oct 26 2017, 8:35 PM
  • Cannot add the same entry if the entry has been deleted.

Yeah, that's a bug. We use soft deletion and unique DB indexes so recreation will need special handling.

  • Get an Integer value of name , and it happens in the following API. Should we get "name": "Planets" instead of "name": 0?

That will be fixed once https://gerrit.wikimedia.org/r/#/c/379702/ is merged. I guess I should just cherry-pick it to the test machine.

  • Not sure how to use it, it returns 500 with the message List has been deleted
    1. /data/lists/changes/since/{date}

Can't see how that could happen. What user and URL did you get it with?

Tgr added a comment.Oct 26 2017, 8:37 PM
  • Not sure how to use it, it returns 500 with the message List has been deleted
    1. /data/lists/changes/since/{date}

Can't see how that could happen. What user and URL did you get it with?

Nvm, that's also an older bug: https://gerrit.wikimedia.org/r/#/c/380695/

Will cherry-pick that as well.

Tgr added a comment.Oct 26 2017, 10:03 PM

The fix for all three issues is cherry-picked now. (The first is tracked in T179120.)

@Tgr Thanks for the information!

Have some questions after doing the test on the https://readinglists.wmflabs.org/api/rest_v1/#/Reading_lists (using the "Try it now" tool) - part 2

  • put /data/lists/{id}
    1. If we delete a list, and we can still use put to update the list that has been deleted. It will return 200 instead of 500
  • post /data/lists/{id}/entries
    1. If we delete a list, and we still can post an entry into the list
  • delete /data/lists/{id}/entries/{entry_id} and delete /data/lists/{id}
    1. When quick click on the “Try it out!” button, sometimes it will return the following messages:
"title": "internal_api_error_LogicException"
"detail": [616fe8d35a71832f541805f8] Exception caught: deleteListEntry failed for unknown reason
  • get /data/lists/{id}/entries/
    1. Does it only support the en.wikipedia.org ? (because of the only testing purposes?)
  • put /data/lists/{id}/order and put /data/lists/order
    1. The request goes success even if the list_entries does not contain all the entries
      • List contains: [1, 2, 3, 4, 5]
      • Put [5,4,3,2,1]-> return 200 -> expected
      • Put [6, 5,4,3,2,1]-> return 500 -> expected
      • Put [2,1]-> return 200 -> not sure

Change 387185 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/extensions/ReadingLists@master] Fix deleted row handling

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

Change 387186 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/extensions/ReadingLists@master] Fix handling of partial order parameters

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

Tgr added a comment.Oct 30 2017, 9:05 AM

@cooltey thanks for the thorough testing :) Those bugs should be fixed now.

Does it only support the en.wikipedia.org ? (because of the only testing purposes?)

At the moment, yes. (Although it should only affect the presence of the summaries.) I can add more domains if it's useful.

@Tgr Thanks!

At the moment, yes. (Although it should only affect the presence of the summaries.) I can add more domains if it's useful.

That would be great if you can also add es.wikipedia.org and also zh.wikipedia.org

Tgr added a comment.Oct 30 2017, 7:53 PM

That would be great if you can also add es.wikipedia.org and also zh.wikipedia.org

Done.

Thanks, @Tgr.

All the APIs look good now except one minor question below:

  • delete /data/lists/{id}/entries/{entry_id}
    1. When trying to delete an entry (e.g. 27), and it belongs to a list (e.g. 19). If we set a list id that is not related to the entry we want to delete, it also could be processed.
      • Try to delete entry_id: 27 and put id: 19 => entry entry_id: 27 will be deleted => Correct
      • Try to delete entry_id: 27 and put id: 20 => entry entry_id: 27 will also be deleted => Entry entry_id : 27 does not belong to list id: 20
Tgr added a comment.Oct 30 2017, 9:34 PM

Yeah, the list ID is not actually used for deletion (the backend API only takes an entry ID), it's only there because the URL hierarchy seemed more logical that way. Given that deletion URLs will never be cached, I don't think that can cause problems.

@Tgr I see.

Excellent works! I have completed the tests.

Dbrant closed this task as Resolved.Nov 1 2017, 2:50 PM

Change 387185 merged by jenkins-bot:
[mediawiki/extensions/ReadingLists@master] Fix deleted row handling

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

Change 387186 merged by jenkins-bot:
[mediawiki/extensions/ReadingLists@master] Fix handling of partial order parameters

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