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

Adopt to the breaking changes in WordPressOrgRestApi #22612

Merged
merged 18 commits into from
Feb 22, 2024

Conversation

crazytonyli
Copy link
Contributor

@crazytonyli crazytonyli commented Feb 14, 2024

Description

WordPressOrgRestApi used to only support self-hosted sites. It's been rewritten to support self hosted sites and WP.com sites. This PR adopts this breaking changes.

The existing Blog.wordPressOrgRestApi returns a non-nil reference only when the blog is a self-hosted site, which is not obvious from the property name "wordPressOrgRestApi", because every WordPress site is bundled with .org rest API. I have renamed it to selfHostedSiteRestApi, especially considering WordPressOrgRestApi now supports all sites.

Test Instructions

There are only a number of places in the app that calls .org REST API. Editor settings API is one of them, which will be fired upon entering Pages List and opening a post. Plugin is another place that uses .org REST API, which is only accessible using self hosted site in the app.

You can put a breakpoint in BlockEditorSettingsService.fetchTheme function to see the API result. Or, you can use a proxy to see all 'wp/v2' HTTP requests.

Regression Notes

  1. Potential unintended areas of impact

All features that call .org REST API. i.e. plugins and gutenberg settings.

  1. What I did to test those areas of impact (or what existing automated tests I relied on)

Verified plugins and gutenberg settings API calls work.

  1. 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.

Testing checklist: N/A

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Feb 15, 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 Numberpr22612-3de5c2f
Version24.3
Bundle IDcom.jetpack.alpha
Commit3de5c2f
App Center Buildjetpack-installable-builds #7912
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 15, 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 Numberpr22612-3de5c2f
Version24.3
Bundle IDorg.wordpress.alpha
Commit3de5c2f
App Center BuildWPiOS - One-Offs #8882
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@crazytonyli crazytonyli self-assigned this Feb 16, 2024
@crazytonyli crazytonyli added this to the 24.3 milestone Feb 16, 2024
@crazytonyli crazytonyli marked this pull request as ready for review February 16, 2024 09:30
@mokagio mokagio mentioned this pull request Feb 19, 2024
4 tasks
@mokagio
Copy link
Contributor

mokagio commented Feb 19, 2024

One of the UI tests failures occurred in the Gallery block.

For some reason, the "Content Structure" that the test runner read returned 2 blocks instead of the expected 5. However, if you look at the test artifacts, you can see the 5 blocks on screen:

image

@mokagio
Copy link
Contributor

mokagio commented Feb 19, 2024

@crazytonyli I'm running out of time in my day. I was hoping to be able to fix the issue and review these changes, but I'm afraid I'll then have to rush through the rest of the 24.3 code freeze.

As such, I'll push this to the next milestone and do as you suggested and release a stable WordPressKit version that doesn't include the trunk changes this PR points to.

This will delay testing the new code IRL, but it will give us time to properly investigate all issues and review the PR.

Thanks 🙌

@crazytonyli
Copy link
Contributor Author

@mokagio That sounds good to me!

@crazytonyli
Copy link
Contributor Author

crazytonyli commented Feb 19, 2024

I have spent quite a while trying to compare the trunk version and this PR version of the app to understand what's the correct behaviour. But I probably don't know Gutenberg Editor enough to understand how to count blocks.

func testAddGalleryBlock() throws {
try BlockEditorScreen()
.enterTextInTitle(text: postTitle)
.addParagraphBlock(withText: postContent)
.addImageGallery()
.verifyContentStructure(blocks: 2, words: postContent.components(separatedBy: " ").count, characters: postContent.count)
}

However, I have perform the steps in this test case on the trunk branch on a freshly installed app, the number of blocks I get is 5, not 2 as asserted by the UI test.

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-02-19.at.19.54.08.mp4

But the UI test on the trunk branch passes, which means the UI test case is correct. Does that mean, we get different results on the actual app and the UI test app?

@wordpress-mobile/mobile-ui-testing-squad can you please help me to understand how to verify the correctness of testAddGalleryBlock?

@tiagomar
Copy link
Contributor

Hey @crazytonyli 👋

I couldn't investigate the block count failure in this branch because I haven't been able to build. Not sure if I'm missing something, but I get PluginStore build erros:

image

Does that mean, we get different results on the actual app and the UI test app?

Yeah, that's true for trunk right now and I don't know why it's different. 🤔
Looking into the Post's HTML version of similar posts from the JP and JP UI Tests apps we can spot the difference. While the images inside the Gallery block are represented as individual blocks on the JP app, there's only the Gallery block on JP UI Tests app. Please check the image below for reference.

image

A visual hint that it's really different is the focused block after the images insertions: a single image for the JP app and the Gallery block for JP UI Tests app.

image

I've tested it on a real device on 24.2 and 23.7 and in both cases the count was the same as in trunk, 5 blocks.

From the image in @mokagio's comment, the first image is in focus and the blocks count was 5 (which is in line with what we observe in real devices and for the JP app) while the UI Tests expected 2 blocks (1 Paragraph and 1 Gallery). So, I still don't understand why, but it looks like something in this branch migh have fixed the JP and JP Unit Tests discrepancy? 🤔 In this case we should just update the test to .verifyContentStructure(blocks: 5, words: postContent.components(separatedBy: " ").count, characters: postContent.count).

let remoteAPI: WordPressRestApi
if blog.isAccessibleThroughWPCom(),
blog.dotComID?.intValue != nil,
let restAPI = blog.wordPressComRestApi() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mokagio @tiagomar I think I have found the root cause of the testAddGalleryBlock failure: .org REST API calls stopped using API-Mocks in this PR. We used to use a WordPressComRestApi instance, which can be mocked by UI tests API-Mocks. But the new WordPressOrgRestApi doesn't support that. I'll just need to add that support back by allowing passing a custom "api base URL", which can be set to mock server url during UI tests.

Here are some details about the issue.

As @tiagomar has pointed out, the "html content" is different: one use images block and the other use a gallery block. This variable here dictates which block to use. It's part of a protocol defined in Gutenberg Mobile. One place, which appears to be the only place in UI tests, that makes Gutenberg Mobile calls this variable is successfully fetching and updating editor settings, which sends a .org REST API call. As I mentioned before, this PR breaks API mocks for .org REST API calls, which results in a fetching editor settings failure, which ultimately leads the editor produces a different html content.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

But the new WordPressOrgRestApi doesn't support that. I'll just need to add that support back by allowing passing a custom "api base URL", which can be set to mock server url during UI tests.

I think being able to customize the base URL is good design regardless, even if 99% of the usages will be with the default one configured in the library.

@dangermattic
Copy link
Collaborator

1 Warning
⚠️ This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

@crazytonyli
Copy link
Contributor Author

@tiagomar Going back to the discrepancy in Gutenberg Editor between using an actual site and the test site used in UI tests, I think it might be good to consider "updating" the test site to an more up-to-date version. The test site is on "5.2.2", which I believe is why it shows "2 blocks" in this particular test case. Changing it to "6.4" or something (I'm not sure if the rest of the site data needs to be changed too) probably would make Gutenberg Editor use 5 blocks like on a non-UI-test app.

@mokagio
Copy link
Contributor

mokagio commented Feb 22, 2024

I'm trying to see how many of the types in WordPressKit closer to the app domain I can move out of the library and into this repo, given WooCommerce doesn't directly depend on them, see woocommerce/woocommerce-ios#12063.

Having these changes merged will help keep my Git history tidy (tidier)

@mokagio mokagio merged commit 3ddebf7 into trunk Feb 22, 2024
21 checks passed
@mokagio mokagio deleted the wordpresskit-error-refactor branch February 22, 2024 03:42
crazytonyli added a commit that referenced this pull request Feb 26, 2024
The call was missed in #22612
@crazytonyli crazytonyli mentioned this pull request Feb 26, 2024
4 tasks
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

5 participants