Offline Pages : Added Support for unpublished revision to Page List

[jd-alexander]

Fixes #11131

Findings

The Page List didn't support loading an unpublished revision when changes were made on other devices.

Solution

• To reuse logic that allows the Post List to support this functionality. Most of this logic lives within the PostListDialogHelper. Since it has way more responsibilities than required in this PR, a PageListDialogHelper was created to extract only the functionality needed.

• While adding this feature, I noticed that the Delete Page Confirmation Dialog was utilizing the BasicFragmentDialog and mixing both implementations would reduce code quality so I opted to migrate the Delete Confirmation Dialog to the PageListDialogHelper for consistency.

• For the "You've made unsaved changes to this page" label, I didn't add it since @malinajirka will be working on a PR to refactor how labels are generated and that would cause a conflict.

• I reused the tracking events from the PostListDialogHelper but I am wondering if pages need to be tracked separately from posts.

Testing

Page Auto Revision Testing

1. Edit a page in WP Calypso and don't click save. You can monitor the network tab in the chrome debugging tools to see when an autosave request is made since this implementation doesn't contain a label.
2. Ensure the device is online and then refresh the page.
3. Click on the page that was edited online and you should see the dialog below.

Before Click	After Click

4. Verify that clicking The Version From Another Device shows the contents that exist in Calypso.
5. Click back without saving and press the page again to verify that The Version From This App is accurate.

Page Deletion Testing

1. Go to the Trash tab and click the More icon.
2. Select Delete Permanently
3. A dialog should be shown that asks you if you are sure you want to delete the page.
4. Once you confirm the deletion, the page should be removed and the list should be empty. A snackbar should also be shown confirming deletion.

Before	After

Reviewing

Only 1 reviewer is needed but anyone can review.

Submitter Checklist

• I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.
• I have considered adding accessibility improvements for my changes.
• If it's feasible, I have added unit tests.

[peril-wordpress-mobile]

You can test the changes on this Pull Request by downloading the APK here.

[malinajirka]

Great job @jd-alexander! I haven't found any major issues. I've left a couple of minor comments with some possible improvements. Let me know what you think ;)

To reuse logic that allows the Post List to support this functionality. Most of this logic lives within the PostListDialogHelper. Since it has way more responsibilities than required in this PR, a PageListDialogHelper was created to extract only the functionality needed.

We might want to consider extracting each dialog into its own class. That'd allow us to share the logic between the screens. But let's consider this during the cleanup phase of the project.

While adding this feature, I noticed that the Delete Page Confirmation Dialog was utilizing the BasicFragmentDialog and mixing both implementations would reduce code quality so I opted to migrate the Delete Confirmation Dialog to the PageListDialogHelper for consistency.

Good call ;)! It might be even better to separate it into a different PR, but it doesn't matter since this PR is pretty straightforward. Thanks!

I reused the tracking events from the PostListDialogHelper but I am wondering if pages need to be tracked separately from posts.

Hmm, we might want to consider adding something like "type:post/page" attribute to the event. Wdyt?

For the "You've made unsaved changes to this page" label, I didn't add it since @malinajirka will be working on a PR to refactor how labels are generated and that would cause a conflict.

👍

Btw I noticed branch names on your PRs don't follow the issue/[issue_number]_more_details pattern. AFAIK our CI doesn't count on the pattern, but I think it'd be good to follow it for consistency with other branch names.

[jd-alexander]

Great job @jd-alexander! I haven't found any major issues. I've left a couple of minor comments with some possible improvements. Let me know what you think ;)

Awesome. Thanks again for another thoughtful review

We might want to consider extracting each dialog into its own class. That'd allow us to share the logic between the screens. But let's consider this during the cleanup phase of the project.

Indeed! I created an issue to track this #11272

Good call ;)! It might be even better to separate it into a different PR, but it doesn't matter since this PR is pretty straightforward. Thanks!

Thanks for the comment 😄

Hmm, we might want to consider adding something like "type:post/page" attribute to the event. Wdyt?

Makes sense! I added it ac2129b So I should create an issue to add this to the PostDialogHelper right?

Btw I noticed branch names on your PRs don't follow the issue/[issue_number]_more_details pattern. AFAIK our CI doesn't count on the pattern, but I think it'd be good to follow it for consistency with other branch names.

Thanks! I sometimes create a quick branch when I start playing around, renamed it locally and didn't realize I didn't push the new name. I will make sure all new PRs adhere to this from the start for sure 👍

[jkmassel]

We're freezing 14.2 today, so this PR is being bumped to 14.3. If you'd like it to ship with 14.2, please merge it into release/14.2 and ping me – I'll be happy to cut a new beta release.

[malinajirka]

Makes sense! I added it ac2129b So I should create an issue to add this to the PostDialogHelper right?

Thanks! Yeah, please create a ticket and link this usage there. I think we'll want share POST_TYPE to make sure no-one changes one place but forgets about the other.

Thank you for all the changes! It all LGTM. I'm merging this PR even though the connect tests are failing, since they are failing on a lot of the PRs. The issue is not related to this PR.