Conflict Resolution: Pages

zwarm commented 1 month ago

The PR adds support for resolving page conflict resolutions

Version conflicts
Autosave conflicts The new PostConflictResolutionOverlay is shown in place of the older Android dialog. Note: Other dialogs remain as is.

Specs pcdRpT-5ID-p2#comment-9638.

To Test:

Install the Jetpack and WordPress apps from this PR

Test Page version forced write on top of web (existing logic)

Login with a site that has pages
Navigate to Me > Debug Settings
Disable the sync_publishing flag and restart the app
Navigate to back to My Site > Pages
Tap the publish tab and then tap a page to open the edit view
PAUSE - GO TO YOUR LAPTOP
Open a browser and navigate to the same WordPress site
Open the same page and make an edit (remember what you changed)
Tap the "update" button to save it and back out of the editor
GO BACK TO YOUR DEVICE
Make a change to the page you have in the edit view
Tap "Update" to save the page to the server
✅ Verify the page did update (snackbar)
GO BACK TO THE LAPTOP
Tap view on the same page you just edited via the app
✅ Verify the changes you made on the device have overwritten the changes you made on your laptop
Repeat this test for Jetpack and Wordpress

Test Page version did not write on top of web (new logic)

Navigate to Me > Debug Settings
Enabled the sync_publishing flag and restart the app
Navigate to back to My Site > Pages
Tap the publish tab and then tap a page to open the edit view
PAUSE - GO TO YOUR LAPTOP
Open a browser and navigate to the same WordPress site
Open the same page and make an edit (remember what you changed)
Tap the "update" button to save it and back out of the editor
GO BACK TO YOUR DEVICE
Make a change to the page you have in the edit view
Tap "Update" to save the page to the server
✅ Verify the page did not update and the post list shows "Version Conflict" within the post row
GO BACK TO THE LAPTOP
Tap view on the same page you just edited via the app
✅ Verify the changes you made on the device did not overwrite the changes you made on your laptop
Repeat this test for Jetpack and Wordpress

Test Conflict Resolution dialog is shown

Continue from the last step of test "Page version did not write on top of web (new logic)"
On the Pages list, tap on the page that shows "Version Conflict"
✅ Verify the conflict resolution dialog is shown
Tap local version
✅ Verify the local version has been applied
Repeat the scenario in test "Page version did not write on top of web (new logic)" to get into a Version Conflict again. Then repeat this test and tap web version in the conflict resolution dialog.

Regression Notes

Potential unintended areas of impact The page is not updated properly
What I did to test those areas of impact (or what existing automated tests I relied on) Updated PagesViewModelTest, PageListViewModelTest, and CreatePageListItemActionsUseCaseTest
What automated tests I added (or what prevented me from doing so) Relied on existing instrumentation tests

PR Submission Checklist:

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

Testing Checklist (strike-out the not-applying and unnecessary ones):

[X] WordPress.com sites and self-hosted Jetpack sites.

dangermattic commented 1 month ago

	2 Warnings
:warning:	`strings.xml` files should only be updated on release branches, when the translations are downloaded by our automation.
:warning:	This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by :no_entry_sign: Danger

wpmobilebot commented 1 month ago

📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.

	App Name	WordPress
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr20635-76f04c8
	Commit	76f04c8c3725fdfe1579d60566a9c08c82d2dd98
	Direct Download	`wordpress-prototype-build-pr20635-76f04c8.apk`

Note: Google Login is not supported on these builds.

wpmobilebot commented 1 month ago

📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.

	App Name	Jetpack
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr20635-76f04c8
	Commit	76f04c8c3725fdfe1579d60566a9c08c82d2dd98
	Direct Download	`jetpack-prototype-build-pr20635-76f04c8.apk`

Note: Google Login is not supported on these builds.

codecov[bot] commented 4 weeks ago

Codecov Report

Attention: Patch coverage is 43.94904% with 88 lines in your changes are missing coverage. Please review.

Project coverage is 40.46%. Comparing base (3f7287b) to head (76f04c8). Report is 39 commits behind head on trunk.

Files	Patch %	Lines
...ss/android/viewmodel/pages/PageListDialogHelper.kt	0.00%	42 Missing :warning:
...ordpress/android/viewmodel/pages/PagesViewModel.kt	47.50%	20 Missing and 1 partial :warning:
...s/android/viewmodel/pages/PageListEventListener.kt	0.00%	18 Missing :warning:
...ordpress/android/ui/posts/PostListMainViewModel.kt	55.55%	3 Missing and 1 partial :warning:
...wordpress/android/ui/posts/PostConflictDetector.kt	60.00%	0 Missing and 2 partials :warning:
.../android/viewmodel/pages/PageUploadErrorWrapper.kt	0.00%	1 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## trunk #20635 +/- ## ======================================== Coverage 40.46% 40.46% ======================================== Files 1485 1488 +3 Lines 68443 68549 +106 Branches 11312 11329 +17 ======================================== + Hits 27696 27739 +43 - Misses 38246 38309 +63 Partials 2501 2501 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

pantstamp commented 3 weeks ago

@zwarm great work! While I was testing it I saw that when the conflict resolution overlay appears for a page and you choose to keep the remote version, event though we see the snackbar that says "Local version discarded", the pages list is not refreshed and we still see the "Version Conflict" label. User need to refresh for it to disappear. Before approving the PR, I will investigate a bit.

pantstamp commented 3 weeks ago

@zwarm as I am looking at the code I noticed one difference between post conflict resolution and pages conflict resolution.

When we update a post with the remote version we call

// Conflicted post has been successfully updated with its remote version
            uploadStore.clearUploadErrorForPost(updatedPost)
            postStore.removeLocalRevision(updatedPost)

in the PostConflictResolver.onPostSuccessfullyUpdated().

I think that these functions are not called anywhere when we update a page with the remote version. Don't we need this there? WDYT?

pantstamp commented 3 weeks ago

@zwarm just a thought. If resolving a page conflict is actually resolving a post conflict, would it make sense to reuse the code of the PostConflictResolver? The PageConflictResolver could act like a wrapper for the PostConflictResolver. In this way we could possibly remove the "resolve conflict" logic from the PagesViewModel. WDYT? This is just food for thought an not a request for changes.

zwarm commented 3 weeks ago

@zwarm just a thought. If resolving a page conflict is actually resolving a post conflict, would it make sense to reuse the code of the PostConflictResolver? The PageConflictResolver could act like a wrapper for the PostConflictResolver. In this way we could possibly remove the "resolve conflict" logic from the PagesViewModel. WDYT? This is just food for thought an not a request for changes.

I did think of this originally, but Pages is not architected the same as Posts. Plus I'm not 100% sure about eliminating the logic from PagesViewModel without fully understanding what we'd be removing. However, I am open to exploring again.

zwarm commented 3 weeks ago

@zwarm great work! While I was testing it I saw that when the conflict resolution overlay appears for a page and you choose to keep the remote version, event though we see the snackbar that says "Local version discarded", the pages list is not refreshed and we still see the "Version Conflict" label. User need to refresh for it to disappear. Before approving the PR, I will investigate a bit.

Nice catch, I'll take a look and get this changed.

pantstamp commented 3 weeks ago

@zwarm just a thought. If resolving a page conflict is actually resolving a post conflict, would it make sense to reuse the code of the PostConflictResolver? The PageConflictResolver could act like a wrapper for the PostConflictResolver. In this way we could possibly remove the "resolve conflict" logic from the PagesViewModel. WDYT? This is just food for thought an not a request for changes.

I did think of this originally, but Pages is not architected the same as Posts. Plus I'm not 100% sure about eliminating the logic from PagesViewModel without fully understanding what we'd be removing. However, I am open to exploring again.

As I said, this is not a request for change :-) We might consider it afterwards if we see that we duplicate a lot of code.

sonarcloud[bot] commented 3 weeks ago

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

zwarm commented 3 weeks ago

Thanks for the refactors @pantstamp 👏 Like the way you moved the logic out of PagesViewModel and added the conflictResolver. I tested and all works as expected. 🙇

sentry-io[bot] commented 3 weeks ago

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

‼️ NullPointerException: Parameter specified as non-null is null: method org.wordpress.android.ui.pages.PagesFragment$setu... org.wordpress.android.ui.pages.PagesFragment$se... View Issue

_{Did you find this useful? React with a 👍 or 👎}

wordpress-mobile / WordPress-Android