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

Add the authorization Step for the Application Passwords #96891

Merged
merged 16 commits into from
Dec 4, 2024

Conversation

valterlorran
Copy link
Contributor

@valterlorran valterlorran commented Nov 28, 2024

Related to #

Proposed Changes

Adds the new Authorization Step to allow us to capture the Application Password authorization. This step gives the user the possibility to authorize our application so we can use the credentials for the automate checks and tasks.

I also added the possibility of adding notices to the Step. I updated the StepContainer to accept a new prop: notice. And it is displayed before the Formatted Header.

Default view

Captura de Tela 2024-12-03 às 20 23 56

Rejected the Authorization

Captura de Tela 2024-12-03 às 20 23 43

Error trying to store the Authorization credentials

Captura de Tela 2024-12-03 às 20 23 23

Why are these changes being made?

  • This is an attempt to improve the low success rates of users credentials. Most of the credentials that the users inputs are not valid and we can proceed with many automated processes that would save a great deal of time for the HEs. More context on: paYKcK-5B6-p2

Testing Instructions

Visual inspection should be enough, as I added some tests to cover the cases.

To test it manually follow the steps:

  • Use calypso live to avoid the SSL error when trying to authenticate.
  • Go through the migration process until you reach the Credentials step.
  • Then update the URL to point to the /setup/site-migration/site-migration-application-password-authorization path. Keep the current parameters, and add the authorizationUrl, which could be a valid authorization URL for the Application Passwords: https://example.com/wp-admin/authorize-application.php&app_id=1083d914-d9f8-47ec-b49b-55d206104730&app_name=Test
  • You should see the default view, inviting the user to Authorize. Click on the "Authorize" button.
  • You should be redirected to your test site.
  • Reject the request, and you should be redirected to the Authorization step with an alert.
  • Click again on the "Authorize" button, and this time approve the request.
  • You should be redirected to the Authorization step and should see a loading state.
  • After the loading state, you'll probably see the Authorization step with an error message, because the endpoint that stores the Password will fail because you don't have the ticket.
  • So far this should be enough to test.

Pre-merge Checklist

  • Has the general commit checklist been followed? (PCYsg-hS-p2)
  • Have you written new tests for your changes?
  • Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • Have you checked for TypeScript, React or other console errors?
  • Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
    • For UI changes, have we tested the change in various languages (for example, ES, PT, FR, or DE)? The length of text and words vary significantly between languages.
  • For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

@matticbot
Copy link
Contributor

matticbot commented Nov 28, 2024

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~245 bytes added 📈 [gzipped])

name                           parsed_size           gzip_size
site-migration-flow                 +597 B  (+0.7%)      +98 B  (+0.6%)
hosted-site-migration-flow          +597 B  (+0.7%)      +98 B  (+0.6%)
update-design-flow                   +38 B  (+0.0%)      +12 B  (+0.0%)
entrepreneur-flow                    +38 B  (+0.0%)      +16 B  (+0.1%)
with-theme-assembler-flow            +27 B  (+0.0%)       +5 B  (+0.0%)
update-options-flow                  +27 B  (+0.0%)       +6 B  (+0.1%)
trial-wooexpress-flow                +27 B  (+0.0%)       +6 B  (+0.1%)
tailored-ecommerce-flow              +27 B  (+0.0%)       +6 B  (+0.1%)
site-setup-wg                        +27 B  (+0.0%)       +3 B  (+0.0%)
site-setup-flow                      +27 B  (+0.0%)       +3 B  (+0.0%)
readymade-template-flow              +27 B  (+0.0%)       +7 B  (+0.0%)
migration-signup                     +27 B  (+0.0%)       +4 B  (+0.0%)
migration-flow                       +27 B  (+0.0%)       +6 B  (+0.0%)
import-flow                          +27 B  (+0.0%)       +7 B  (+0.1%)
free-post-setup-flow                 +27 B  (+0.0%)       +4 B  (+0.1%)
assembler-first-flow                 +27 B  (+0.0%)       +5 B  (+0.0%)
ai-assembler-flow                    +27 B  (+0.0%)       +6 B  (+0.0%)
write-flow                           +11 B  (+0.0%)       +8 B  (+0.0%)
transferring-hosted-site-flow        +11 B  (+0.0%)       +8 B  (+0.0%)
plugin-bundle-flow                   +11 B  (+0.0%)      +10 B  (+0.0%)
plans                                +11 B  (+0.0%)       +8 B  (+0.0%)
newsletter-post-setup-flow           +11 B  (+0.0%)       +8 B  (+0.0%)
link-in-bio-tld-flow                 +11 B  (+0.0%)       +8 B  (+0.0%)
link-in-bio-post-setup-flow          +11 B  (+0.0%)       +8 B  (+0.0%)
import-hosted-site-flow              +11 B  (+0.0%)      +10 B  (+0.0%)
domain-user-transfer-flow            +11 B  (+0.0%)       +8 B  (+0.0%)
copy-site-flow                       +11 B  (+0.0%)      +10 B  (+0.0%)
build-flow                           +11 B  (+0.0%)       +8 B  (+0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~16 bytes added 📈 [gzipped])

name                                                 parsed_size           gzip_size
async-load-signup-steps-initial-intent                     +11 B  (+0.0%)       +9 B  (+0.0%)
async-load-automattic-onboarding-src-step-container        +11 B  (+0.1%)       +7 B  (+0.2%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

@valterlorran valterlorran self-assigned this Dec 2, 2024
@valterlorran valterlorran requested a review from a team December 3, 2024 23:24
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 3, 2024
@valterlorran valterlorran marked this pull request as ready for review December 3, 2024 23:35
@valterlorran valterlorran requested a review from a team as a code owner December 3, 2024 23:35
@valterlorran valterlorran force-pushed the add/authorize-step-application-password branch from 0bd4911 to 228b6f3 Compare December 3, 2024 23:37
@matticbot
Copy link
Contributor

matticbot commented Dec 3, 2024

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

  • notifications
  • wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug add/authorize-step-application-password on your sandbox.

@valterlorran valterlorran force-pushed the add/authorize-step-application-password branch from c0268d0 to 4a0deb8 Compare December 4, 2024 00:32
Copy link
Contributor

@andres-blanco andres-blanco left a comment

Choose a reason for hiding this comment

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

Here's a bunch of small design differences that doesn't match the figma file:

  • Spacing between the title and the paragraph below
  • The site URL should be bold
  • Spacing between paragraph and CTA button
  • The weight of the Share credentials instead link is different
  • Spacing between Share credentials instead link and Here's what else you're getting title is different
  • Spacing between Here's what else you're getting title and the description box is different
  • The description box items have icons in the figma
  • Font size of the description box items is different

Also, the Need help? Let us guide you link in the top right is missing.
image

Please tackle the easy fixes and leave the rest for a subsequent PR.

@andres-blanco
Copy link
Contributor

@valterlorran the CTA en the Figma reads Authorize access, in the PR is Authorize

@andres-blanco
Copy link
Contributor

Screen.Recording.2024-12-04.at.9.46.24.AM.mov

When I click authorize I get redirected to the first step of the flow. Based on the undefined that appears on the URL this might be related to the site_url vs from parameter

@valterlorran
Copy link
Contributor Author

@andres-blanco

Spacing between the title and the paragraph below

We should not override these values to not create inconsistencies with other steps. We should use the default behavior of the component.

The site URL should be bold

Fixed

Spacing between paragraph and CTA button

I’m assuming this means the space between the We're ready to migrate… subtitle and the Authorize access button: Again the space added between is default of the header and the step container, reducing that would be different from other steps.

The weight of the Share credentials instead link is different

I’m using the default link that we have been using in other steps. I think if we decide to tackle that, we should do a refactor.

Spacing between Share credentials instead link and Here's what else you're getting title is different

Fixed

Spacing between Here's what else you're getting title and the description box is different

Fixed

The description box items have icons in the figma

Isn’t that showing up for you? Can you share a screen shot?

Font size of the description box items is different

I used the $font-body-small instead of hardcoding the site and to have consistence.

@andres-blanco
Copy link
Contributor

Isn’t that showing up for you? Can you share a screen shot?

image

Figma: F8foKv974Gc5dTHSh0i0Wo-fi-648_7452

Copy link
Contributor

@andres-blanco andres-blanco left a comment

Choose a reason for hiding this comment

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

The code overall looks good and the happy path seems to be working correctly. I found a couple of blockers.

image
  • The let us guide you in the top right is still missing

<h3>{ translate( "Here's what else you're getting" ) }</h3>
<AuthorizationBenefits
benefits={ [
translate( 'Uninterrupted service throughout the entire migration experience.' ),
Copy link
Contributor

Choose a reason for hiding this comment

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

where this strings added to the string freeze PR we where using for having translations ready earliear?

@valterlorran
Copy link
Contributor Author

When I get redirected to the wp-admin screen to authorize there's no Application Name pre-filled (we should user the app_name query param specified here

This should be working, the thing is that on the testing instructions I didn't add the App ID and App Name, but this should come from the endpoint. In order for that to work just add the app_id and the app_name params to your authorizationUrl:

Please see: fbhepr%2Skers%2Sjcpbz%2Sjc%2Qpbagrag%2Syvo%2Szvtengvba%2Snhgbzngrq%2Qzvtengvba%2Qgbbyf.cuc%3Se%3Q9r16q365%231171%2Q1178-og

Copy link
Contributor

@andres-blanco andres-blanco left a comment

Choose a reason for hiding this comment

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

Based on this comment I think this is in a good state for merging.

@valterlorran valterlorran merged commit cf6e517 into trunk Dec 4, 2024
11 checks passed
@valterlorran valterlorran deleted the add/authorize-step-application-password branch December 4, 2024 19:07
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 4, 2024
@a8ci18n
Copy link

a8ci18n commented Dec 4, 2024

This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/17046536

Some locales (Hebrew, Japanese) have been temporarily machine-translated due to translator availability. All other translations are usually ready within a few days. Untranslated and machine-translated strings will be sent for translation next Monday and are expected to be completed by the following Friday.

Thank you @valterlorran for including a screenshot in the description! This is really helpful for our translators.

mehmoodak pushed a commit that referenced this pull request Dec 6, 2024
* Add the authorization Step for the Application Passwords

* Add the notice when the authorization is request and add the texts

* Apply the style to the component

* Added the submit logic for the new step

* Add the tests

* Add the action to skip the error on storing the credentials

* Add the redirect to the fallback step

* Fix the authorizationUrl parameter name

* Try fix the errors

* Fix tests

* Add the back button logic

* Fix the from param

* Fix design issues

* Fix button test

* We should encode the success_url to allow the source site to redirect to the step approprieally

* Fix the authorizationUrl parameter
@a8ci18n
Copy link

a8ci18n commented Dec 8, 2024

Translation for this Pull Request has now been finished.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants