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

[N6] AddressSearch component is not autofilling #5794

Closed
Luke9389 opened this issue Oct 12, 2021 · 5 comments
Closed

[N6] AddressSearch component is not autofilling #5794

Luke9389 opened this issue Oct 12, 2021 · 5 comments
Assignees
Labels
Daily KSv2 Improvement Item broken or needs improvement.

Comments

@Luke9389
Copy link
Contributor

Luke9389 commented Oct 12, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Action Performed:

There are two ways to test this

  1. Sign into New Dot (new account is fine)
  2. Create a new workspace
  3. Connect a new bank account
  4. Follow this SO for the routing number and account number https://stackoverflow.com/c/expensify/questions/342/525#525
  5. Input valid information on all fields in the Company Information step (doesn't need to be real info)

Variant 1

  1. Refresh the page

Variant 2

  1. Hit Submit. When you're on Step 3 (Personal Information) click the back arrow to go back to Step 2 (Company Informaion)

Either way

  1. Notice that all the fields are auto-filled except for the Company Address.

Expected Result:

Describe what you think should've happened

The Company Address field should draw info from the reimbursementAccountDraft onyx key to autofill itself.

Screen Shot 2021-10-12 at 2 13 05 PM

Actual Result:

The Company Address field does not autofill

Screen Shot 2021-10-12 at 6 27 10 PM

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

The user can manually input their address again.

Platform:

Where is this issue occurring?

  • Web - definitely
  • iOS - haven't tested
  • Android - haven't tested
  • Desktop App - haven't tested
  • Mobile Web - haven't tested

Version Number:
Reproducible in staging?:
Reproducible in production?:
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:
Issue reported by:
Slack conversation:

View all open jobs on GitHub

@Luke9389 Luke9389 added the Daily KSv2 label Oct 12, 2021
@Luke9389 Luke9389 self-assigned this Oct 12, 2021
@Luke9389 Luke9389 added the Improvement Item broken or needs improvement. label Oct 12, 2021
@Luke9389
Copy link
Contributor Author

Luke9389 commented Oct 13, 2021

Alright, this issue has been a bear to work on (surprisingly). This lib is designed to work inside of functional components, and I'm not sure why using it inside of a class component isn't working. The behavior described above is due to a race condition. I've been able to verify this with debuggers.

I've been able to fix this issue by doing the following

  1. Refactor AddressSearch into a functional component.
  2. Use a setTimeout in the componentDidMount method.
  3. Edit the library and add a prop to the GooglePlacesAutocomplete

I like the third option the best, but it would require forking the lib. I'd like to get some other engineers' thoughts, so I'll be posting a link to this comment in #eng-chat.

@roryabraham since we talked about this today.

@Luke9389 Luke9389 changed the title [N6] AddressSearch component is not setting value properly [N6] AddressSearch component is not autofilling Oct 13, 2021
@marcaaron
Copy link
Contributor

Spent some time looking into this and alternatives with @Luke9389 here. I am pretty solidly on team 1 and if we know that will work let's just do it. Sometimes we have to apply things the way a 3rd party libs wants and in those cases I think it's fine to make exceptions. Feels like the rule about hooks here applies. Let's make just add a comment if we have to do useEffect() or something the developer recommends because we have no other option.

@Luke9389 Luke9389 reopened this Oct 14, 2021
@Luke9389
Copy link
Contributor Author

le whoops!

@Luke9389
Copy link
Contributor Author

Hmm, not on production yet. Looking into deploy blockers.

@MelvinBot MelvinBot removed the Overdue label Oct 18, 2021
@Luke9389
Copy link
Contributor Author

Oh, actually it looks like it is on production. #5800 (comment)
Not sure why the staging message came after the production one...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Daily KSv2 Improvement Item broken or needs improvement.
Projects
None yet
Development

No branches or pull requests

3 participants