Skip to content

[REST API] Merge WordPressOrgRequest into RESTRequest #8553

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Jan 6, 2023
Merged

[REST API] Merge WordPressOrgRequest into RESTRequest #8553

merged 7 commits into from
Jan 6, 2023

Conversation

@selanthiraiyan
Copy link
Contributor

@selanthiraiyan selanthiraiyan commented Jan 5, 2023

Closes: #8512

Description

WordPressOrgRequest and RESTRequest have similar properties. For clarity we decided to merge them both and use
RESTRequest.

Testing instructions

  • We need to test the areas in which WordPressOrgRequest (deleted in this PR) and RESTRequest are being used.
  • Create a test store using JN. (without Jetpack)

Jetpack installation

  • Log out of the app or skip login onboarding if needed.
  • On the prologue screen, select "Log In" or "Continue with WordPress.com" based on the A/B test variant you get.
  • Log in with your WordPress.com account.
  • If your account doesn't have any associated site, notice that the empty store picker is displayed with the matching design as on Figma.
  • Select Add a Store > Connect an existing site.
  • Enter the address of your test store and tap Continue.
  • You should see a Jetpack error screen from where you can Install Jetpack or Connect Jetpack natively.
  • Validate that the Jetpack installation/setup works as before.

Application password

  1. Turn on the applicationPasswordAuthenticationForSiteCredentialLogin feature flag.
  2. Login into the app using .org site credentials.
  3. Navigate to the coupons tab.
  4. Try creating, editing and deleting coupons.
  5. All features in the coupons screen should work as before.

Screenshots

NA

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@selanthiraiyan selanthiraiyan added type: task An internally driven task. feature: login Related to any part of the log in or sign in flow, or authentication. labels Jan 5, 2023
@selanthiraiyan selanthiraiyan added this to the 11.8 milestone Jan 5, 2023
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jan 5, 2023

You can test the changes from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr8553-5b9705b on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@selanthiraiyan selanthiraiyan added feature: REST API Authenticating requests using application password and using REST API instead of Jetpack tunnel. and removed feature: login Related to any part of the log in or sign in flow, or authentication. labels Jan 5, 2023
@selanthiraiyan selanthiraiyan marked this pull request as ready for review January 5, 2023 05:06
@itsmeichigo itsmeichigo self-assigned this Jan 5, 2023
itsmeichigo
itsmeichigo approved these changes Jan 6, 2023
View reviewed changes
Copy link
Contributor

@itsmeichigo itsmeichigo 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 left a question regarding the tests.

Networking/NetworkingTests/Remote/RemoteTests.swift
/// Verifies that RESTRequest request doesn't parse WordPressApiError
///
func test_wordpress_org_request_parses_wordpress_api_error() async throws {
func test_wordpress_org_request_does_not_parse_wordpress_api_error() async throws {
Copy link
Contributor

@itsmeichigo itsmeichigo Jan 6, 2023

Choose a reason for hiding this comment

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

I don't understand this test case, should the request parse the error though?

Copy link
Contributor Author

@selanthiraiyan selanthiraiyan Jan 6, 2023

Choose a reason for hiding this comment

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

The original test case validated that WordPressApiValidator is used in WordPressOrgRequest.

In RESTRequest we decided to use PlaceholderDataValidator which will not validate the request. So, I updated the test case to check that no errors are parsed.
i.e. test_wordpress_org_request_does_not_parse_wordpress_api_error and test_wordpress_org_request_does_not_parse_dotcom_error will ensure that PlaceholderDataValidator is used.

Networking/NetworkingTests/Remote/RemoteTests.swift
let result = try await remote.enqueue(request, mapper: mapper)

// Then
XCTAssertNotNil(result)
Copy link
Contributor

@itsmeichigo itsmeichigo Jan 6, 2023

Choose a reason for hiding this comment

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

Should we check that result is failure because we are mocking a timeout error?

Copy link
Contributor Author

@selanthiraiyan selanthiraiyan Jan 6, 2023

Choose a reason for hiding this comment

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

I followed this pattern from the test_wordpress_org_request_does_not_parse_dotcom_error method.

The idea here is ensure try await remote.enqueue(request, mapper: mapper) doesn't throw any error.

Please note that the result is of type Data from DummyMapper, and not of Swift.Result<Data, Error> type.

@selanthiraiyan
Copy link
Contributor Author

selanthiraiyan commented Jan 6, 2023

Thank you for the review, Huong! 🙇

I am going ahead and merging this now. I will make changes to the unit tests if anything comes up from our PR review discussions.

@selanthiraiyan selanthiraiyan merged commit 7463829 into trunk Jan 6, 2023
@selanthiraiyan selanthiraiyan deleted the feat/8512-merge-rest-request-struct branch January 6, 2023 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@itsmeichigo itsmeichigo itsmeichigo approved these changes

@jaclync jaclync Awaiting requested review from jaclync

Assignees

@itsmeichigo itsmeichigo

Labels

feature: REST API Authenticating requests using application password and using REST API instead of Jetpack tunnel. type: task An internally driven task.

Projects

None yet

Milestone

11.8

Development

Successfully merging this pull request may close these issues.

[REST API] Merge WordPressOrgRequest and RESTRequest

4 participants

@selanthiraiyan @wpmobilebot @itsmeichigo