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.72k stars 2.94k forks source link

[HOLD for payment 2024-02-07] [$500] [MEDIUM] Your location does not show full address in Distance expense #34233

Closed m-natarajan closed 9 months ago

m-natarajan commented 11 months 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!


Version Number: 1.4.24-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 Expensify/Expensify Issue URL: Issue reported by: @puneetlath Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1704809953374819

Action Performed:

  1. create a workspace
  2. Open workspace chat
  3. Create a distance expense with start or stop waypoint as " your location"
  4. OPen the receipt details

Expected Result:

Your location should show full address

Actual Result:

Does not show full address

Workaround:

unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

Screen Shot 2024-01-10 at 4 04 50 AM

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0122ae0fa315e4c6b8
  • Upwork Job ID: 1745071353821593600
  • Last Price Increase: 2024-01-17
  • Automatic offers:
    • esh-g | Contributor | 28117765
    • dukenv0307 | Contributor | 0
Issue OwnerCurrent Issue Owner: @dylanexpensify
melvin-bot[bot] commented 11 months ago

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

melvin-bot[bot] commented 11 months ago

Job added to Upwork: https://www.upwork.com/jobs/~0122ae0fa315e4c6b8

melvin-bot[bot] commented 11 months ago

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

unidev727 commented 11 months ago

Proposal

from: @unicorndev-727

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

Your location does not show full address in Distance expense

What is the root cause of that problem?

It is a new feature because we just set CONST.YOUR_LOCATION_TEXT as a address here. https://github.com/Expensify/App/blob/7ae1ec7e74b908e6340bccaf7e42b0345d766711/src/components/AddressSearch/index.js#L348-L353

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

  1. We need to add a API that returns the full current address and name from 'latitude' and longitude.
  2. If the user select use current location, we should call above API here in FE and set the full address and name from API result here .

What alternative solutions did you explore?

Already we get geolocation API url here so we can do same for this case, I think.

abzokhattab commented 11 months ago

This is the expected behavior since we set the address to Your Location when the user selects current location https://github.com/Expensify/App/blob/179d306f249993fd3ee7921d69b2c3e574ba1d62/src/components/AddressSearch/index.js#L343-L348

the way to make it show the address would be to call an external api that would take the lat and long and return the address info.

dukenv0307 commented 11 months ago

Proposal

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

Does not show full address in distance e-receipt

What is the root cause of that problem?

When the user uses current location, we're setting both the address and name to Your location. This makes total sense in normal case but doesn't make sense for e-receipt because this is something you're supposed to be able to share with others, and they have no idea what Your location is.

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

  1. Use Google Reverse Geocoding API (or Mapbox's) to get the address from the lat and lng. We can use the API directly or can use a package for react/react-native/js that supports it.
  2. Either add a field to the location (like reversedAddress) and set the geocoding-reversed address to that field, or put in the address and/or name field, but this might cause UX changes for current location flow
  3. When showing the distance e-receipt, use the address field above instead of the address field (or we can use in both places of name and address, since Your location doesn't make much sense in the context of an e-receipt).

The reason another field is recommended for the reversed address is because it's only relevant for the e-receipt, we don't want to change the current UX for selecting current location. But this is up to design team's decision.

Updates after the confirmed back-end change:

Make sure the style of the address and name are the same by updating this to

{waypoint.address && <Text style={[styles.eReceiptWaypointAddress, waypoint.name && {fontSize: variables.fontSizeLabel}]}>{waypoint.address}</Text>}

What alternative solutions did you explore? (Optional)

Also need to check the case of e-receipt in offline mode as well to see if we'd like to show, because in that case there might not be the reversedAddress to show, maybe we can show TBD or Your location as it currently is.

Instead of using the Reverse Geocoding API directly in front-end side, we can return it from the back-end and the back-end will handle it internally for Your location case. But other steps should be the same (still need to show it in the e-receipt.

rojiphil commented 11 months ago

Proposal

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

For Distance Expenses, a generic message Your location is displayed.

What is the root cause of that problem?

Here, when we use Geolocation API to fetch the current position, we explicitly set the address and name to a generic Your location text which is the root cause of this problem. As Your location text is a piece of generic information without any specifics it is not useful for reference purposes.

As mentioned here, BE now performs reverse geocoding and fills in address with an address if available for Your location case. Otherwise, it would contain location coordinates. Further, name would be an empty string such a case.

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

Because of the recent changes, only a styling change would be required i.e. to show address with monospaced font and white color.

Option 1: If name is empty, show address with font style as that of name For this , we can make the following change here

{waypoint.address && <Text style={waypoint.name ? styles.textLabelSupporting: styles.eReceiptWaypointAddress}>{waypoint.address}</Text>}

Option 2: Show name and address with the same font style For this, we can make the following change here

{waypoint.address && <Text style={styles.eReceiptWaypointAddress}>{waypoint.address}</Text>}

Also, it is most likely that the address also starts with the name. In this case, I think we may want to avoid showing the name if address is already beginning with name to avoid duplicates. We can change this line as follows:

{waypoint.name && !waypoint.address.startsWith(waypoint.name) && <Text style={styles.eReceiptWaypointAddress}>{waypoint.name}</Text>}

Screenshot 2024-01-19 at 7 48 08 AM

Similar style issue with AddressSearch There is a similar style issue with AddressSearch as shown below: This happens when we create a distance request offline with waypoints and, later, come online. We can fix this by showing address if name does not exist by making following changes here:

          <Text style={[styles.googleSearchText]}>{title ?? subtitle}</Text>
          {subtitle && title && <Text style={[styles.textLabelSupporting]}>{subtitle}</Text>}

Screenshot 2024-01-19 at 1 28 55 PM

What alternative solutions did you explore? (Optional)

Alternatively, we may not want to show the name at all and remove this line

Archived Solution 1: We can leave name as user friendly Your location text itself and set address with the location coordinates in an agreed format. In this case, the receipt will also have location-specific details for reference purposes in e-receipt.

Archived Solution 2: As converting the location coordinates to a full address is not guaranteed, I would suggest that we include the location coordinate information after Your location text. The format can be Your location,latitude,longitude or Your location[latitude][longitude] where latitude and longitude refer to the current position co-ordinates E.g. Your Location,13.0644986,77.5702959 or Your Location[13.0644986][77.5702959] or any other agreed format The format with comma seems to be simple to use.

The changes can be done here for address and name like this:

  address: CONST.YOUR_LOCATION_TEXT + `,`+ successData.coords.latitude + `,`+ successData.coords.longitude,
  name: CONST.YOUR_LOCATION_TEXT + `,`+ successData.coords.latitude + `,`+ successData.coords.longitude,

We may also want to prevent such locations from being added to the recent destination list. For this, we can check if the address starts with Your location here like this:

if (waypoint?.address.startsWith(CONST.YOUR_LOCATION_TEXT)) {    
    return;
}
dylanexpensify commented 10 months ago

@thesahindia can we get reviews today please? 🙇‍♂️

thesahindia commented 10 months ago

@dylanexpensify, it seems this was built like this. Can you please confirm if we wanna change this and what should be the expected result?

dylanexpensify commented 10 months ago

@neil-marcellini mind confirming where we landed here?

melvin-bot[bot] commented 10 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

rojiphil commented 10 months ago

@thesahindia @neil-marcellini @dylanexpensify

it seems this was built like this. Can you please confirm if we wanna change this and what should be the expected result?

While I also think it was built like this, I think there was an oversight of the implication on the e-receipt when it comes to Your location. I think it seems meaningless to see Your location as name and Your location as address in the e-receipt as shown in OP screenshot here. And I believe that is the issue here.

I think, at the minimum, we can set the address with the location coordinates in an agreed format which can solve this problem. This is my alternative solution in my proposal.

melvin-bot[bot] commented 10 months ago

Current assignee @thesahindia is eligible for the Internal assigner, not assigning anyone new.

tgolen commented 10 months ago

I'm going to switch this to be internal. We discussed (ref) that the best thing to do would be to reverse-geocode the address from the lat/lng on the server when the request is created. The phrase "Your location" will remain in location.name, but the address will be updated in location.address.

melvin-bot[bot] commented 10 months ago

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

tgolen commented 10 months ago

OK, there are still some things we want to change on the front-end, so I am going to mark this external as well.

Here is an update on the backend changes I am making:

@rojiphil, let's definitely do "We may also want to prevent such locations from being added to the recent destination list."

The styling also needs to be fixed. The location address needs to be the same font and style as the location name (the monospaced font and white color).

image

Would anyone like to modify their proposals to do these?

esh-g commented 10 months ago

Proposal

Please re-state the issue we are trying to solve

The styling needs to be fixed. The location address needs to be the same font and style as the location name (the monospaced font and white color).

What is the root cause?

Only a styling change (n/a)

What changes should be made to fix this?

If we want a solution to show the address in primary font if name is absent go with this option: We should set each title and subtitle like this:

const title = waypoint.name ? waypoint.name : waypoint.address;
const subtitle = waypoint.name ? waypoint.address : ''; // We can also use a translation here for 'Your Location'

And then use them here: https://github.com/Expensify/App/blob/599c005849cd065a53d35e015991b637c768f2da/src/components/DistanceEReceipt.js#L99-L100 This would mean that if no waypoint.name is present, we use waypoint.address as the main title and waypoint.address is empty.

What alternate did you explore?

If we just want to modify the address font to be like name but smaller, we can go with this option: We should modify the following address: https://github.com/Expensify/App/blob/599c005849cd065a53d35e015991b637c768f2da/src/components/DistanceEReceipt.js#L99-L100 to:

{waypoint.address && <Text style={styles.eReceiptGuaranteed}>{waypoint.address}</Text>}

This way the font and style matches the address while the size is still smaller.

Screenshot 2024-01-19 at 9 18 25 PM
esh-g commented 10 months ago

@tgolen While adding the name field, I remember discussing this with @neil-marcellini whether we should use waypoint.address as the title if waypoint.name is not present. It was decided to keep them separate (as name is optional and address is mandatory) but maybe we can look into that again.

tgolen commented 10 months ago

I think we still want to keep them separate for now. A few other things I noticed:

esh-g commented 10 months ago

@tgolen

tgolen commented 10 months ago

it would be useful if user wants to use their last location

That's a good point that I hadn't considered. OK, let's leave it for now.

To be more clear on my other point. I'm saying that both of these should use the same font style (and it should be the monospaced/white font style). Does that make more sense? I just want to be sure your solution covers both (maybe it does and I just didn't realize it).

image

rojiphil commented 10 months ago

Proposal

Updated with styling changes. @tgolen Additionally, wondering if we can remove display of name in e-receipt as there would always be an address to show and the address would most likely also include the name.

rojiphil commented 10 months ago

Additionally, wondering if we can remove display of name in e-receipt as there would always be an address to show and the address would most likely also include the name.

Or maybe, skip name display in e-receipt if the address already includes name in its beginning. This seems even better. I have updated the proposal with this option too.

dukenv0307 commented 10 months ago

Proposal updated to cover the minor front-end style change.

@tgolen My proposal is the first to suggest using the reverse geocoding approach and also mentioned doing that from the back-end (others either suggested the wrong approach, or only come in for the minor change after the back-end approach has been finalized).

Considering that the front-end change is minor and only cosmetic, do you think I should be assigned here for suggesting the correct core approach to solve the issue.

Thank you!

cc @thesahindia

rojiphil commented 10 months ago

@tgolen @thesahindia

The change suggested by @dukenv0307 is the same as mine. I just wanted to put this on record to avoid any confusion later.

Also, my proposal has one more suggested improvement to avoid duplicate names. Since we need to keep the same font and color for name and address, I think it's odd to see duplicates which we may want to avoid.

esh-g commented 10 months ago

@tgolen I first thought you meant to only make the address font white and monospaced if the name was absent, but now I included it in my alternate solution to make it so in every case. And unlike other proposals, it is still smaller than main name and easily distinguishable with the eReceiptGuaranteed style.

Screenshot 2024-01-19 at 9 25 34 PM
tgolen commented 10 months ago

@Expensify/design Can you help weigh-in here on how you'd like the styles to be for the addresses?

shawnborton commented 10 months ago

I think what I see in the comment here works pretty well. Just confirming - what font size are we using for the address (smaller text) below the location name?

rojiphil commented 10 months ago

The location address needs to be the same font and style as the location name (the monospaced font and white color).

@tgolen @shawnborton My understanding was that we need to keep the same font, size, and color as there can be cases where name would be empty as mentioned here. In such a case, seeing the address with a smaller font size or a different color would look odd. Or did I understand something wrong here?

shawnborton commented 10 months ago

Curious what the rest of the @Expensify/design thinks, but I think if we have a location name (like Expo Guadalajara in the example above) it's okay if the address portion is smaller.

rojiphil commented 10 months ago

Proposal

Interesting comment here. I like the idea. Updated with Option 1: If name is empty, show address with font style as that of name

rojiphil commented 10 months ago

Proposal

On testing further, I noticed a similar style issue in AddressSearch which we may want to fix here. Updated with fix for style issue in AddressSearch

dukenv0307 commented 10 months ago

Proposal updated based on this confirmation

dannymcclain commented 10 months ago

Curious what the rest of the @Expensify/design thinks, but I think if we have a location name (like Expo Guadalajara in the example above) it's okay if the address portion is smaller.

I agree that it's fine for the address text to be smaller than the name. I still think it would be ideal to create some separation between them using text and supporting-text colors—but I understand if that's not in the cards right now.

tgolen commented 10 months ago

I prefer the frontend solution from @esh-g because it generalizes the approach to be based on title/subtitle which I think makes the styling easier to reason about for the next engineer that comes along.

I also want to credit @dukenv0307 for the backend change for reverse-geocoding.

What I would like to propose is that the payment for this issue is split 50/50 with $250 going to @dukenv0307 and $250 for the frontend solution.

@thesahindia @dylanexpensify Are you OK with this arrangement?

dukenv0307 commented 10 months ago

What I would like to propose is that the payment for this issue is split

I'd be happy with that, thank you!

dylanexpensify commented 10 months ago

That works for me 👍

esh-g commented 10 months ago

What I would like to propose is that the payment for this issue is split 50/50 with $250 going to @dukenv0307 and $250 for the frontend solution.

I understand that @dukenv0307 suggested the reverse geocoding, but it is reasonably obvious as we need the full address from the coordinates and using the API was the only way we can do that. Here is an issue in which the same happened to me: https://github.com/Expensify/App/issues/14493#issuecomment-1451109437

Although I'm not completely opposed to this arrangement if you think their contribution was important, then we can go ahead with this...

tgolen commented 10 months ago

I understand we can't be consistent 100% of the time, but I do think this arrangement is fair. Thanks! Assigning @esh-g for the style changes.

melvin-bot[bot] commented 10 months ago

📣 @esh-g 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer 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 📖

s77rt commented 10 months ago

The backend changes here were only applied to CreateDistanceRequest. The issue still persist when editing money request i.e. command UpdateMoneyRequestDistance

s77rt commented 10 months ago

The BE changes caused a regression https://github.com/Expensify/App/issues/35245

dylanexpensify commented 10 months ago

cc @tgolen

tgolen commented 10 months ago

Ah, I didn't think of the edit case. I can get a PR done for that this week. I created this GH to track it so I don't forget.

tgolen commented 10 months ago

You're right, I forgot about the update flow 🤦

I created this GH to track it so I can get that added this week.

melvin-bot[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.4.33-5 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 2024-02-07. :confetti_ball:

For reference, here are some details about the assignees on this issue:

melvin-bot[bot] commented 10 months 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:

dylanexpensify commented 10 months ago

payment today! @thesahindia to do checklist!

dukenv0307 commented 10 months ago

@dylanexpensify can you also assign me to this GH issue for payment as discussed here, thank you!