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: Sync Publishing #22689

Merged
merged 10 commits into from
Mar 5, 2024
Merged

Offline Mode: Sync Publishing #22689

merged 10 commits into from
Mar 5, 2024

Conversation

kean
Copy link
Contributor

@kean kean commented Feb 23, 2024

Part of #22579

To test:

Important

Make sure to enable the .offlineMode feature flag.

  • Tap "+" to create a new post
  • Add content and tap "Publish"
  • Verify that the pre-publishing sheet is shown
  • Tap "Publish"
  • Verify that that sheet is disabled and can't be dismissed during publishing
  • Verify that if request fails, a snackbar is shown (note: it should not have a "Retry" button)
  • Verify that if request succeed, the editor is dismissed

This is the only scenario that is now officially supported. We'll need more tests, including automated test in the future PRs.

Regression Notes

  1. Potential unintended areas of impact: Publishing
  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)

@@ -35,6 +35,8 @@ NS_ASSUME_NONNULL_BEGIN

/**
BOOL flag set to true if the post is first time published.

- note: Deprecated (kahu-offline-mode)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suggest using this marker for anything that's scheduled for removal once the project is live. I'm not "officially"deprecating these APIs to avoid polluting the project with warnings.

///
/// - warning: Work-in-progress (kahu-offline-mode)
@MainActor
func _publish(_ post: AbstractPost) 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.

I'm underscoring the methods that are work-in-progress and are only designed to be used in the scope of the project. It also helps avoid the name conflicts.

notifyNewPostPublished()
}
SearchManager.shared.indexItem(post)
AppRatingUtility.shared.incrementSignificantEvent()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Everything non-editor related is now handled by PostCoordinator to allow you to safely publish outside of the editor.

Noting that it no longer sets post.shouldAttemptAutoUpload = true and no longer uses isFirstTimePublish which should never have been part of the model layer.

///
/// - warning: Work-in-progress (kahu-offline-mode)
@MainActor
func _upload(_ parameters: RemotePost, for post: AbstractPost) 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.

parameters: RemotePost – this will definitely change, but the idea is that by bringing back synchronous operations, we need a way to:

  • Send the request with new parameters without updating the database to make sure there is nothing to revert if it fails
  • Be able to send only what's changes and that's coming in the next scopes

post = original
}
PostHelper.update(post, with: uploadedPost, in: context)
PostService(managedObjectContext: context)
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 method, including updateMediaFor is pretty much one-to-one replacement to the Objective-C version from PostService that Tony replaced with PostRepository.

updateMediaFor is an odd one. It set the postID for assets, but it makes little scene because asset->post is a one-to-many relationship.

let viewController = PrepublishingViewController(post: post, identifiers: prepublishingIdentifiers) { [weak self] result in
switch result {
case .completed(let post):
self?.post = post
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 was a no-op because the post instance never changes.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Feb 23, 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 Numberpr22689-5205217
Version24.3
Bundle IDcom.jetpack.alpha
Commit5205217
App Center Buildjetpack-installable-builds #8105
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 Feb 23, 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 Numberpr22689-5205217
Version24.3
Bundle IDorg.wordpress.alpha
Commit5205217
App Center BuildWPiOS - One-Offs #9062
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@kean kean force-pushed the task/sync-publishing-p2 branch from a83b389 to 0a155d2 Compare February 23, 2024 02:06
@kean kean force-pushed the task/sync-publishing-p2 branch from 0a155d2 to 8047234 Compare February 23, 2024 18:56
@kean kean requested a review from momo-ozawa February 23, 2024 19:01
@kean kean marked this pull request as ready for review February 23, 2024 19:02
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 described!

I wonder if we should continue showing the success notice message when creating a post/page from the FAB. IMO I don't think the implicit indication of closing the editor is enough - I'd expect to see some explicit indication that the post was uploaded (if I'm on the My Site screen).

sync-publishing-from-fab.mp4

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.

Approving to unblock - lmk what you think of adding an explicit message to acknowledge a successful upload for the FAB flow.

@kean
Copy link
Contributor Author

kean commented Mar 4, 2024

Test cases for notifications:

Success (Notice)

  • Publish a post
  • Verify that a success notice is shown with a "View" button
  • Tap "View" and verify that the success screen is shown

Success (Local Notification)

  • Tap "Publish" and quickly close the app
  • Verify that a success local notification is shown
  • Note: tapping on it just opens the post list

Failure (Notice)

  • When connection is down
  • Tap "Publish"
  • Verify that a failure notice is shown with no buttons

Failure (Local Notification)

  • When connection is down
  • Tap "Publish" and quickly close the app
  • Verify that a failure local notification is shown

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.

Test cases work as described

@kean kean modified the milestones: Pending, 24.6 Mar 5, 2024
@kean kean merged commit a66adde into trunk Mar 5, 2024
23 of 24 checks passed
@kean kean deleted the task/sync-publishing-p2 branch March 5, 2024 13:22
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