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

[RNMobile] Sync VideoPress metadata when post is manually saved #30131

Merged
merged 19 commits into from
Apr 20, 2023

Conversation

SiobhyB
Copy link

@SiobhyB SiobhyB commented Apr 17, 2023

Fixes wordpress-mobile/gutenberg-mobile#5666

Related PRs

Proposed changes:

Other information:

  • Have you written new tests for your changes, if applicable?
  • Have you checked the E2E test CI results, and verified that your changes do not break them?
  • Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

p9ugOq-3mV-p2

Does this pull request change what data or activity we track or use?

No, it doesn't.

Testing instructions:

  • In the app, either add a new VideoPress block or navigate to an existing one.
  • Tweak the block's settings. It's particularly important to change the settings in the Details and Privacy and Rating panels, as these are the settings that are not currently being synced as expected.
  • Manually save your changes in the app. You can find different flows for manually saving on both Android and iOS below.
  • Confirm that you see the expected logs in the console, showing that the settings have been synced.
  • Navigate to the block on the web and verify that the block's settings have been correctly synced there.
  • Go through a few tries of editing the block on the app then returning the web, and vice versa, to ensure syncing is working as expected.

Android flows

Flows for saving a draft ⤵️
  • Tap the three dots to the upper right and then Save.
  • Tap the back button.
  • Tap the Publish button.
  • The the three dots to the upper right to get to Post Settings. From here, set the post's publish date and then Schedule.
Flows for saving a published post ⤵️
  • Tap the Update button.
  • Tap the back button.
  • The the three dots to the upper right to get to Post Settings. From here, change the post's publish date and then Schedule.
Flows for saving a scheduled post ⤵️
  • Tap the Schedule button.
  • Tap the back button.
  • The the three dots to the upper right to get to Post Settings. From here, change the post's publish date and then Schedule.

iOS flows

Flows for saving a draft ⤵️
  • Tap the Update button.
  • Tap the X icon to exit the post and Update Draft.
  • Tap the three dots to the upper right and then Publish.
  • The the three dots to the upper right to get to Post Settings. From here, set the post's publish date and then Schedule.
Flows for saving a published post ⤵️
  • Tap the Update button.
  • Tap the X icon to exit the post and Update Post.
  • The the three dots to the upper right to get to Post Settings. From here, set the post's publish date and then Schedule.
Flows for saving a scheduled post ⤵️
  • Tap the Update button.
  • Tap the X icon to exit the post and Update Post.
  • The the three dots to the upper right to get to Post Settings. From here, set the post's publish date and then Schedule.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 17, 2023

Are you an Automattician? You can now test your Pull Request on WordPress.com. On your sandbox, run

bin/jetpack-downloader test jetpack rnmobile/detect-when-post-is-saved

to get started. More details: p9dueE-5Nn-p2

@github-actions
Copy link
Contributor

github-actions bot commented Apr 17, 2023

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ✅ All commits were linted before commit.
  • ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖


The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.


Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

@SiobhyB SiobhyB changed the base branch from trunk to rnmobile/fix/stop-saving-html-markup April 17, 2023 18:59
@github-actions github-actions bot added [Focus] Compatibility Ensuring our products play well with third-parties [Package] Admin Ui [Package] Jetpack mu wpcom WordPress.com Features [Plugin] Backup A plugin that allows users to save every change and get back online quickly with one-click restores. [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Plugin] mu wpcom jetpack-mu-wpcom plugin [Tests] Includes Tests [Tools] Development CLI The tools/cli to assist during JP development. labels Apr 17, 2023
@SiobhyB SiobhyB changed the base branch from rnmobile/fix/stop-saving-html-markup to trunk April 17, 2023 20:04
@SiobhyB SiobhyB removed [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Plugin] Backup A plugin that allows users to save every change and get back online quickly with one-click restores. [Focus] Compatibility Ensuring our products play well with third-parties [Package] Admin Ui [Package] Jetpack mu wpcom WordPress.com Features [Tools] Development CLI The tools/cli to assist during JP development. labels Apr 17, 2023
Copy link
Author

Choose a reason for hiding this comment

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

These changes are brought in from #30134, in order to avoid a regression that prevented this PR from being built. #30134 should be merged before this one.

Copy link
Contributor

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

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

Tested different flows in both platforms and confirmed that VideoPress metadata is saved in the below cases:

iOS:

  • Published post - Tap Update button and then close the editor. ✅
  • Published post - Close editor and save unsaved changes. ✅
  • Draft post - Tap Update button and then close the editor. ✅
  • Draft post - Close editor and save unsaved changes. ✅
  • Draft post - Tap Three dots button/Publish and open the post again. ✅
  • Scheduled post - Tap Update button and then close the editor. ✅
  • Scheduled post - Close the editor and save unsaved changes. ✅

Android:

  • Published post - Tap Update button and open the post again. ✅
  • Published post - Close the editor with unsaved changes. ✅
  • Published post - Tap Schedule button and open the post again. ✅
  • Draft post - Tap Three dots button/Save and open the post again. ✅
  • Draft post - Close the editor and save unsaved changes. ✅
  • Draft post - Tap Publish and open the post again. ✅
  • Scheduled post - Tap Schedule and open the post again. ✅
  • Scheduled post - Close the editor and save unsaved changes. ✅
  • Scheduled post - Tap Publish Now and open the post again. ✅

However, I noticed an issue when previewing a post or switching to HTML mode. It seems unrelated to these changes, so I wouldn't block the PR, but we'd need to track it.

Preview a post

  1. Having a VideoPress block with a video.
  2. Change some of the settings that will be synced (e.g. title).
  3. Tap on the three dots button.
  4. Tap on Preview.
  5. Close the preview.
  6. Open the block settings and observe that the settings changes were discarded.

Switch to HTML mode

  1. Having a VideoPress block with a video.
  2. Change some of the settings that will be synced (e.g. title).
  3. Tap on the three dots button.
  4. Tap on HTML Mode.
  5. Switch back to Visual Mode.
  6. Open the block settings and observe that the settings changes were discarded.

Apart from the above-unrelated issue, the PR looks great to me. I only added a comment that would be great to tackle before approving it.

@SiobhyB
Copy link
Author

SiobhyB commented Apr 18, 2023

@fluiddot, thank you for catching those issues with preview and toggle mode! I've gone ahead to create an issue in wordpress-mobile/gutenberg-mobile#5677 and have added it to our project board. 🙇‍♀️

@SiobhyB SiobhyB requested a review from fluiddot April 18, 2023 13:59
Copy link
Contributor

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

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

I re-tested some of the cases shared in my last review. However, I'm trying to test the web version but I'm having trouble to have the VideoPress block registered 🤔.

Copy link
Contributor

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

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

As internally discussed (p1681833498706689/1681831906.251289-slack-C03TA48NSUX), we'd need to move this import to a native file because the package is only available in the native version.

@SiobhyB
Copy link
Author

SiobhyB commented Apr 19, 2023

I've gone ahead to move the native functionality to a new .native.js file. Note, I opted to go with js file as we're making some changes to the web's types. In addition, although we're not currently using any strings in this file on native, the fact we can't import translations from .ts files swayed my decision. It felt that using a .js version would be the most straightforward and future-proof approach. Open to different opinions, though.

The removal of native-specific functionality from the main file can be found in f050d65. For the new native file, it was possible to remove a bunch of (currently) unused functionality, as shown in 321d343.

@SiobhyB SiobhyB requested a review from fluiddot April 19, 2023 12:03
Copy link
Contributor

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

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

LGTM 🎊 !

I tested the web version and worked as expected ✅ . Nevertheless, it would be great to request a second review from web folks to confirm no regressions are introduced.

Regarding the native version, I re-tested some of the cases shared here in both platforms and worked as expected ✅.

Copy link
Contributor

@dhasilva dhasilva left a comment

Choose a reason for hiding this comment

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

No regressions on web version. Nice work!

@bindlegirl bindlegirl added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Apr 20, 2023
@SiobhyB SiobhyB merged commit e6e36ae into trunk Apr 20, 2023
@SiobhyB SiobhyB deleted the rnmobile/detect-when-post-is-saved branch April 20, 2023 16:20
@zinigor zinigor added this to the Jetpack/12.1 milestone Apr 24, 2023
@kraftbj kraftbj removed the [Status] Ready to Merge Go ahead, you can push that green button! label Nov 17, 2023
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.

VideoPress block: Limit synchronization of block attributes to when post has just been saved
6 participants