Create currency modal with react-native-permissions and geolocation #2194

rameshhpathak · 2021-04-01T04:23:23Z

Details

I used react-native-geolocation-service because the original library itself suggests to use it based on it's use of Google Services in Android.

From the library itself:

Currently, on Android, this uses the android.location API. This API is not recommended by Google because it is less accurate and slower than the recommended Google Location Services API. This is something that we want to change in the near future https://github.com/react-native-community/react-native-geolocation/issues/6.

Fixed Issues

Fixes #1970

Tests

1. Go to /iou/split on web. Verify that the currency is based on your location.
2. Click on the currency, a list with all currencies should show.
3. Search for a currency, or scroll down to your desired currency
4. Click on the currency to select and click on the Confirm button the confirm selection
5. Verify that the selected currency shows now in the amount page
6. Refresh the page, and verify that the same currency shows in the amount page

QA Steps

Same as tests

Tested On

Screenshots

Julesssss · 2021-04-01T17:44:24Z

Hi @rameshhpathak, thanks for raising the PR. I'm afraid I didn't get around to testing this today. Will do a full review after the easter bank holiday weekend (Tuesday)

ios/File.swift

AndrewGable · 2021-04-01T22:47:17Z

ios/ExpensifyCash-Bridging-Header.h

@@ -0,0 +1,4 @@
+//
+//  Use this file to import your target's public headers that you would like to expose to Swift.


Is this file required it seems blank?

What was the resolution here? I see these files were removed but there's still a BridgingFile.swift committed. Is that file necessary?

src/components/CreateMenu.js

jasperhuangg

Left a few comments, also looks like there are a few merge conflicts that need to be resolved. Thanks!

src/ONYXKEYS.js

src/libs/OptionsListUtils.js

src/pages/iou/steps/IOUAmountPage.js

src/pages/iou/steps/IOUCurrencySelection.js

rameshhpathak · 2021-04-02T03:16:52Z

Hey @jasperhuangg @AndrewGable thank you for your reviews, I will resolve the issues in line with the comments you have sent

AndrewGable · 2021-04-05T20:24:19Z

@rameshhpathak - Can you fix the failing test? Thanks!

shawnborton · 2021-04-05T22:59:51Z

I would also love to see some updated screenshots. From what I can tell, some of the spacing looks off. You should be able to use the "New Group" flow in Expensify.cash to see exactly what the spacing should look like in terms of an existing piece of UI where we have a text input, then rows of information, with a floating button at the bottom.

android/build.gradle

Julesssss · 2021-04-06T15:46:37Z

src/libs/OptionsListUtils.js

@@ -432,11 +432,37 @@ function getHeaderMessage(hasSelectableOptions, hasUserToInvite, searchValue, ma
    return '';
 }

+/**
+ * Returns the personal details for an array of logins


This comment isn't correct

Julesssss · 2021-04-06T15:51:10Z

src/libs/OptionsListUtils.js

+        isSearchStringMatch(searchValue, currencyOption.searchText)));
+
+    return {
+        // currency options holds a section for those currencies which are not selected


I'm not sure what this comment means, could you please clarify?

Sorry @Julesssss I think this was missed while I was staging changes. I will change it.

Julesssss · 2021-04-06T15:53:28Z

src/libs/actions/PersonalDetails/index.js

+ *
+ * @param {Object} details
+ */
+function setCurrencyPreferences() {


Perhaps this should be called fetchCurrencyPreferences to match the other functions called on app start.

Julesssss · 2021-04-06T16:07:50Z

src/pages/iou/steps/IOUCurrencySelection.js

+        const {currencyOptions} = getCurrencyListForSections(this.props.currencyList,
+            '');


Why the newline?

Julesssss

Noticed a few things in my first review:

The Currency Modal is supposed to be separate from the IOUModal, rather than a step within the flow. It should open on top of the current modal and then close to reveal the IOU Modal again.
At the main page after logging in on web I was asked for location permission, we want to avoid this and only ask if the user changes their currency. We don't want to ask this on every refresh. Only if previously enabled should we make the check on app start.
I'm unable to build to iOS or Android, can you confirm you haven't faced the same problem?

rameshhpathak · 2021-04-07T08:47:44Z

Noticed a few things in my first review:

* The Currency Modal is supposed to be separate from the IOUModal, rather than a step within the flow. It should open on top of the current modal and then close to reveal the IOU Modal again.

* At the main page after logging in on web I was asked for location permission, we want to avoid this and only ask if the user changes their currency. We don't want to ask this on every refresh. Only if previously enabled should we make the check on app start.

* I'm unable to build to iOS or Android, can you confirm you haven't faced the same problem?

Oh okay. I will make the necessary changes.
So we only make this request inside the setCurrencyLocation (renamed: fetchCurrencyLocation) function? Alright.

I will make the necessary changes and solve the build problems in my next commit. I have added comments to your review as well. Thanks!

Out of date

rameshhpathak · 2021-05-20T13:03:29Z

@Julesssss Sorry, I just switched it back, so we ask it every time user tries to change the currency through the modal.

Julesssss · 2021-05-20T13:08:21Z

Sorry @rameshhpathak that's not quite right. Can you revert the last commit and we'll worry about this permission prompt later? Thanks 👍

Julesssss · 2021-05-20T13:11:25Z

There are two very last issues to do with prop type validation, after that I think we're good to merge:

Opening the CurrencyModal A

Warning: Failed prop type: The prop `optionIsFocused` is marked as required in `OptionRow`, but its value is `undefined`.
    in OptionRow (created by ItemWithSeparator)
    in ItemWithSeparator (created by CellRenderer)
    in div (created by View)
    in View (created by CellRenderer)
    in CellRenderer (created by VirtualizedList)
    in div (created by View)
    in View (created by ScrollView)
    in div (created by View)
    in View (created by ForwardRef)
    in ForwardRef (created by ScrollView)
    in ScrollView (created by VirtualizedList)
    in VirtualizedList (created by VirtualizedSectionList)

Opening the CurrencyModal B

Failed prop type: The prop `option.alternateText` is marked as required in `OptionRow`, but its value is `undefined`.
    in OptionRow (created by ItemWithSeparator)
    in ItemWithSeparator (created by CellRenderer)
    in div (created by View)
    in View (created by CellRenderer)
    in CellRenderer (created by VirtualizedList)
    in div (created by View)
    in View (created by ScrollView)
    in div (created by View)
    in View (created by ForwardRef)

rameshhpathak · 2021-05-20T13:11:31Z

Hi @Julesssss Done :)

Julesssss

Okay, thanks for the changes! Looking good.

To unblock other IOU issues we'll create a separate issue to A) Modify the prompt logic to occur after a new currency is selected, and B) use the location permission prompt for mobile.

Julesssss · 2021-05-20T14:02:41Z

@rameshhpathak -- could you just run pod install from the ios folder, then commit and push the 'Podfile.lock' changes? The iOS app won't build locally without this change. Thanks!

Julesssss · 2021-05-20T14:49:06Z

Hey @rameshhpathak, just bumping again in the hope you see this today. We'll likely have more conflicts to solve again tomorrow else 🙁

rameshhpathak · 2021-05-20T14:54:43Z

Hi @Julesssss sorry for late reply. I did push it, please take a look

rameshhpathak · 2021-05-20T15:46:29Z

@Julesssss

Julesssss · 2021-05-20T17:18:18Z

Fantastic, thanks again @rameshhpathak. I think we'll have some much smaller follow-up issues here if that's something you're interested in.

rameshhpathak · 2021-05-20T17:52:41Z

@Julesssss Of course! I will be in the look out for it! Thank you for your help!

isagoico · 2021-05-20T22:37:50Z

Not sure if this PR covers the following issue so please let me know if I should open it separately

Currency in LHN does not match the requested currency

Expected Result

Contact will have a badge indicating there is money requested with the correct currency.

Actual Result

Badge is there, but currency does not match. It always displays $

Action Performed

Open staging.expensify.cash and login
Request money to a contact you have messages with
Choose local currency / other than USD

Platform

Issue is reproduced in?

Web ✔️

Notes/Images/Video

iwiznia · 2021-05-20T23:06:50Z

That's known and is going to be fixed here

isagoico · 2021-05-20T23:12:02Z

Great! Will check the PR off then, Thanks 🙇

src/libs/API.js

src/libs/Navigation/AppNavigator/AuthScreens.js

Julesssss · 2021-05-21T10:28:20Z

Thanks @marcaaron, good points. @rameshhpathak would you mind following up with these changes?

rameshhpathak · 2021-05-24T12:54:57Z

Hi @Julesssss, sure! Will send a PR later tonight.

Julesssss · 2021-06-03T14:20:19Z

Hi @rameshhpathak, have you been able to take a look at this?

Install react-native-permissions and Geolocation

3696362

rameshhpathak mentioned this pull request Apr 1, 2021

Create Currency Modal, with geolocation and Location permissions #2128

Closed

5 tasks

rameshhpathak added 5 commits April 1, 2021 10:21

Add currency selection page

2c293e2

Set up currency modal

5875377

Configure pod file and prop type for IOU Amount

50dc09e

Switch library for geolocation to better optimized library

39aeaac

Add mockups for jest

0c00dd8

rameshhpathak marked this pull request as ready for review April 1, 2021 06:55

rameshhpathak requested a review from a team as a code owner April 1, 2021 06:55

MelvinBot requested review from jasperhuangg and removed request for a team April 1, 2021 06:55

rameshhpathak changed the title ~~Install react-native-permissions and Geolocation~~ Create currency modal with react-native-permissions and geolocation Apr 1, 2021

marcaaron assigned rameshhpathak Apr 1, 2021

iwiznia requested a review from Julesssss April 1, 2021 22:42

AndrewGable reviewed Apr 1, 2021

View reviewed changes

jasperhuangg requested changes Apr 2, 2021

View reviewed changes

src/ONYXKEYS.js Outdated Show resolved Hide resolved

src/libs/OptionsListUtils.js Outdated Show resolved Hide resolved

src/pages/iou/steps/IOUAmountPage.js Outdated Show resolved Hide resolved

src/pages/iou/steps/IOUCurrencySelection.js Outdated Show resolved Hide resolved

Remove swift bridging files

b930a2b

rameshhpathak added 2 commits April 2, 2021 09:08

Resolve comments

be12f2a

Merge with master to resolve conflicts

52d245c

Julesssss reviewed Apr 6, 2021

View reviewed changes

android/build.gradle Show resolved Hide resolved

Julesssss reviewed Apr 6, 2021

View reviewed changes

Julesssss requested changes Apr 6, 2021

View reviewed changes

Revert back changes

af1b5c5

Change option row prop validation as per suggestion

13b161d

Julesssss previously approved these changes May 20, 2021

View reviewed changes

Pod install and push changes

b88b3ce

rameshhpathak dismissed Julesssss’s stale review via b88b3ce May 20, 2021 14:51

Julesssss approved these changes May 20, 2021

View reviewed changes

Julesssss merged commit ceb36f2 into Expensify:main May 20, 2021

OSBotify mentioned this pull request May 20, 2021

Deploy Checklist: Expensify.cash 2021-05-19 #3000

Closed

10 tasks

marcaaron reviewed May 21, 2021

View reviewed changes

src/libs/API.js Show resolved Hide resolved

marcaaron reviewed May 21, 2021

View reviewed changes

src/libs/Navigation/AppNavigator/AuthScreens.js Show resolved Hide resolved

This was referenced May 18, 2023

[HOLD for payment 2023-05-25] [$1000] Blue bottom border seperator among currencies is not pressent. #18908

Closed

Refactor iou page by separating each step as individual screen #17964

Merged

eVoloshchak mentioned this pull request Jun 22, 2023

[HOLD for payment 2023-06-19] [$1000] Split Bill preview not showing correct description/currency #19870

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create currency modal with react-native-permissions and geolocation #2194

Create currency modal with react-native-permissions and geolocation #2194

rameshhpathak commented Apr 1, 2021 •

edited

Loading

Julesssss commented Apr 1, 2021

AndrewGable Apr 1, 2021

jasperhuangg Apr 2, 2021

marcaaron May 21, 2021

jasperhuangg left a comment

rameshhpathak commented Apr 2, 2021

AndrewGable commented Apr 5, 2021

shawnborton commented Apr 5, 2021

Julesssss Apr 6, 2021

Julesssss Apr 6, 2021

rameshhpathak Apr 7, 2021

Julesssss Apr 6, 2021

rameshhpathak Apr 7, 2021

Julesssss Apr 6, 2021

Julesssss left a comment

rameshhpathak commented Apr 7, 2021

rameshhpathak commented May 20, 2021

Julesssss commented May 20, 2021

Julesssss commented May 20, 2021

rameshhpathak commented May 20, 2021

Julesssss left a comment

Julesssss commented May 20, 2021 •

edited

Loading

Julesssss commented May 20, 2021

rameshhpathak commented May 20, 2021

rameshhpathak commented May 20, 2021

Julesssss commented May 20, 2021

rameshhpathak commented May 20, 2021

isagoico commented May 20, 2021 •

edited

Loading

iwiznia commented May 20, 2021

isagoico commented May 20, 2021

Julesssss commented May 21, 2021

rameshhpathak commented May 24, 2021

Julesssss commented Jun 3, 2021

		@@ -0,0 +1,4 @@
		//
		// Use this file to import your target's public headers that you would like to expose to Swift.

		const {currencyOptions} = getCurrencyListForSections(this.props.currencyList,
		'');

Create currency modal with react-native-permissions and geolocation #2194

Create currency modal with react-native-permissions and geolocation #2194

Conversation

rameshhpathak commented Apr 1, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Julesssss commented Apr 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasperhuangg left a comment

Choose a reason for hiding this comment

rameshhpathak commented Apr 2, 2021

AndrewGable commented Apr 5, 2021

shawnborton commented Apr 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Julesssss left a comment

Choose a reason for hiding this comment

rameshhpathak commented Apr 7, 2021

rameshhpathak commented May 20, 2021

Julesssss commented May 20, 2021

Julesssss commented May 20, 2021

rameshhpathak commented May 20, 2021

Julesssss left a comment

Choose a reason for hiding this comment

Julesssss commented May 20, 2021 • edited Loading

Julesssss commented May 20, 2021

rameshhpathak commented May 20, 2021

rameshhpathak commented May 20, 2021

Julesssss commented May 20, 2021

rameshhpathak commented May 20, 2021

isagoico commented May 20, 2021 • edited Loading

Currency in LHN does not match the requested currency

Expected Result

Actual Result

Action Performed

Platform

iwiznia commented May 20, 2021

isagoico commented May 20, 2021

Julesssss commented May 21, 2021

rameshhpathak commented May 24, 2021

Julesssss commented Jun 3, 2021

rameshhpathak commented Apr 1, 2021 •

edited

Loading

Julesssss commented May 20, 2021 •

edited

Loading

isagoico commented May 20, 2021 •

edited

Loading