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

Show a general error message when the phone number verification request is failed #1999

Merged
merged 2 commits into from
Jun 26, 2023

Conversation

eason9487
Copy link
Member

Changes proposed in this Pull Request:

This PR solves a task of 📌 Clarify API error messages in #1993

Closes #1101

This PR fixes the blank error message when requesting verification with an invalid phone number. Please also refer to #1998 for why this PR can only show a general error message.

Screenshots:

image

Detailed test instructions:

  1. Go to the phone number editing page under the settings page or step 3 of the onboarding flow.
  2. Request verification with an invalid phone number. For example, +31 and 111111111.
  3. Check if the UI shows a proper error message.

Additional details:

I didn't add tests in this PR as I hope to add more comprehensive tests for the VerifyPhoneNumberContent component before the end of the project if there is still time. Added a follow-up item to 📌 Add tests.

Changelog entry

Fix - Show a general error message when the phone number verification request is failed.

@eason9487 eason9487 requested a review from a team June 23, 2023 09:46
@eason9487 eason9487 self-assigned this Jun 23, 2023
@github-actions github-actions bot added changelog: fix Took care of something that wasn't working. type: bug The issue is a confirmed bug. labels Jun 23, 2023
@eason9487 eason9487 linked an issue Jun 23, 2023 that may be closed by this pull request
Copy link
Contributor

@jorgemd24 jorgemd24 left a comment

Choose a reason for hiding this comment

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

Thanks @eason9487 for addressing this. I left a minor comment about the "TODO" label. LGTM 👍

Comment on lines 613 to 618
// TODO: Remove the handling of 'backendError' and add the case for 'badRequest'.
//
// Currently, 'badRequest' won't be presented but maybe someday it can be handled.
// Ref:
// - https://github.com/woocommerce/google-listings-and-ads/issues/1101
// - https://github.com/woocommerce/google-listings-and-ads/issues/1998
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm unsure whether this should be marked as a "TODO" or considered pending since we lack knowledge about potential changes in the API response down the line. One option could be to omit the "TODO" label and simply leave the comments as they are.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removing only TODO might make the semantics deviate somewhat from what this error handling was supposed to do. I rephrased it in cbda283 to only describe the "why" but didn't specify "what" to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @eason9487 for the adjustment, I think the comment is clearer now.

Base automatically changed from tweak/1246-display-api-error-message to develop June 26, 2023 09:57
@eason9487 eason9487 merged commit 3cc6936 into develop Jun 26, 2023
4 checks passed
@eason9487 eason9487 deleted the fix/1101-invalid-phone-number-error-msg branch June 26, 2023 10:00
@eason9487 eason9487 mentioned this pull request Jul 11, 2023
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: fix Took care of something that wasn't working. type: bug The issue is a confirmed bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No message on invalid phone number
2 participants