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

Adds Duplicate/Copy post functionality #13521

Merged
merged 27 commits into from
Dec 8, 2020
Merged

Adds Duplicate/Copy post functionality #13521

merged 27 commits into from
Dec 8, 2020

Conversation

antonis
Copy link
Contributor

@antonis antonis commented Dec 2, 2020

Fixes #7954

Depends on: wordpress-mobile/WordPress-FluxC-Android#1794

Description

This PR implements the "Duplicate post" functionality (already exists on Calypso)

Design

Blueprint Android

Note: for post with conflicts or autosave versions I chose to redirect to the edit flow. It is debatable if this is the right approach but duplicating the edit flow conflict handling for the copy functionality seemed to add unnecessary complexity in my view

To test

Duplicate published post

  1. From My Site press the Posts button or scroll down and select Blog Posts from the list
  2. Create a new post or open a post by tapping on it
  3. Take a note of the tile and content
  4. In post cards mode press the More button / In post list mode press the dots at the right of the post
  5. Verify that the Duplicate button is visible as a last item
  6. Press the Duplicate button
  7. Verify that the editor opens with the title and content from step 3
  8. Make a minor change
  9. Publish or exit to save the post as draft
  10. Verify that both the original and the duplicate post are listed

Duplicate draft post

  1. From My Site press the Posts button or scroll down and select Blog Posts from the list
  2. Select the Drafts (second) tab
  3. Create a new post or open a post by tapping on it
  4. Take a note of the tile and content
  5. In post cards mode press the More button / In post list mode press the dots at the right of the post
  6. Verify that the Duplicate button is visible as a last item
  7. Press the Duplicate button
  8. Verify that the editor opens with the title and content from step 4
  9. Make a minor change
  10. Publish or exit to save the post as draft
  11. Verify that both the original and the duplicate post are listed

Note: For the steps below I'm not sure how to consistently create conflicts for testing. Any input on this is welcome.

Duplicate post with conflict

  1. From My Site press the Posts button or scroll down and select Blog Posts from the list
  2. Select a post with conflicts or a newer autosave version
  3. In post cards mode press the More button / In post list mode press the dots at the right of the post
  4. Verify that the Duplicate button is visible as a last item
  5. Press the Duplicate button
  6. Verify that the Conflict dialog(check screenshot below) appears

Edit post with conflict

  1. Follow the steps from Duplicate post with conflict above
  2. Select Edit the post first
  3. Verify that the conflict resolution dialogs appear and the post opens for editing

Duplicate local post

  1. Follow the steps from Duplicate post with conflict above
  2. Select Copy the version from this app
  3. Verify that the editor opens
  4. Make a minor change
  5. Publish or exit to save the post as draft
  6. Verify that both the original and the duplicate post are listed

Screenshots

Published posts card list actions Draft posts list actions Conflict dialog
device-2020-12-03-132343 device-2020-12-03-132356 device-2020-12-03-151244

PR submission checklist:

  • I have considered adding unit tests where possible.
  • 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.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Dec 2, 2020

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@antonis antonis self-assigned this Dec 2, 2020
@antonis antonis added this to the 16.4 milestone Dec 2, 2020
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Dec 2, 2020

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

@antonis antonis force-pushed the issue/7954-Duplicate branch from d318090 to aa66603 Compare December 2, 2020 21:33
@antonis
Copy link
Contributor Author

antonis commented Dec 2, 2020

Hey @osullivanchris 👋
I've wrangled this issue during groundskeeping based on your design. Though the code is still draft the basic functionality has been implemented.
I'd appreciate any feedback on this 🙇

@antonis antonis added [Status] Needs Design Review A designer needs to sign off on the implemented design. [Status] Not Ready for Merge labels Dec 2, 2020
@osullivanchris
Copy link

@antonis Thanks for looking at this, and sharing the build! I tried it out and everything works as expected 😄

Just one note, per the discussion here we should change the label to 'duplicate' rather than 'copy' for consistency, as it sounds like we can't use 'copy' on iOS.

@antonis
Copy link
Contributor Author

antonis commented Dec 3, 2020

Thank you for the prompt response @osullivanchris 👍 I really appreciate this 🙇

Just one note, per the discussion here we should change the label to 'duplicate' rather than 'copy' for consistency, as it sounds like we can't use 'copy' on iOS.

Oh, I missed this 🤷 I'll change the label in the next build.

@antonis antonis changed the title Adds Duplicate/Copy post functionality Adds Duplicate post functionality Dec 3, 2020
@antonis antonis removed [Status] Needs Design Review A designer needs to sign off on the implemented design. [Status] Not Ready for Merge labels Dec 3, 2020
@antonis antonis changed the title Adds Duplicate post functionality Adds Duplicate/Copy post functionality Dec 3, 2020
@antonis antonis requested a review from malinajirka December 3, 2020 14:02
@antonis antonis marked this pull request as ready for review December 3, 2020 14:03
@malinajirka malinajirka self-assigned this Dec 3, 2020
@malinajirka
Copy link
Contributor

malinajirka commented Dec 3, 2020

Thanks @antonis! I run out of time today and I haven't finish the review yet. But I think I found an issue and I have one question.

Idea: Have you considered createing a copy of the post outside of the EditPostActivity (EPA). The EPA has quite a lot of responsibilities already. I'm wondering if we could create the copy of the post before we actually open the post in the EPA. It feels like creating the copy of the post isn't related to the editor anyway. We could write tests for what we want to and what we don't want to copy from the post (how to handle PostStatus etc). Wdyt?

Issue I encountered:
The app crashes with the following exception when I copy a post and I leave the editor without making any changes. It crashes only in debug builds, but we might still want to look into it. I think the issue is that we programatically set the title and content, which doesn't invoke autosave. When the user exits the editor we compare the state of the post when the EPA was created vs the current state (we are comparing an empty post with the post we programatically edited).

java.lang.RuntimeException: Debug build crash: Post had changes that needed to be saved when exiting the editor, and there was NOT an autosave pending. This means that the editor's autosave mechanism failed. This only crashes on debug builds.
        at org.wordpress.android.ui.posts.EditPostActivity.lambda$updateAndSavePostAsyncOnEditorExit$17$EditPostActivity(EditPostActivity.java:1784)
        at org.wordpress.android.ui.posts.-$$Lambda$EditPostActivity$9NVQt-kzRnzH2hHWsfH1Zj-h1kU.onPostUpdatedFromUI(Unknown Source:6)
        at org.wordpress.android.ui.posts.EditPostActivity.lambda$updateAndSavePostAsync$16(EditPostActivity.java:1749)
        at org.wordpress.android.ui.posts.-$$Lambda$EditPostActivity$qRWqo7UlqWexkHWD3rfBKjpicsg.invoke(Unknown Source:6)
        at org.wordpress.android.ui.posts.EditPostRepository$updateAsync$1.invokeSuspend(EditPostRepository.kt:128)

@antonis
Copy link
Contributor Author

antonis commented Dec 3, 2020

Thank you for the prompt response on this @malinajirka 🙇

Idea: Have you considered createing a copy of the post outside of the EditPostActivity (EPA). The EPA has quite a lot of responsibilities already. I'm wondering if we could create the copy of the post before we actually open the post in the EPA. It feels like creating the copy of the post isn't related to the editor anyway. We could write tests for what we want to and what we don't want to copy from the post (how to handle PostStatus etc). Wdyt?

That's a good idea 👍
I haven't actually looked this option. I'm seeing a jetpack-copy api call on the web. I will investigate this further since it will also resolve the reported Debug build crash.

@antonis antonis marked this pull request as ready for review December 4, 2020 15:05
@antonis
Copy link
Contributor Author

antonis commented Dec 4, 2020

Hey @malinajirka 👋

Idea: Have you considered createing a copy of the post outside of the EditPostActivity (EPA). The EPA has quite a lot of responsibilities already. I'm wondering if we could create the copy of the post before we actually open the post in the EPA. It feels like creating the copy of the post isn't related to the editor anyway. We could write tests for what we want to and what we don't want to copy from the post (how to handle PostStatus etc). Wdyt?

I changed the implementation based on your suggestion. It avoids adding even more complexity in the EPA and doesn't trigger the debug crash 👍
If/when you find some time please have a look 🙇

@mkevins
Copy link
Contributor

mkevins commented Dec 7, 2020

This is in good shape Antonis. I've tested the flows described, and it's mostly working as expected. I also ran the unit tests for PostListMainViewModelCopyPostTest and everything is passing. I was unable to run the tests for PostListMainViewModelCopyPostTest locally, because of a problem with AS 4.1 though.

Note: For the steps below I'm not sure how to consistently create conflicts for testing. Any input on this is welcome.

I've found that I could create conflicts by simultaneously editing a published post on both the web and the app, but the same "trick" did not work for a draft.

The one issue I noticed is that when a post is duplicated and closed without changes, the post list does not reflect the addition of the new post until the list is manually refreshed.

@antonis
Copy link
Contributor Author

antonis commented Dec 7, 2020

Thank you for taking the time to review this @mkevins 🙇

I've found that I could create conflicts by simultaneously editing a published post on both the web and the app, but the same "trick" did not work for a draft.

That's really helpful for testing. Thank you :)

The one issue I noticed is that when a post is duplicated and closed without changes, the post list does not reflect the addition of the new post until the list is manually refreshed.

Nice catch 👍
I've triggered a refresh of the posts list to fix this with f5ab73b

Copy link
Contributor

@mkevins mkevins left a comment

Choose a reason for hiding this comment

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

The new (unedited) duplicate post now shows up without manual refreshing. Nice work Antonis! LGTM!

@malinajirka
Copy link
Contributor

Thank you both! I have just quickly scanned through the changes and it looks great overall.

One thing we might want to consider is invoking something like this:
mDispatcher.dispatch(ListActionBuilder .newListRequiresRefreshAction(PostListDescriptor.calculateTypeIdentifier(postModel.getLocalSiteId())));
in the instantiatePostModel instead of manually refreshing the list on the client. Feel free to ignore this comment if you think it's not worth it or not a good idea.

@antonis
Copy link
Contributor Author

antonis commented Dec 8, 2020

Thank you both for taking the time to review this @malinajirka and @mkevins 🙇‍♂️

I agree with your suggestion @malinajirka and moved the list refresh to the FluxC project with 47d01d8 👍

This reverts commit f0147aa.
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.

4 participants