search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.49k stars 2.84k forks source link

[HOLD for payment 2023-05-04] ON HOLD [$1000] Suggestion text below company address is not visible when we don't select any suggested option but it is visible in every other cases #16448

Closed kavimuru closed 1 year ago

kavimuru commented 1 year ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Action Performed:

  1. Open the app
  2. Open Workspaces
  3. Open any workspace
  4. Open connect bank account
  5. Click on connect manually
  6. Fill in step 1 information (eg data: routing number = 210021002, acc no = 1234)
  7. In company address, type in address but don't select any option and after typing, select city below
  8. Observe that suggestion text below company address is gone
  9. Fill in other details, click on continue and click on back and obserce that suggestion text below company address is back
  10. Click on back few times and click on startover, again fill in step 6 details and observe that company address has suggestion text below it
  11. Type in address and select any one suggestion and observe that company address has suggestion text below it

Expected Result:

App should display suggestion text below company address if user decides to fill it on their own and not use the suggested addresses

Actual Result:

App does not display suggestion text below company address if user decides to fill it on their own and if suggestions were still displayed before user tries to move to fill in other details

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Version Number: 1.2.88-0 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos:

https://user-images.githubusercontent.com/43996225/227250842-bc253546-6f19-4bd5-97b1-2778e8171123.mp4

https://user-images.githubusercontent.com/43996225/227250929-6267a11f-4548-4ddf-be78-3398e62e9764.mp4

Expensify/Expensify Issue URL: Issue reported by: @dhanashree-sawant Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1679566976110789

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~018ed613b675853bc5
  • Upwork Job ID: 1641411262190702592
  • Last Price Increase: 2023-03-30
MelvinBot commented 1 year ago

Triggered auto assignment to @zanyrenney (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

MelvinBot commented 1 year ago

Bug0 Triage Checklist (Main S/O)

zanyrenney commented 1 year ago

I'm not sure if this constitutes a bug, so need to check on this and bring a conversation to bug0.

zanyrenney commented 1 year ago

I guess looking at this again, it is inconsistent so makes sense to progress forward here.

MelvinBot commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~018ed613b675853bc5

MelvinBot commented 1 year ago

Current assignee @zanyrenney is eligible for the External assigner, not assigning anyone new.

MelvinBot commented 1 year ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak (External)

MelvinBot commented 1 year ago

Triggered auto assignment to @iwiznia (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

Prince-Mendiratta commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

In the AddressSearch component, when we there is a hint present on the input and we enter an address but do not select any recommendation, then when we focus away from the input, the hint is not shown.

What is the root cause of that problem?

We render the hint conditionally to hide it when the suggestions are being shown: https://github.com/Expensify/App/blob/b3f6dda43f013179fa405948f6262cc54341df1e/src/components/AddressSearch.js#L231 However, when we do not select an address and focus away, the displayListViewBorder state does not change, thus making the hint not show up.

What changes do you think we should make in order to solve the problem?

We need to modify the onBlur prop passed to GooglePlacesAutocomplete such that it executes any existing onBlur prop passed to it and sets the displayListViewBorder to false.

It is necessary to execute the props.onBlur because in most components, the AddressSearch component is a part of the Form.js. In Form.js, there exists an onBlur prop such that when the focus is changed, a validation function is executed. So to fix this issue, we need to execute that function in conjugation with executing the passed onBlur prop.

This can be achieved by creating an inline function which is passed to the onBlur prop in GooglePlacesAutocomplete. This function will first check if props.onBlur is a function and if it is, it executes with props.onBlur(). Then, it executes setDisplayListViewBorder(false); to set the correct state, allowing for the hint to show back up.

Result https://user-images.githubusercontent.com/54077356/228835671-4bd46971-db5b-45e1-9261-ca3ce641189b.mp4
zanyrenney commented 1 year ago

Thanks for your proposal @Prince-Mendiratta

Pujan92 commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Suggestion text not toggles and shows once we move focus to the other elements without selecting/pressing any suggested option

What is the root cause of that problem?

Currently, onPress of the option from the GooglePlacesAutocomplete we are setting the displayListViewBorder to false which shows the hint text. But on blur of the textInput of GooglePlacesAutocomplete textInputProps we are not setting displayListViewBorder to false which causes this issue.

What changes do you think we should make in order to solve the problem?

We need to update the onBlur inside textInputProps of the GooglePlacesAutocomplete with onBlur: () => setDisplayListViewBorder(false), https://github.com/Expensify/App/blob/b091e3c747dc39e9642c65199ee31d46b09d81ef/src/components/AddressSearch.js#L213-L236 We are not passing onBlur from the parent for any occurrences so we can take out props.onBlur.

tienifr commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

App does not display hint text under the company address is the user click outside of the dropdown without selecting an option.

What is the root cause of that problem?

In this line https://github.com/Expensify/App/blob/4ee21e95a38858df28e46caf54dc97e1bca9bc7a/src/components/AddressSearch.js#L236, the hint is only displayed if the displayListViewBorder is false.

The displayListViewBorder is set to false if the user selects an option, but is not if the user blurs out of the GooglePlacesAutocomplete.

What changes do you think we should make in order to solve the problem?

We need to set the displayListViewBorder to false when the user blurs the address search input and the option list.

This can be done by updating this line https://github.com/Expensify/App/blob/4ee21e95a38858df28e46caf54dc97e1bca9bc7a/src/components/AddressSearch.js#L241 to 

onBlur: (event) => {
    if (!(containerRef.current && event.target && containerRef.current.contains(event.relatedTarget))) {
        setDisplayListViewBorder(false);
    }
    props.onBlur(event);
}

The containerRef is the ref of the address search container (could be this one) that we can set via useRef.

The related target check is required here because without it, when we select an option, the onBlur will still trigger, setting displayListViewBorder to false will make the auto complete component re-render before onPress is called, making selecting an option not working.

Currently there's a bug in our react-native-google-places-autocomplete fork here https://github.com/Expensify/react-native-google-places-autocomplete/blob/e12768f1542e7982d90f6449798f0d6b7f18f192/GooglePlacesAutocomplete.js#L860 that is not passing the event to onBlur, we need to fix that as well. The main react-native-google-places-autocomplete lib already fixed is so we can just update our fork to the latest

What alternative solutions did you explore? (Optional)

Result

Working well after the fix:

https://user-images.githubusercontent.com/113963320/229275934-9e6971a6-6c5e-4688-9716-afb4be087812.mov

MelvinBot commented 1 year ago

@iwiznia, @eVoloshchak, @zanyrenney Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

iwiznia commented 1 year ago

@eVoloshchak can you review the proposals above please?

eVoloshchak commented 1 year ago

@Prince-Mendiratta's and @Pujan92's proposals use the same approach: call setDisplayListViewBorder(false) when GooglePlacesAutocomplete's input is blurred. They both resolve our issue, but as @tienifr's pointed out in their proposal, there is a bug: when we select an option, the onBlur will still trigger, setting displayListViewBorder to false will make the auto-complete component re-render before onPress is called, making selecting an option not working. So essentially you can't select an address

@tienifr's proposal addresses this issue.

We need to set the displayListViewBorder to false inside InteractionManager.runAfterInteractions when the user blurs the address search input and the option list.

Not sure I fully understand, do you propose we use both InteractionManager.runAfterInteractions and relatedTarget check or just one of these?

tienifr commented 1 year ago

@eVoloshchak sorry for the confusion, either one of these will fix the issue (no need for both), but I prefer to use the relatedTarget check

eVoloshchak commented 1 year ago

@eVoloshchak sorry for the confusion, either one of these will fix the issue, but I prefer to use the relatedTarget check

Ok, thanks for the clarification. I agree relatedTarget check is preferred, we're using the same approach in a couple of components throughout the app. I think we can proceed with @tienifr's proposal (relatedTarget check)

🎀👀🎀 C+ reviewed! cc: @iwiznia

iwiznia commented 1 year ago

Proposal looks good

MelvinBot commented 1 year ago

📣 @tienifr You have been assigned to this job by @iwiznia! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

tienifr commented 1 year ago

Hi @eVoloshchak @iwiznia We need to update react-native-google-places-autocomplete in our fork before. The PR on react-native-google-places-autocomplete fork is ready for review

tienifr commented 1 year ago

@iwiznia Friendly bump, could you take a look at 2 PRs on this issue? Thanks

iwiznia commented 1 year ago

Reviewed the fork and posted in slack to see how we handle the unsigned commits

tienifr commented 1 year ago

@iwiznia @zanyrenney I think we need to put this issue on hold until the slack discussion is finished and the rule is removed cc @eVoloshchak

zanyrenney commented 1 year ago

Sounds good. Putting on hold now.

MelvinBot commented 1 year ago

@iwiznia, @eVoloshchak, @zanyrenney, @tienifr Whoops! This issue is 2 days overdue. Let's get this updated quick!

eVoloshchak commented 1 year ago

Not overdue We should remove the signed commits requirement in all of the forks soon, which would allow us to merge the PR for this issue

eVoloshchak commented 1 year ago

This shouldn't be on HOLD anymore, the first PR was merged, only https://github.com/Expensify/App/pull/16875 is left, currently in review

MelvinBot commented 1 year ago

Reviewing label has been removed, please complete the "BugZero Checklist".

MelvinBot commented 1 year ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.3.6-0 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-05-04. :confetti_ball:

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

Assigned: 2023-04-03 5:01pm On hold until: 2023-04-23 5:04pm Merged: 2023-04-25 7:42pm

Based on the time between it being taken off hold, and it being merged, I think we're 👍 for the 50% urgency bonus

MelvinBot commented 1 year ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

dhanashree-sawant commented 1 year ago

Hi @zanyrenney , @iwiznia , @eVoloshchak ping for payment.

melvin-bot[bot] commented 1 year ago

@iwiznia, @eVoloshchak, @zanyrenney, @tienifr Eep! 4 days overdue now. Issues have feelings too...

zanyrenney commented 1 year ago

I was OOO. Catching up now

zanyrenney commented 1 year ago

Currently having an issue on Upwork with the Job posting: 2023-05-09_14-01-28

zanyrenney commented 1 year ago

asking for help in bug0 https://expensify.slack.com/archives/C01SKUP7QR0/p1683637396695429

eVoloshchak commented 1 year ago
eVoloshchak commented 1 year ago

Regression Test Proposal

  1. Open Workspaces > select any workspace > connect bank account
  2. Click on connect manually
  3. Fill in step 1 information (eg data: routing number = 210021002, acc no = 1234)
  4. In the company address, type in address but don't select any option, wait for the list with addresses to load
  5. Click outside the list (ex: click on the city field)
  6. Verify that the suggestion text below the company address is displayed

Do we agree 👍 or 👎

zanyrenney commented 1 year ago

At Accountex so won't get to this regression test + payout for a few days, reassiging.

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @conorpendergrast (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

conorpendergrast commented 1 year ago

Cool I'll get to this today!

conorpendergrast commented 1 year ago

@iwiznia Can you give me a sense-check and a 👍 or 👎 on the bonus here? I've ignored the assignment date, and instead I'm working based on the date that this was taken off hold. Dates are here

conorpendergrast commented 1 year ago

Offers sent at base rate, will add bonus and pay pending discussion here

melvin-bot[bot] commented 1 year ago

Current assignee @conorpendergrast is eligible for the Bug assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @sophiepintoraetz (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

conorpendergrast commented 1 year ago

@sophiepintoraetz Sorry to bring you in on this too Sophie! I am OoO until Monday and in the interest of these payments being overdue, I wanted to make sure they didn't have to wait until then.

Once you and @iwiznia have agreed if the bonuses are due, then the contracts here can be paid. I've already paid the issue reporter!

iwiznia commented 1 year ago

Yep, agree on bonuses

conorpendergrast commented 1 year ago

Ionatan is speedy, so I'm doing the payments. Wrap-up steps are yours, Sophie!