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: Trash, Delete, Restore #22947

Merged

Conversation

kean
Copy link
Contributor

@kean kean commented Apr 3, 2024

Adds support for trash, delete, and restore (existing move to draft).

When you trash a post, it deletes your local unsynced changes after a confirmation, sends a patch, and moves it to trash. Invariant: all posts in the "Trash" tab have no unsynced changes.

When you delete a post, it uses a new endpoint.

To test:

  • Verify that when you trash a post, the cells becomes grayed-out (takes precedence over "has unsynced changes" and other states)
  • Verify that when you trash a post, it sends only { "status": "trash" } in the request
  • Verify that if you trash a post while the sync request is running, it waits until the request completes
  • Verify that when you trash a post, it deletes all the local unsynced changes after a confirmatin
  • Verify that if you trash a post that was permanently deleted, it deletes it form the list (known issue: API returns 403 instead of 404) (the production version also shows an error; so it's not a regression)
  • Verify that you can still trash a draft or a pending post without a confirmation (unless they have local changes)
  • Verifythat if you permanently delete and already permanently deleted post, it succeeds (the production version shows an error)

Regression Notes

  1. Potential unintended areas of impact: Post List
  2. What I did to test those areas of impact (or what existing automated tests I relied on): manual
  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.

Testing checklist:

  • WordPress.com sites and self-hosted Jetpack sites.
  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • VoiceOver.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • iPhone and iPad.
  • Multi-tasking: Split view and Slide over. (iPad)

@kean kean requested a review from momo-ozawa April 3, 2024 15:07
@kean kean added this to the 24.7 milestone Apr 3, 2024
/// - Parameters:
/// - postID: Object ID of the given post
/// - status: The post's original status before it's moved to the trash bin.
func restore<P: AbstractPost>(_ postID: TaggedManagedObjectID<P>, to status: BasePost.Status) async throws {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no need for a separate restore. It's "Move to draft" and in terms of the implementation it's "patch" with { "status": "draft" }.

@@ -642,6 +642,47 @@ class AbstractPostListViewController: UIViewController,
navigationController?.present(navWrapper, animated: true)
}

func _trash(_ post: AbstractPost, completion: @escaping () -> Void) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The implementation is now shared between posts and pages.
I thought it would also be clearer if there was a separate .delete action, so I added it.

@kean kean force-pushed the task/offline-mode-delete-trash branch from e96a79d to e571240 Compare April 3, 2024 16:32
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Apr 3, 2024

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 Numberpr22947-cac27ff
Version24.5
Bundle IDorg.wordpress.alpha
Commitcac27ff
App Center BuildWPiOS - One-Offs #9410
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Apr 3, 2024

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 Numberpr22947-cac27ff
Version24.5
Bundle IDcom.jetpack.alpha
Commitcac27ff
App Center Buildjetpack-installable-builds #8453
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

Copy link
Contributor

@momo-ozawa momo-ozawa left a comment

Choose a reason for hiding this comment

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

Works as expected, except for this flow:

Verify that if you trash a post that was permanently deleted, it deletes it form the list

I get an error "User cannot delete this post"

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-04-04.at.16.23.38.mp4

Base automatically changed from task/offline-update-remaining-post-coordinator-usages to feature/offline-mode-milesone-2 April 5, 2024 17:51
@kean
Copy link
Contributor Author

kean commented Apr 5, 2024

I get an error "User cannot delete this post"

I reported in it our channel and asked Enej to help look into it.

I also compared it with wp.com and it uses the /delete path for moving posts to trash, which returns 404 in this scenario – as expected. But /delete is also use for permanent deletion. Using it for both operations is extremely unsafe as you might end up unintentionally permanently deleting a post that the frontend thinks it's moving to trash. Out of these two options, I would pick the existing behavior in the app.

I also totally forgot the following scenario that is now fixed:

  1. Open the post list in the app
  2. Permanently delete the post on the web
  3. Refresh the list in the app
  4. Verify that the post was removed from the list

@kean kean requested a review from momo-ozawa April 5, 2024 19:41
@momo-ozawa
Copy link
Contributor

Out of these two options, I would pick the existing behavior in the app.

Sounds good.

I also totally forgot the following scenario that is now fixed:

Open the post list in the app
Permanently delete the post on the web
Refresh the list in the app
Verify that the post was removed from the list

Tested - works as described!

@kean kean merged commit 2f28218 into feature/offline-mode-milesone-2 Apr 8, 2024
21 of 24 checks passed
@kean kean deleted the task/offline-mode-delete-trash branch April 8, 2024 12:38
@kean kean mentioned this pull request Apr 10, 2024
14 tasks
@momo-ozawa momo-ozawa modified the milestones: 24.7, Pending, 24.8 Apr 15, 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.

3 participants