Page MenuHomePhabricator

[Talk Pages Dev] Talk Page Offline Support
Closed, DeclinedPublic

Description

This is the v. .0001 initial handling. I am currently working towards this.

Pulling:

  1. We pull existing local talk page from DB if one exists.
  2. We fetch the latest revisionIDs from API (regardless of local talk page existence in 1)
  3. If that succeeds, compare revision IDs. If it’s the same return local talk page to the screen.
  4. If revision IDs are not the same, fetch from the network, throw out old talk page and add new talk page to DB (note all old discussions are deleted from local and recreated regardless of if they changed)
  5. Every user talk page they visited will be persisted, there’s not any sort of expiration date or maximum # of talk pages we have persisting.
  6. Note all failures (pulling existing talk page, fetching revision IDs, fetching from network) stop it entirely. User should see some sort of error popup.
  7. Note database section html is in CoreData as a string.

Posting:

  1. If user doesn’t have a network connection, disable reply / new discussion buttons, make sure standard network banner shows.
  2. If user does have a network connection, fetch a revision ID.
  3. If fetch succeeds, compare to revisionID to revisionID user is replying to. If it’s the same, post the new reply to the API. If post succeeds, fetch talk page again which will propagate into the database and refresh the list screen. User will see a spinner during all of these chained calls so it could take a while.
  4. For failure on any of these steps (fetch revision ID, posting a reply, fetching the talk page again) user will see an error after posting. Even on the last step if last fetch fails after the post technically succeeded.

Improvements to be made:

The only benefit as far as I can tell from the initial setup is not needing to fetch from the API if the revisionID hasn’t changed, which means the user will see the data faster. We can get more benefits with these:

Pulling:

  1. Currently user will see a spinner on initial fetch during steps 1-3/4. Instead we should make it more progressive. If a local user talk page exists, return it immediately to screen for user to see but continue with the rest of the steps. We will need to make sure new discussion/reply buttons are disabled since we are still confirming version with the server. Once revisionID comparison & API talk page fetch comes through, update the DB and refresh the screen with the new data, and enable the new discussion/reply buttons.
  2. If local db talk page fetch fails, continue on with network fetch instead of erroring out.
  3. If there’s no local talk page, we still chain the calls to fetch the revisionID then fetch the talk page. We could dispatch group this particular situation instead of chaining.
  4. If latest revisionID fetch fails, continue on to fetching talk page instead of erroring out.
  5. If latest revisionID or talk page fetch fails and local page is already showing on screen because of step 1, continue showing local talk page but disable new discussion/reply buttons.
  6. Right now when talk page comes through from API, we throw out all old discussions and recreate them in the db. We could make it smarter and diff it, only throwing out discussions that have changed. This could also limit cases of question #1 below.
  7. Right now every user talk page is persisted in the DB (this is regardless of it’s on their watchlist/reading list). Seems like we should limit this - persist an expiration date or a created date and only keep the last 10 or so talk pages in the DB.
  8. Since sections could get rather long, we should not store the actual html in Core Data. Instead store a link to a local file with the html.

Posting:

  1. Allow user to reply/create a new discussion regardless of if user has a network connection or not, add post to local database with a local pending flag (can use this to display a pending tag to the user as well). User will immediately see their reply in the UI.
  2. As a part of a background sync process (similar to reading lists I gather?), queue up a sync operation that takes the post (with its current revisionID, fetches the latest revision ID, if latest revID = current revID attempts to post. Once post goes through switch off the pending flag in the local db. Network syncing operations should be serial so user can post multiple replies but latter replies won’t attempt to post until the first reply goes through. If any of the API calls fail, depending on the severity of the error we either try again later or delete the sync operations (and any reply sync operations lined up behind it) and delete the replies out of the local database. If revisionID has incremented since posting, delete sync operations and delete pending replies.
  3. We also might want an expiration date as a part of these sync operations and cancel the operations / delete the local db replies if it’s past that date. We shouldn’t attempt to sync something indefinitely if for some unknown reason it’s never going to work.

Questions:

  1. If user is viewing one of their old local threads and the fetch API comes through informing us that this discussion no longer exists, do we automatically pop them out of the screen? Or don’t do anything and when they go back to the discussion list they won’t be able to get back to that old thread anymore?
  2. If one of these situations calls for the new discussion/reply buttons to be disabled, do we inform the user somehow that this is why they are disabled?
  3. Should we have special syncing preference options in Settings (a la “Article storage and syncing”)? Or how does this screen apply to talk pages?
  4. If somehow the post sync operations fail (either revisionID incremented or otherwise) and cause pending local replies to be deleted, do we inform the user?
  5. Engineering thoughts on how adding a talk page to reading list will work? My thinking is we add a isBookmarked flag to the talk page db object, and make sure we never clean it out in step Improvements.Pulling.7. Might be hard to integrate that into the reading list UI screen / syncing operations without significant rework though.

Event Timeline

Tsevener renamed this task from Talk Page Offline Support to [Talk Pages Dev] Talk Page Offline Support.May 7 2019, 2:31 PM

These are notes from talks last week:

Offline ask is if a talk page was open when user backgrounded or the app was terminated, restore it when user comes back to the app.

This might involve saving an isOpen flag in CoreData and using housekeeping to clean out non-isOpen articles upon launch.

Notes from task sync -

No need for smart offline retry. Keep the draft open, if API fails (via network connection failure or server failure) show error state. User should be able to background/foreground and try again later.

No need to fetch revision ID again when they try to post. Since replies are always append, we aren't going to worry about race conditions / someone posting another reply in between initial fetch and reply at this point.

We were able to tackle some of these improvements for v1 - I have documented below:

Pulling:

Currently user will see a spinner on initial fetch during steps 1-3/4. Instead we should make it more progressive. If a local user talk page exists, return it immediately to screen for user to see but continue with the rest of the steps. We will need to make sure new discussion/reply buttons are disabled since we are still confirming version with the server. Once revisionID comparison & API talk page fetch comes through, update the DB and refresh the screen with the new data, and enable the new discussion/reply buttons.

This is in place.

If local db talk page fetch fails, continue on with network fetch instead of erroring out.

Not in place.

If there’s no local talk page, we still chain the calls to fetch the revisionID then fetch the talk page. We could dispatch group this particular situation instead of chaining.

This is in place.

If latest revisionID fetch fails, continue on to fetching talk page instead of erroring out.

Not in place.

If latest revisionID or talk page fetch fails and local page is already showing on screen because of step 1, continue showing local talk page but disable new discussion/reply buttons.

Almost in place (in https://github.com/wikimedia/wikipedia-ios/pull/3133)

Right now when talk page comes through from API, we throw out all old discussions and recreate them in the db. We could make it smarter and diff it, only throwing out discussions that have changed. This could also limit cases of question #1 below.

This is in place.

Right now every user talk page is persisted in the DB (this is regardless of it’s on their watchlist/reading list). Seems like we should limit this - persist an expiration date or a created date and only keep the last 10 or so talk pages in the DB.

Almost in place (in https://github.com/wikimedia/wikipedia-ios/pull/3134)

Since sections could get rather long, we should not store the actual html in Core Data. Instead store a link to a local file with the html.

Not in place.

Posting

Allow user to reply/create a new discussion regardless of if user has a network connection or not, add post to local database with a local pending flag (can use this to display a pending tag to the user as well). User will immediately see their reply in the UI.
As a part of a background sync process (similar to reading lists I gather?), queue up a sync operation that takes the post (with its current revisionID, fetches the latest revision ID, if latest revID = current revID attempts to post. Once post goes through switch off the pending flag in the local db. Network syncing operations should be serial so user can post multiple replies but latter replies won’t attempt to post until the first reply goes through. If any of the API calls fail, depending on the severity of the error we either try again later or delete the sync operations (and any reply sync operations lined up behind it) and delete the replies out of the local database. If revisionID has incremented since posting, delete sync operations and delete pending replies.
We also might want an expiration date as a part of these sync operations and cancel the operations / delete the local db replies if it’s past that date. We shouldn’t attempt to sync something indefinitely if for some unknown reason it’s never going to work.

Not in place.

Questions:

If user is viewing one of their old local threads and the fetch API comes through informing us that this discussion no longer exists, do we automatically pop them out of the screen? Or don’t do anything and when they go back to the discussion list they won’t be able to get back to that old thread anymore?
If one of these situations calls for the new discussion/reply buttons to be disabled, do we inform the user somehow that this is why they are disabled?
Should we have special syncing preference options in Settings (a la “Article storage and syncing”)? Or how does this screen apply to talk pages?
If somehow the post sync operations fail (either revisionID incremented or otherwise) and cause pending local replies to be deleted, do we inform the user?
Engineering thoughts on how adding a talk page to reading list will work? My thinking is we add a isBookmarked flag to the talk page db object, and make sure we never clean it out in step Improvements.Pulling.7. Might be hard to integrate that into the reading list UI screen / syncing operations without significant rework though.

Not in place.