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: Improve Post Settings #22863

Closed

Conversation

kean
Copy link
Contributor

@kean kean commented Mar 19, 2024

This PR improves Post Settings in multiple ways by introducing a child context as a scratchpad for changes. The only complication is due to the relationships. For example, when you add a category, its added in the viewContext, so we have to get the same object in the scratchpad.

Known Issues

In this PR, I want to make sure we get the bigger ideas right. There are a couple of known issues:

  • publicizeMessage and social integration not encoded in the patch parameters
  • Assertion when using "password protected" status from the standalone settings screen
  • Removing feature image doesn't work (needs to be set to "" instead of null)

I have a list of those and I suggest not focusing on the individual fields for now. We'll do a full test before merging the feature branch.

Changes

  • Update the navigation bar style in the Post Settings in the Editor
  • Add “Cancel” button to allow undoing the changes – extra peace of mind. Thanks to a child context, the app no longer needs to do anything to undo the changes.
  • "Save" button will now work based on the diff – same as in the standalone editor
  • Fix an issue with “Cancel” button being enable during save (production issue)
  • Remove "Done" button from the Tags screen (same behavior as back)
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-03-19.at.16.51.01.mp4

To test:

  • Verify that adding new categories works (important because blog still operates on viewContext, so it was a bit tricky to support): you can select new categories, add new categories, and the categories are pre-selected after re-opening the picker
  • Verify that passport-protected visibility does not crash in the standalone editor
  • Verify that you can cancel any change
  • Verify that "Save" is enabled only if there are any changes (undoing a change will disable the button)
  • Verify that if you dismiss the editor using a swipe gesture, the changes are not saved
  • Verify that if you open Post Settings from the Editor, make changes, and tap "Save", the changes are saved to the database (check by re-opening the settings)
  • Disable .syncPublishing feature flag and perform regression test of the settings

Known Issues

  • I'm planning to change how status updates from the Post Settings are handled in the upcoming PR

Regression Notes

  1. Potential unintended areas of impact: n/a
  2. What I did to test those areas of impact (or what existing automated tests I relied on): n/a
  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 a team as a code owner March 19, 2024 17:38
@kean kean added this to the Pending milestone Mar 19, 2024
@kean kean requested a review from momo-ozawa March 19, 2024 17:38
@kean kean mentioned this pull request Mar 19, 2024
4 tasks
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Mar 19, 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 Numberpr22863-cf313c6
Version24.5
Bundle IDorg.wordpress.alpha
Commitcf313c6
App Center BuildWPiOS - One-Offs #9274
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 Mar 19, 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 Numberpr22863-cf313c6
Version24.5
Bundle IDcom.jetpack.alpha
Commitcf313c6
App Center Buildjetpack-installable-builds #8318
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@kean kean marked this pull request as draft March 19, 2024 20:33
@kean kean removed the request for review from momo-ozawa March 19, 2024 20:33
@tiagomar
Copy link
Contributor

Hey @kean 👋
You're probably aware, but I was taking a look at the UI Tests failures and noticed they caught a crash while setting the Featured Image. The app consistently crashes right after selecting the image, I've tried:

  • Choose from Device
  • Take Photo
  • Choose from Media

@kean
Copy link
Contributor Author

kean commented Mar 19, 2024

Hey, @tiagomar. Yeah, I've noticed it too. I moved the PR to draft and will try a slightly different approach tomorrow.

Btw, there are 4-6 UI tests we are planning to add in the scope of the project.

@kean
Copy link
Contributor Author

kean commented Mar 20, 2024

We'll keep this change out of the scope for now. It's too risky, it deviates from the existing behavior too much, and is not necessarily required to fix the sync issues.

@kean kean closed this Mar 20, 2024
@kean kean reopened this Mar 21, 2024
@kean kean removed the request for review from a team March 21, 2024 23:45
@kean kean force-pushed the task/simplify-post-settings branch from 3678f3a to b9e7b95 Compare March 21, 2024 23:45
@kean kean marked this pull request as ready for review March 22, 2024 00:31
@kean kean requested a review from momo-ozawa March 22, 2024 00:31
@kean kean force-pushed the task/simplify-post-settings branch from b9e7b95 to cf313c6 Compare March 22, 2024 00:32
///
/// - warning: Work-in-progress (kahu-offline-mode)
@MainActor
func _update(_ post: AbstractPost, changes: RemotePostUpdateParameters) 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.

This is the simplest possible form of updating the post: send diff, update the original post with RemotePost from the server. Keeps the local revision intact. It'll be required in a few places when working with published posts.

@@ -73,12 +73,12 @@ extension PostSettingsViewController {
guard let media = MediaCoordinator.shared.addMedia(from: asset, to: apost) else {
return
}
self.apost.featuredImage = media
self.apost.featuredImage = try? self.apost.managedObjectContext?.existingObject(with: TaggedManagedObjectID(media))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what I referred to in the description. When you add a featuredImage, it gets added to viewContext, so we have to get the instance in the temporary child context. The child context also automatically merges the changes from viewContext to allow the screen to show the updated status of the Media asset. This is probably the only complication with child contexts.


/// - note: Deprecated (kahu-offline-mode)
private func _buttonSaveTapped() {
saveChangesToParentContext()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's slightly different from the previous production behavior – it's now fully offline-first and it should be OK. It means that the "Save" is now also offline-first. I hope we don't even end up shipping this and be ready for 24.6 release.

if (self.isStandalone) {
if ((self.isBeingDismissed || self.parentViewController.isBeingDismissed) && !self.isStandaloneEditorDismissingAfterSave) {
// TODO: Implement it using a ViewModel or a child context to eliminate the risk of accidently saving the changes without uploading them
[self.apost.managedObjectContext refreshObject:self.apost mergeChanges:NO];
Copy link
Contributor Author

@kean kean Mar 22, 2024

Choose a reason for hiding this comment

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

To discard the changes, you no longer need to do anything thanks to the child context. This is probably why it's really worth it – we no longer have to worry about accidentally saving the changes to the database without committing them to the server. This will be used in the upcoming changes.

@kean kean removed the request for review from momo-ozawa March 22, 2024 21:25
@kean kean marked this pull request as draft March 22, 2024 21:25
@kean kean marked this pull request as ready for review March 26, 2024 23:43
@kean kean requested a review from momo-ozawa March 26, 2024 23:43
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.

@kean 👋 I tested the flows and found a few issues.

When I make changes to a post setting and go back to the editor, the "Update" button is disabled. Tapping the more button enables the "Update" button.

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-03-28.at.14.19.50.mp4

Verify that password-protected visibility does not crash in the standalone editor

If I access post settings from the post list context menu, this flow crashes.

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-03-28.at.14.25.50.mp4

If I access post settings from the editor, change to password protected visibility, I am not able to update my post with the new post settings.

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-03-28.at.14.23.28.mp4

Verify that "Save" is enabled only if there are any changes (undoing a change will disable the button)

If I undo a change for tags, the "Save" button is not disabled.

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-03-28.at.14.20.16.mp4

@kean
Copy link
Contributor Author

kean commented Mar 28, 2024

When I make changes to a post setting and go back to the editor, the "Update" button is disabled. Tapping the more button enables the "Update" button.

It's a production issue. It's on my stretch-task list.

This has the same root cause:

If I access post settings from the editor, change to password protected visibility, I am not able to update my post with the new post settings.

If I access post settings from the post list context menu, this flow crashes.

I initially put it on the list of tests, but forgot to remove it once I decided not to fix it just yet. It's anther production issue. It's an assertion, so it doest' crash in production, but does in the debug mode.

If I undo a change for tags, the "Save" button is not disabled.

Looking into it – I'll push a fix.

@kean
Copy link
Contributor Author

kean commented Mar 28, 2024

If I undo a change for tags, the "Save" button is not disabled.

Hmm, I can't reproduce it. I'll put in on the list to check and fix later.

@kean kean requested a review from momo-ozawa April 1, 2024 14:28
@kean kean marked this pull request as draft April 1, 2024 16:52
@kean
Copy link
Contributor Author

kean commented Apr 1, 2024

Closed in favor of #22934

@kean kean closed this Apr 1, 2024
@momo-ozawa momo-ozawa modified the milestones: Pending, 24.7 Apr 15, 2024
@jkmassel jkmassel deleted the task/simplify-post-settings branch July 26, 2024 18:21
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.

None yet

4 participants