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

Allow users to enter addresses manually when needed #6210

Merged
merged 1 commit into from
Nov 5, 2021

Conversation

Gonals
Copy link
Contributor

@Gonals Gonals commented Nov 4, 2021

cc @tgolen

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/183067
$ https://github.com/Expensify/Expensify/issues/183908

Tests

  • Log into NewDot
  • If you don't have one, create a workspace
  • Open the workspace and click Connect Bank account. You can use our test account
  • Make it to the Company information step.
  • Do some regression test to make sure the Company address field still works as expected: You should be able to select an address from the drop-down menu and move to the next step. Entering a free text address should stop you from moving forward.
  • Click on Can't find your address? Enter it manually:

Screen Shot 2021-11-04 at 5 25 53 PM

  • Fields to enter the address manually should show up:

Screen Shot 2021-11-04 at 5 28 03 PM

  • Populating all the fields should allow you to go to the next step. You should not be able to move forward with empty fields OR with a PO box address on the street field.
  • Move ahead to the Personal Information step. There's another address field in there. It should behave the same

QA Steps

Same as testing

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

@Gonals Gonals requested a review from a team November 4, 2021 16:37
@Gonals Gonals self-assigned this Nov 4, 2021
@MelvinBot MelvinBot requested review from jasperhuangg and removed request for a team November 4, 2021 16:37
Comment on lines +149 to +155
if (!isValidAddress(this.state.addressStreet)) {
errors.addressStreet = true;
}

if (!isValidZipCode(this.state.addressZipCode)) {
errors.addressZipCode = true;
}
Copy link
Contributor

@jasperhuangg jasperhuangg Nov 4, 2021

Choose a reason for hiding this comment

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

Can probably condense this to the following, unless you specifically want errors.addressStreet to be undefined instead of false after this runs.

Suggested change
if (!isValidAddress(this.state.addressStreet)) {
errors.addressStreet = true;
}
if (!isValidZipCode(this.state.addressZipCode)) {
errors.addressZipCode = true;
}
errors.addressStreet = !isValidAddress(this.state.addressStreet) || !isValidZipCode(this.state.addressZipCode);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

? I'm a bit confused. Why do you want to get rid of errors.addressZipCode?

Copy link
Contributor

@jasperhuangg jasperhuangg left a comment

Choose a reason for hiding this comment

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

A few minor changes, overall looks good and tests well (on Web)

@jasperhuangg
Copy link
Contributor

jasperhuangg commented Nov 4, 2021

@Gonals Also, I would test this out on Mobile Web, iOS, and (if possible) Android. Mobile Web Safari and iOS can sometimes handle rendering certain styles differently, such as flexbox. If you tested with a Chromium based browser for Web, then Android should be safe.

Let me know if you need help with this; I have physical iOS and Android devices.

@Gonals
Copy link
Contributor Author

Gonals commented Nov 5, 2021

@Gonals Also, I would test this out on Mobile Web, iOS, and (if possible) Android. Mobile Web Safari and iOS can sometimes handle rendering certain styles differently, such as flexbox. If you tested with a Chromium based browser for Web, then Android should be safe.

Let me know if you need help with this; I have physical iOS and Android devices.

Hey! I tested in Android (and chrome on android), but my iOS simulator is not having it today (just deciding not to launch), so I'll take you up on your offer! Could you check if it works fine on your physical device?

Copy link
Contributor

@jasperhuangg jasperhuangg left a comment

Choose a reason for hiding this comment

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

Looks good! Wasn't able to test it on a physical iOS device either 😅 (I didn't realize you needed an Apple dev account), but I was able to get it to render correctly on the simulator!

@jasperhuangg jasperhuangg merged commit 8cca7fe into main Nov 5, 2021
@jasperhuangg jasperhuangg deleted the alberto-manualAddress branch November 5, 2021 20:15
@OSBotify
Copy link
Contributor

OSBotify commented Nov 5, 2021

✋ 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
Copy link
Contributor

🚀 Deployed to staging by @jasperhuangg in version: 1.1.14-5 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @AndrewGable in version: 1.1.15-15 🚀

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

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.

3 participants