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.

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

Handle VBA errors with red field outlines #4431

Merged
merged 16 commits into from
Aug 12, 2021
Merged

Handle VBA errors with red field outlines #4431

merged 16 commits into from
Aug 12, 2021

Conversation

jasperhuangg
Copy link
Contributor

@jasperhuangg jasperhuangg commented Aug 5, 2021 •
edited
Loading

Details

This PR adds red borders for any validation errors stemming from text inputs in the VBA flow.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/171620

Tests/QA

  1. Navigate to <baseURL>/bank-account/company.
  2. Input invalid data for the following fields and try to submit the form. Verify that red outlines AND growl messages occur.
    • Empty password
    • Invalid address street
    • Invalid address zip code
    • Invalid website URL
    • Invalid company tax ID
    • Invalid industry code
    • Invalid incorporation date
  3. Navigate to <baseURL>/bank-account/requestor.
  4. Input invalid data for any of the fields and try to submit the form. Verify that red outlines AND growl messages occur.
    • Empty date of birth
    • Invalid address street
    • Invalid address zip code
    • Invalid last 4 digits of SSN

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screen Shot 2021-08-09 at 7 23 23 AM

Mobile Web

Screen Shot 2021-08-09 at 7 23 40 AM

Desktop

Screen Shot 2021-08-09 at 7 56 10 AM

iOS

Simulator Screen Shot - iPad Pro (12 9-inch) (4th generation) - 2021-08-09 at 07 42 55

Android

Screenshot_1628531581

@jasperhuangg jasperhuangg changed the title Jasper vba flow red field outlines Handle VBA errors with red field outlines Aug 5, 2021
@jasperhuangg jasperhuangg self-assigned this Aug 5, 2021
@jasperhuangg jasperhuangg marked this pull request as ready for review August 9, 2021 17:56
@jasperhuangg jasperhuangg requested a review from a team as a code owner August 9, 2021 17:56
@MelvinBot MelvinBot requested review from joelbettner and removed request for a team August 9, 2021 17:56
@marcaaron marcaaron mentioned this pull request Aug 9, 2021
Merged
5 tasks
@Gonals Gonals self-requested a review August 10, 2021 13:40
@Gonals
Copy link
Contributor

Gonals commented Aug 10, 2021

Grabbing the review since I have some bandwidth and was asked to!

@Gonals
Copy link
Contributor

Gonals commented Aug 10, 2021

Tests well, but shouldn't we change the Incorporation Date field to be a date input?
Similarly, we could also enforce the format on the phone number (where I can write letters), tax ID number, zip code, and so on. Is that planned down the line? It doesn't look very professional like this

Gonals
Gonals reviewed Aug 10, 2021
View reviewed changes
Copy link
Contributor

@Gonals Gonals left a comment

Choose a reason for hiding this comment

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

Tests well. Just one comment (and there are conflicts)

@kevinksullivan
Copy link
Contributor

@Gonals I totally agree we can clean up fields for polish, but we can get a follow up issue for that.

@joelbettner
Copy link
Contributor

@jasperhuangg looks good, but conflicts.

@TomatoToaster TomatoToaster self-assigned this Aug 11, 2021
@TomatoToaster
Copy link
Contributor

I'll help out with the conflicts here so ya'll can re-review.

@TomatoToaster
Copy link
Contributor

Jasper is OOO, but I resolved the merge conflicts for him. Ready for a re-review @Gonals @joelbettner

@joelbettner
Copy link
Contributor

@Gonals all yours.

Gonals
Gonals approved these changes Aug 12, 2021
View reviewed changes
Copy link
Contributor

@Gonals Gonals left a comment

Choose a reason for hiding this comment

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

LGTM!

@Gonals Gonals merged commit 9f73a02 into main Aug 12, 2021
@Gonals Gonals deleted the jasper-VBAFlowRedFieldOutlines branch August 12, 2021 08:05
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify OSBotify mentioned this pull request Aug 12, 2021
Closed
53 tasks
@jasperhuangg
Copy link
Contributor Author

@Gonals @joelbettner Thanks so much for taking charge and resolving the merge conflicts :)

@OSBotify OSBotify mentioned this pull request Aug 17, 2021
Closed
@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @Gonals in version: 1.0.85-10 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@isagoico isagoico mentioned this pull request Aug 18, 2021
Closed
@isagoico
Copy link

isagoico commented Aug 18, 2021 •
edited
Loading

Unable to check the Requestor page because of this issue #4728

@isagoico isagoico mentioned this pull request Aug 18, 2021
Closed
@isagoico
Copy link

This other issue is failing #4730

@isagoico
Copy link

Retest for this was a pass!
image

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.0.86-11 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joelbettner joelbettner joelbettner approved these changes

@Gonals Gonals Gonals approved these changes

Assignees

@TomatoToaster TomatoToaster

@jasperhuangg jasperhuangg

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jasperhuangg @Gonals @kevinksullivan @joelbettner @TomatoToaster @OSBotify @isagoico