Show a toast message (notice) when moving posts to trash #21724

crazytonyli · 2023-10-10T22:35:52Z

For context, this change is initiated from this conversation: p1694476848843439-slack-C0180B5PRJ4

Changes

This PR changes how to present an "undo" action to users when moving a post to trash. Here is a video preview of before & after the change.

Before (with an undo row)	After (with a toast message)
undo.mp4	toast.mp4

Problems of the "Undo" row

From design perspective, there are a couple of issues:

This feature here is the only one in the app that shows an "undo" row. In other actions in the posts list and other areas in the app, a toast message with an action button is commonly used.
After moving many posts to trash, the trashed posts' corresponding "undo" row has exactly the same appearance which is not intuitive to users when they do want to undo the previous trash action.

From implementation perspective, it's not straight-forward to implement keeping the trashed post position and placing a "undo" row in it. From a quick testing, the existing implementation has an obvious bug when trashing posts from bottom to up.

New flow

As you can see in the video, the new design is showing a toast message with an "Undo" button, which is consistent with other actions like restoring post from trash.

Implementation details

There are three parts involved in this PR

1. Removing the `restorableStatus` stored property from `AbstractPost`

This property is used to kept the original post status when it's moved to trash.

One obvious issue is, this property is not a "managed" property, which means it's not stored into the Core Data database. So, its value is guaranteed to be lost if multiple context objects are used to write and read this information.

Also, this property gives a false sense of a trashed post can be "restored" to any status, but I believe in reality a trashed post can only be restored to draft.

2. Use delete, trash, restore functions in `PostRepository`

These functions a newly implemented in PostRepository, as replacements for the ones in PostService which are now deleted in this PR.

3. Show toast messages after trashing posts

This is the UI change part of this PR.

Test Instructions

Here are a few things that I can think of:

Verify the "Undo" button can restore the trashed post as draft.
Verify that there is no side-effect in perform "Move to trash" action multiple times on the same post.
Verify trashing many posts at the same time works.

Regression Notes

Potential unintended areas of impact
None.
What I did to test those areas of impact (or what existing automated tests I relied on)
What's described in the "Test Instructions" section.
What automated tests I added (or what prevented me from doing so)
None.

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.

UI Changes testing checklist:

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)

wpmobilebot · 2023-10-10T23:02:38Z

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

	App Name	WordPress Alpha
	Configuration	Release-Alpha
	Build Number	pr21724-97ff26c
	Version	23.5
	Bundle ID	org.wordpress.alpha
	Commit	`97ff26c`
	App Center Build	WPiOS - One-Offs #7570

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

wpmobilebot · 2023-10-10T23:06:06Z

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

	App Name	Jetpack Alpha
	Configuration	Release-Alpha
	Build Number	pr21724-97ff26c
	Version	23.5
	Bundle ID	com.jetpack.alpha
	Commit	`97ff26c`
	App Center Build	jetpack-installable-builds #6599

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

peril-wordpress-mobile · 2023-10-11T00:21:28Z

	Warnings
⚠️	This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

mokagio

Left a few questions.

mokagio · 2023-10-13T02:02:50Z

WordPress/Classes/Services/PostRepository.swift


+        let status = try await coreDataStack.performQuery { try $0.existingObject(with: postID).status }
+        assert(status == .trash, "This function can only be used to delete trashed posts/pages.")
+


At first I was surprised to see the assert here and not at the start of the method. But I guess the reason is that the code above recur to find the original post, and for the same reason we want to delete the original, we want to check the status on it. Am I reading this right?

Yes. This status check doesn't exist in the original delete function in PostService. I added it here to catch incorrect calls to this function (which may never happen).

mokagio · 2023-10-13T02:06:30Z

WordPress/Classes/ViewRelated/Post/AbstractPostListViewController.swift

-        let postObjectID = apost.objectID
+    @MainActor
+    func deletePost(_ apost: AbstractPost) async {
+        assert(apost.managedObjectContext == ContextManager.shared.mainContext)


Out of scope: I've been thinking about all the accesses to ContextManager.shared that we have and the issues they sometime cause such as #21742.

Would it make sense to instantiate the view controller with a context or CoreDataStack object? In practice, this would be ContextManager.shared.mainContext or ContextManager.shared but the separation would:

Centralize the definition of the shared resource access

Allow us to replace it in the tests

What do you think?

mokagio · 2023-10-13T02:09:10Z

WordPress/Classes/ViewRelated/Post/AbstractPostListViewController.swift

+            let message = NSLocalizedString("Post moved to trash.", comment: "A short message explaining that a post was moved to the trash bin.")
+            let undoAction = NSLocalizedString("Undo", comment: "The title of an 'undo' button. Tapping the button moves a trashed post out of the trash folder.")


This would be a good time to introduce reverse-DNS keys in these localized strings. For context: #19028

These strings were copied from https://github.com/wordpress-mobile/WordPress-iOS/pull/21724/files/aedccc4558780dbd690d4e2f586536f1b0ea8a97#diff-0b401d58526d69f6fa54a6679880b35a5fc7bf7643731e9c55f5d4b7b56f8d63L34-L35. I have updated them in 58c1d44

mokagio · 2023-10-13T02:12:58Z

WordPress/Classes/ViewRelated/Post/AbstractPostListViewController.swift

            }
-        })
+        }


Have you considered moving some of the logic here into a ViewModel-like object, so that we can test it?

One point of friction I can see is that there is no such object for this VC that I can see at the moment. Another is that this is a base class, meaning that the actual subclasses the app uses at runtime might need additional behavior and might be left with having to create their own ViewModel subclasses. This would be confusing. At the same time, it would be a sign of an abstraction that no longer serves us well and we might want to update its design.

That'd be much better than this view controller's current state. But I don't want to just move this small part (trashing, deleting, restoring) out, leaving this view controller in a weird state with mixed-patterns. A large refactor on this screen also seems unnecessary at this point.

leaving this view controller in a weird state with mixed-patterns

Good point 🤔

I am a fan of sprouting (a la Working Effectively with Legacy Code) better code from existing code, even if what is added/extracted is a little portion of the total. But ☝️ having mixed patterns is a concern.

I guess it would be acceptable only if we had an agreed upon expectation that "all new and modified code should follow ." That is not the case at the moment.

mokagio

Given this PR has been sitting idle waiting for my feedback, the least I could do was fixing the merge conflict (on RELEASE-NOTES.txt) to unblock it's merge

crazytonyli added 7 commits October 11, 2023 09:58

Store original status of trashed posts in view controller

a31b199

Remove restorableStatus property from AbstractPost

a378ddc

Add an error log for an practically impossible case

db03329

Remove the restore page/posts row

f12086f

Show notice when moving a post/page to draft

746f4ca

Use the trash and restore function in PostRepository

ec47672

Fix unit tests

cfa63e2

crazytonyli requested a review from mokagio October 11, 2023 00:21

crazytonyli self-assigned this Oct 11, 2023

crazytonyli added the Posts + Pages label Oct 11, 2023

crazytonyli added this to the 23.5 milestone Oct 11, 2023

crazytonyli marked this pull request as ready for review October 11, 2023 00:21

Update release notes

aedccc4

mokagio reviewed Oct 13, 2023

View reviewed changes

Use reverse DNS domain strings

58c1d44

crazytonyli modified the milestones: 23.5, 23.6 Oct 15, 2023

crazytonyli and others added 3 commits October 16, 2023 12:54

Merge branch 'trunk' into tonyli-refactor-restore-post

2846f7a

Move a release note to the v23.6 section

501d09b

Merge 'origin/trunk' into tonyli-refactor-restore-post

97ff26c

mokagio approved these changes Oct 27, 2023

View reviewed changes

mokagio enabled auto-merge October 27, 2023 01:35

mokagio merged commit 1be66fc into trunk Oct 27, 2023

mokagio deleted the tonyli-refactor-restore-post branch October 27, 2023 02:19

This was referenced Oct 30, 2023

Posts & Pages: Remove "Undo" funtionality #21713

Closed

Post & Page Lists: It can be unclear which "undo" view is for which trashed post. #4185

Closed

Post deletion / undoing in offline mode is not working right #5811

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show a toast message (notice) when moving posts to trash #21724

Show a toast message (notice) when moving posts to trash #21724

crazytonyli commented Oct 10, 2023 •

edited

Loading

wpmobilebot commented Oct 10, 2023 •

edited

Loading

wpmobilebot commented Oct 10, 2023 •

edited

Loading

peril-wordpress-mobile bot commented Oct 11, 2023 •

edited

Loading

mokagio left a comment

mokagio Oct 13, 2023

crazytonyli Oct 15, 2023

mokagio Oct 27, 2023

mokagio Oct 13, 2023

mokagio Oct 13, 2023

crazytonyli Oct 15, 2023

mokagio Oct 27, 2023

mokagio Oct 13, 2023

crazytonyli Oct 15, 2023

mokagio Oct 27, 2023

mokagio left a comment


		let status = try await coreDataStack.performQuery { try $0.existingObject(with: postID).status }
		assert(status == .trash, "This function can only be used to delete trashed posts/pages.")

		let message = NSLocalizedString("Post moved to trash.", comment: "A short message explaining that a post was moved to the trash bin.")
		let undoAction = NSLocalizedString("Undo", comment: "The title of an 'undo' button. Tapping the button moves a trashed post out of the trash folder.")

Show a toast message (notice) when moving posts to trash #21724

Show a toast message (notice) when moving posts to trash #21724

Conversation

crazytonyli commented Oct 10, 2023 • edited Loading

Changes

Problems of the "Undo" row

New flow

Implementation details

1. Removing the restorableStatus stored property from AbstractPost

2. Use delete, trash, restore functions in PostRepository

3. Show toast messages after trashing posts

Test Instructions

Regression Notes

wpmobilebot commented Oct 10, 2023 • edited Loading

wpmobilebot commented Oct 10, 2023 • edited Loading

peril-wordpress-mobile bot commented Oct 11, 2023 • edited Loading

mokagio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mokagio left a comment

Choose a reason for hiding this comment

crazytonyli commented Oct 10, 2023 •

edited

Loading

1. Removing the `restorableStatus` stored property from `AbstractPost`

2. Use delete, trash, restore functions in `PostRepository`

wpmobilebot commented Oct 10, 2023 •

edited

Loading

wpmobilebot commented Oct 10, 2023 •

edited

Loading

peril-wordpress-mobile bot commented Oct 11, 2023 •

edited

Loading