Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Offline Mode: Deduplicate new posts #23152

Closed
wants to merge 2 commits into from

Conversation

momo-ozawa
Copy link
Contributor

Fixes pe7hp4-N2-p2#comment-328

Description

  • Fixes duplicate posts from appearing in the posts list when refresh and sync (upon regaining connectivity) happens in parallel

Notes

  • According to Apple Docs, you must not override hash for NSManagedObject, which is why I added the postHash computed variable

How to test

Precondition: you are offline

  • Create and save a new draft
  • Open the post for editing and make more changes
  • Enable connectivity
  • Pull to refresh
  • ✅ Verify that duplicate posts don't appear when refresh and sync (upon regaining connectivity) happens in parallel

Regression Notes

  1. Potential unintended areas of impact
    n/a

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    n/a

  3. What automated tests I added (or what prevented me from doing so)
    n/a

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@dangermattic
Copy link
Collaborator

1 Warning
⚠️ This PR is assigned to the milestone 24.8 ❄️. The due date for this milestone has already passed.
Please assign it to a milestone with a later deadline or check whether the release for this milestone has already been finished.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Contributor

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr23152-8a80970
Version24.8
Bundle IDcom.jetpack.alpha
Commit8a80970
App Center Buildjetpack-installable-builds #8835
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr23152-8a80970
Version24.8
Bundle IDorg.wordpress.alpha
Commit8a80970
App Center BuildWPiOS - One-Offs #9789
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@@ -541,7 +541,7 @@ class AbstractPostListViewController: UIViewController,
let posts = try result.map { try coreDataStack.mainContext.existingObject(with: $0) }
let hasMore = result.count >= number

return (posts, hasMore)
return (posts.deduplicated(by: \.postHash), hasMore)
Copy link
Contributor

@kean kean May 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This leaves a period of time where database has two posts with the same postID – invalid state. I suggest fixing it on the PostService/getPostWithID level where it decides when to create a new or update the existing post. The app has to make sure that it keeps the local variant as it might have newer revisions at this point.

AbstractPost *post = [blog lookupPostWithID:postID inContext:self.managedObjectContext];

I suggest updating the lookup with a foreignID (or whatever you want to call it).

I tested passing a foreign ID using the metadata field (only tested wp.com).

Request:

{
  "metadata": [
    {
      "key": "wp-jp-app-foreign-id",
      "operation": "update",
      "value": "x-coredata://ED210C7A-B958-4371-8193-2B708EB122B5/Post/p1218"
    }
  ],
  "format": "standard",
  "content": "<!-- wp:paragraph -->\n<p>1</p>\n<!-- /wp:paragraph -->",
  "status": "draft",
  "title": "Off-04",
  "type": "post",
  "author": 235722583
}

Response:

{
  "ID": 713,
  "site_ID": 231378612,
  // ...
  "metadata": [
    {
      "id": "1782",
      "key": "wp-jp-app-foreign-id",
      "value": "x-coredata://ED210C7A-B958-4371-8193-2B708EB122B5/Post/p1218"
    }
  ],
  // ...
}

The list request /rest/v1.2/sites/*/posts also return the posts with wp-jp-app-foreign-id key.

Note: I used objectID as an example, but it is probably the best type of ID we could send. It would probably be best to generate a UUID, but it'll require database schema changes.

Copy link
Contributor

@staskus staskus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix!

I suggest fixing it on the PostService/getPostWithID level where it decides when to create a new or update the existing post

The issue is still reproducible, and this could be the right suggestion.

Those duplicated posts are created in the db and get picked up by NSFetchedResultsController (https://www.diffchecker.com/671fRofD/).

As far as I understood, the post list returned by syncPosts is used to index iOS search and update some posts date filters but not include or exclude particular posts. So even after I removed this return statement the post list updates continued to work.

@momo-ozawa
Copy link
Contributor Author

Closing this PR in favor of #23173

@momo-ozawa momo-ozawa closed this May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants