Refactor GetLocalCurrency #9607

luacmartins · 2022-06-28T19:21:24Z

Details

Creates OpenApp and ReconnectApp
Removes usages of fetchLocalCurrency from App init
Removes usages of getCurrencyList since we only want to fetch it on OpenApp

Holding on #9560 and https://github.com/Expensify/Web-Expensify/pull/34151 hitting prod

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/213883

Tests

Pull the changes from https://github.com/Expensify/Web-Expensify/pull/34151
Open the Network tab in the browser console.
Clear cache and browsing history for the App.
Sing in
Verify that OpenApp is fired and that the localCurrencyCode and currencyList are returned
Verify that those values are set in the personalDetails and currencyList keys
Navigate to + > Request money
Verify that your local currency shows up
Click on the currency symbol and verify that you can see the currency list.
Set the App to offline and back online in the browser console.
Verify that the ReconnectApp API request was triggered.

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Clear cache and browsing history for the App.
Sing in
Navigate to + > Request money
Verify that your local currency shows up
Click on the currency symbol and verify that you can see the currency list.
Set the App to offline and back online in the browser console.
Repeat steps 3-5

Verify that no errors appear in the JS console

Screenshots

Web

web.mov

Mobile Web

mweb.mov

Desktop

desktop.mov

iOS

ios.mov

Android

android.mov

melvin-bot · 2022-06-28T19:21:30Z

Looks like you modified deprecatedAPI.js! To be clear, you should not be adding any code to this file.

Instead, all new API commands should use API.js, and follow our guidelines for writing new API commands.

Unsure if your change is okay? Drop a note in #expensify-open-source!

PauloGasparSv · 2022-06-29T16:52:19Z

@luacmartins can I test this despite the last comment in here https://github.com/Expensify/Web-Expensify/pull/34151 and the fact it is on hold too?

luacmartins · 2022-06-29T16:59:05Z

I'd wait since this PR might change a bit based on that comment.

mountiny · 2022-07-12T17:55:16Z

src/libs/actions/PersonalDetails.js

@@ -323,7 +310,6 @@ function fetchLocalCurrency() {
        .then((data) => {
            currency = data.currency;
        })
-        .then(getCurrencyList)
        .then(() => {
            Onyx.merge(ONYXKEYS.MY_PERSONAL_DETAILS, {localCurrencyCode: currency});


@luacmartins What I mentioned here, this is how we currently store and merge the data about localCurrencyCode

Yea, I'm planning on changing this PR based on the changes here

luacmartins · 2022-07-13T00:32:45Z

@mountiny @PauloGasparSv this is ready for review! We are still holding on Web-E hitting prod and the changes to the Onyx key.

mountiny

In general, looks good to me, but there is one unit test, which was implemented for the currency list localization testing.

App/tests/unit/CurrencySymbolUtilsTest.js

Lines 4 to 8 in c93b4cb

    
           // This file can get outdated. In that case, you can follow these steps to update it: 
        
           // - in src/libs/API.js 
        
           // - call: GetCurrencyList().then(data => console.log(data.currencyList)); 
        
           // - copy the json from console and format it to valid json using some external tool 
        
           // - update currencyList.json

The instruction on how to obtain new JSON of the currencies uses our API so we should find a way how to keep this possible to be completed.

I am not sure how though as if we remove the API method from App then they can call it as they need authToken even if we keep the MobileConstants endpoint in the backend (or I mean they could use curl, and add their authToken but that would be quite cumbersome.

The best might be to update the instructions so they ask in the expensify-open-source channel so someone internal helps them retrieve the JSON. What do you think? Or just keep that change internal 🤷

luacmartins · 2022-07-13T18:25:28Z

Thanks for the review @mountiny! I updated the instructions. I think contributors can get the updated list from the OpenApp command right?

mountiny

@luacmartins Good call, yeah they should!

Thanks for the changes, just one typo.

tests/unit/CurrencySymbolUtilsTest.js

mountiny

LGTM! Thanks for the changes!

PauloGasparSv · 2022-07-13T19:06:34Z

I'm still going to take a little time to review this to make sure this error is my fault and proceed. But right now It's giving me an error when I click to request money right after logging. If I refresh the error stops.

PauloGasparSv · 2022-07-13T20:34:12Z

Still have the same error! Can you help me on this @luacmartins?
Did it on the Incognito mode this time but also tested on a normal Chrome session

Screen.Recording.2022-07-13.at.17.28.58.mov

luacmartins · 2022-07-13T21:04:09Z

@PauloGasparSv the problem is that #9560 is not merged yet. We set selectedCurrencyCode here, but it expects localCurrencyCode to be in the myPersonalDetails Onyx key. However, that key is now in personalDetails. Once that PR is merged it should fix this issue.

A side note is that this default prop is doing nothing since other keys in props.iou are defined. We should probably fix that.

PauloGasparSv

I see! Nice : )
Approving it for now since everything LGTM!
But let's test it again when that P.R. is merged

luacmartins · 2022-07-16T02:43:04Z

#9560 merged! Retested and all is good. Removing hold and merging!

melvin-bot · 2022-07-16T02:43:28Z

@luacmartins looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

luacmartins · 2022-07-16T02:43:47Z

Tests passed! Removing emergency label!

OSBotify · 2022-07-16T02:50:02Z

✋ 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 · 2022-07-25T17:21:41Z

🚀 Deployed to production by @luacmartins in version: 1.1.85-8 🚀

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

luacmartins added 2 commits June 28, 2022 12:59

create openApp

09e87e2

rm usage of getCurrencyList

2b281f6

luacmartins self-assigned this Jun 28, 2022

luacmartins added 3 commits June 28, 2022 13:29

rm unused imports

425480b

create reconnectApp

2dfa6f0

rm Mobile_GetConstants

1dac239

luacmartins changed the title ~~Refactor GetLocalCurrency~~ [Hold Web-E #34151] Refactor GetLocalCurrency Jun 28, 2022

luacmartins marked this pull request as ready for review June 28, 2022 23:32

luacmartins requested a review from a team as a code owner June 28, 2022 23:32

melvin-bot bot requested review from PauloGasparSv and removed request for a team June 28, 2022 23:32

luacmartins closed this Jun 29, 2022

luacmartins reopened this Jun 29, 2022

mountiny reviewed Jul 12, 2022

View reviewed changes

luacmartins force-pushed the cmartins-refactorGetLocalCurrency branch from fa575db to 1dac239 Compare July 13, 2022 00:23

merge main

7e45467

luacmartins changed the title ~~[Hold Web-E #34151] Refactor GetLocalCurrency~~ [Hold Web-E #34151 and App #9560] Refactor GetLocalCurrency Jul 13, 2022

mountiny requested changes Jul 13, 2022

View reviewed changes

mountiny mentioned this pull request Jul 13, 2022

[Refactor] Create new OpenIOUModal action instead of GetLocalCurrency #9840

Merged

93 tasks

luacmartins changed the title ~~[Hold Web-E #34151 and App #9560] Refactor GetLocalCurrency~~ [Hold App #9560] Refactor GetLocalCurrency Jul 13, 2022

luacmartins added 2 commits July 13, 2022 11:10

resolve conflicts

b69f0ec

update currencyList tests

1c8d490

mountiny reviewed Jul 13, 2022

View reviewed changes

tests/unit/CurrencySymbolUtilsTest.js Outdated Show resolved Hide resolved

fix typo

10406bf

mountiny approved these changes Jul 13, 2022

View reviewed changes

PauloGasparSv approved these changes Jul 13, 2022

View reviewed changes

luacmartins mentioned this pull request Jul 13, 2022

Fix broken defaultProps in IOUAmountPage #9889

Closed

Merge branch 'main' into cmartins-refactorGetLocalCurrency

94d728e

luacmartins changed the title ~~[Hold App #9560] Refactor GetLocalCurrency~~ Refactor GetLocalCurrency Jul 16, 2022

luacmartins merged commit 1fc51f4 into main Jul 16, 2022

luacmartins deleted the cmartins-refactorGetLocalCurrency branch July 16, 2022 02:43

melvin-bot bot added the Emergency label Jul 16, 2022

luacmartins removed the Emergency label Jul 16, 2022

OSBotify mentioned this pull request Jul 18, 2022

Deploy Checklist: New Expensify 2022-07-18 #9970

Closed

79 tasks

kbecciv mentioned this pull request Jul 19, 2022

Web - Request Money - App crashed after return to online. #9992

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor GetLocalCurrency #9607

Refactor GetLocalCurrency #9607

luacmartins commented Jun 28, 2022 •

edited

Loading

melvin-bot bot commented Jun 28, 2022

PauloGasparSv commented Jun 29, 2022

luacmartins commented Jun 29, 2022

mountiny Jul 12, 2022

luacmartins Jul 12, 2022

luacmartins commented Jul 13, 2022

mountiny left a comment •

edited

Loading

luacmartins commented Jul 13, 2022

mountiny left a comment

mountiny left a comment

PauloGasparSv commented Jul 13, 2022

PauloGasparSv commented Jul 13, 2022 •

edited

Loading

luacmartins commented Jul 13, 2022 •

edited

Loading

PauloGasparSv left a comment

luacmartins commented Jul 16, 2022

melvin-bot bot commented Jul 16, 2022

luacmartins commented Jul 16, 2022

OSBotify commented Jul 16, 2022

OSBotify commented Jul 25, 2022

	// This file can get outdated. In that case, you can follow these steps to update it:
	// - in src/libs/API.js
	// - call: GetCurrencyList().then(data => console.log(data.currencyList));
	// - copy the json from console and format it to valid json using some external tool
	// - update currencyList.json

Refactor GetLocalCurrency #9607

Refactor GetLocalCurrency #9607

Conversation

luacmartins commented Jun 28, 2022 • edited Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

melvin-bot bot commented Jun 28, 2022

PauloGasparSv commented Jun 29, 2022

luacmartins commented Jun 29, 2022

mountiny Jul 12, 2022

Choose a reason for hiding this comment

luacmartins Jul 12, 2022

Choose a reason for hiding this comment

luacmartins commented Jul 13, 2022

mountiny left a comment • edited Loading

Choose a reason for hiding this comment

luacmartins commented Jul 13, 2022

mountiny left a comment

Choose a reason for hiding this comment

mountiny left a comment

Choose a reason for hiding this comment

PauloGasparSv commented Jul 13, 2022

PauloGasparSv commented Jul 13, 2022 • edited Loading

luacmartins commented Jul 13, 2022 • edited Loading

PauloGasparSv left a comment

Choose a reason for hiding this comment

luacmartins commented Jul 16, 2022

melvin-bot bot commented Jul 16, 2022

luacmartins commented Jul 16, 2022

OSBotify commented Jul 16, 2022

OSBotify commented Jul 25, 2022

luacmartins commented Jun 28, 2022 •

edited

Loading

mountiny left a comment •

edited

Loading

PauloGasparSv commented Jul 13, 2022 •

edited

Loading

luacmartins commented Jul 13, 2022 •

edited

Loading