Add new capability to set tracking rate and unit by marktoman · Pull Request #6839 · Expensify/App

marktoman · 2021-12-20T18:26:22Z

Details

Fixed Issues

$ #6213

Tests and QA Steps

Action 1

Steps

Click the profile icon
Go to Workspace

Result

The menu item is named "Reimburse expenses"

Action 2

Steps

Click the profile icon
Go to Workspace
Go to Reimburse expenses
Change Rate and Unit
Reload

Result

Values are preserved

Action 3

Steps

Repeat Action 2
Log out and delete local storage
Log in and go to Reimburse expenses

Result

Values are preserved

Action 4

Steps

Log in to an account that has no custom Unit or Rate set
Go to Reimburse expenses

Result

Miles is the value in Unit

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

deetergp · 2021-12-28T18:40:45Z

Hey @marktoman 👋 Just want to point out there's a merge conflict and I'm waiting on testing instructions. Once you've added those I will review it. I also know it's the holidays, so no worries if you don't get to it till afterwards.

trjExpensify · 2022-01-05T10:24:58Z

Hi @marktoman - happy new year! Can you provide us with an ETA on fixing the merge conflict and adding the testing steps for @deetergp, please? It would be great to have this squared away this week if we can!

…t-capability

marktoman · 2022-01-07T13:51:12Z

Happy new year to you guys as well!

I have updated the instruction and QA steps. Unfortunately, it does not build on windows for some reason, so I am not able to provide more platforms.

deetergp · 2022-01-13T01:51:57Z

Heya Mark, thank you for adding the instructions and screenshots, it makes life easier for everyone.

Going through the testing steps, I'm experiencing issues with the settings persistence. If I Change the rate to 68 and units to kilometers the settings will persist after page refreshes. But if I log out and then back in and go back to the settings, the rate defaults to 0 and the units back to Miles. This is on web. The results are the same if I clear out local storage between logging out and then back in.

marktoman · 2022-01-14T14:44:15Z

Hi Scott, thank you for the review. I can replicate the problem.

The issue is caused by withFullPolicy not calling Policy.loadFullPolicy when you log in. The following check returns false: !isPreviousRouteInSameWorkspace(currentRoute.name, policyID).

All other scenarios seem to work, e.g.: Log out & in > refresh the page > navigate to Reimburse expenses. (It fetches the latest values and displays them correctly.)

Do you think I should find a workaround like calling Policy.loadFullPolicy directly from Reimburse expenses, or should this be somehow solved in withFullPolicy itself, given the name does not represent its behavior for this scenario?

deetergp · 2022-01-18T20:37:46Z

@marktoman Great questions! Since it looks like you've got some merge conflicts around Policy.js, let's resolve those first and re-test to see we don't get lucky and fix our problem.

…t-capability

marktoman · 2022-01-19T13:49:20Z

@deetergp Merged and tested again — sadly no luck.

…t-capability

marktoman · 2022-01-20T20:11:49Z

@deetergp I have made it work the following way:

Set a flag in SignInRedirect.js > clearStorageAndRedirect > Onyx.clear().then: Onyx.merge(ONYXKEYS.SESSION, {myFlag: true});
In WithFullPolicy, changed the condition to: ... (props.session.myFlag || !isPreviousRouteInSameWorkspace(currentRoute.name, policyID))
If true, reset: Onyx.merge(ONYXKEYS.SESSION, {myFlag: false});

…t-capability

marktoman · 2022-01-23T12:50:22Z

Hi Scott, curious about your opinion. I consider the second solution cleaner because it does not require adding sessions to withFullPolicy and solves the general policy problem for future use cases as well. Both solutions work.

deetergp

Overall it looks good and passes tests, thanks for sorting it out. I had a few minor requests mostly around variable declarations that only get used once and are unnecessary.

deetergp · 2022-01-28T22:57:34Z

+            updateLocalPolicyValues(policyID, {customUnit: localCustomUnit});
+        }).catch(() => {
+            // Show the user feedback
+            const errorMessage = Localize.translateLocal('workspace.editor.genericFailureMessage');


The errorMessage variable isn't really necessary, just call Localize.translateLocal inside the growl error.

deetergp · 2022-01-28T23:00:47Z

+ * @param {Object} values
+ */
+function setCustomUnit(policyID, values) {
+    const payload = {


No real need for this variable, just define this object inside the call to API.Policy_CustomUnit_Update.

deetergp · 2022-01-28T23:01:22Z

+                throw new Error();
+            }
+
+            const localCustomUnit = {


Unnecessary variable, define in the function call.

deetergp · 2022-01-28T23:02:19Z

+ * @param {Object} values
+ */
+function setCustomUnitRate(policyID, customUnitID, values) {
+    const payload = {


Unnecessary variable declaration

deetergp · 2022-01-28T23:02:25Z

+                throw new Error();
+            }
+
+            const localCustomUnitRate = {


Unnecessary variable declaration

deetergp · 2022-01-28T23:02:31Z

+            updateLocalPolicyValues(policyID, {customUnit: {rate: localCustomUnitRate}});
+        }).catch(() => {
+            // Show the user feedback
+            const errorMessage = Localize.translateLocal('workspace.editor.genericFailureMessage');


Unnecessary variable declaration

deetergp · 2022-01-28T23:16:38Z

+
+        this.setState({rateValue: numValue.toString()});
+
+        const values = {


You only use this once, you don't need it.

deetergp · 2022-01-28T23:16:49Z

+    setUnit(value) {
+        this.setState({unitValue: value});
+
+        const values = {


deetergp · 2022-01-29T00:44:22Z

Oh, and one other thing, let's make sure we are showing the second 0 when displaying the rates. 3.50 showed up as 3.5

deetergp

Looks good. Thanks for the changes!

deetergp · 2022-02-03T19:46:12Z

@marktoman Your changes look good, but there's one slight snag: two of your four most recent commits (04bac5c & 93a37f2) contain unverified signatures which prevent me from being able to merge it.

I think the right way to fix this will be to do the following (I HIGHLY recommend testing it on a copy of your branch first just to be safe):

Make a backup branch!
Do a hard reset back to 1b398a5, your last commit before these changes
Merge main back into your branch with a signed & verified account
Cherry pick 0857ed6 from your backup branch (You shouldn't need to CP 56832ee)
Force push the changes.

Because the changes are after the last review, it shouldn't mess with the PR history adversely.

…t-capability

marktoman · 2022-02-03T20:03:54Z

@deetergp Thank you for all your help on this issue!

deetergp

Looks great! Thanks for sticking with it, @marktoman!

OSBotify · 2022-02-03T22:16:44Z

✋ 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-02-04T12:23:19Z

🚀 Deployed to staging by @deetergp in version: 1.1.36-0 🚀

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

OSBotify · 2022-02-07T18:50:19Z

🚀 Deployed to production by @roryabraham in version: 1.1.36-0 🚀

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

trjExpensify · 2022-02-07T19:41:41Z

Added a comment to the linked issue here, but I think we're displaying the decimals in the wrong place in the workspace editor setting.

iwiznia · 2022-02-17T16:20:36Z

+            kilometers: '',
+            miles: '',
            viewAllReceipts: 'Ver todos los recibos',
            reimburseReceipts: 'Reembolsar recibos',
+            trackDistance: '',
+            trackDistanceCopy: '',
+            trackDistanceRate: '',
+            trackDistanceUnit: '',


This is wrong, you need to complete the translations for this language.
Since it was not done, it generated a few alerts. I am going to post them in the original issue so you can get them fixed @marktoman.
@deetergp you should've not merged this

Lo siento! 😬 Not sure how I missed that but will get some translations to Mark Toman, so he can add them.

It looks like the translations have already been added in main

Is that all that was missing to fix those BugBot issues?

Oh did not see that, I just saw the bugbot issues and assumed it was still happening. Curious where were they added?
And yes, that's all we need to do to fix the issues.

I'll close them as my penance. The changes were added in this commit by @PrashantMangukiya, whom I now owe a 🍺.

Add new capability to set tracking rate and unit

da99214

marktoman requested a review from a team as a code owner December 20, 2021 18:26

MelvinBot requested review from deetergp and removed request for a team December 20, 2021 18:26

marktoman added 3 commits January 6, 2022 21:04

Merge branch 'main' into add-tracking-unit-capability

ce484ac

Merge branch 'main' of github.com:expensify/App into add-tracking-uni…

123cfb5

…t-capability

Update component names

527c997

Merge branch 'main' of github.com:expensify/App into add-tracking-uni…

b4110db

…t-capability

Merge branch 'main' of github.com:expensify/App into add-tracking-uni…

98e14ab

…t-capability

marktoman added 2 commits January 23, 2022 13:29

Merge branch 'main' of github.com:expensify/App into add-tracking-uni…

fcdebc2

…t-capability

Distinguish if policy is from full or summary

1b398a5

trjExpensify mentioned this pull request Jan 25, 2022

[HOLD for payment 2022-03-02] [$250] Add the ability to configure the distance tracking rate and unit on the Free Plan #6213

Closed

deetergp requested changes Jan 28, 2022

View reviewed changes

deetergp previously approved these changes Feb 3, 2022

View reviewed changes

marktoman added 2 commits February 3, 2022 20:57

Merge branch 'main' of github.com:expensify/App into add-tracking-uni…

2277fee

…t-capability

Remove single-use variables, improve rate display

78007a2

marktoman dismissed deetergp’s stale review via 78007a2 February 3, 2022 20:00

deetergp approved these changes Feb 3, 2022

View reviewed changes

deetergp merged commit 16d38de into Expensify:main Feb 3, 2022

OSBotify mentioned this pull request Feb 4, 2022

Deploy Checklist: New Expensify 2022-02-04 #7564

Closed

22 tasks

kbecciv mentioned this pull request Feb 4, 2022

[HOLD for payment 2022-05-20] [$4,000] Web - Workspace- New capability to set tracking rate and unit persists after page refresh #7573

Closed

iwiznia reviewed Feb 17, 2022

View reviewed changes

iwiznia mentioned this pull request Feb 17, 2022

[HOLD for payment 2022-03-02] Workspace- Track distance part of the section is not translated into Spanish #7686

Closed

fedirjh mentioned this pull request May 23, 2023

[HOLD for payment 2023-05-22] [$1000] The placeholder of rate doesn't dynamically update on one platform when default currency is changed from the other platform (works well with the rate input and the unit) #18206

Closed

6 tasks


		this.setState({rateValue: numValue.toString()});

		const values = {

Conversation

marktoman commented Dec 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests and QA Steps

Action 1

Steps

Result

Action 2

Steps

Result

Action 3

Steps

Result

Action 4

Steps

Result

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Uh oh!

deetergp commented Dec 28, 2021

Uh oh!

trjExpensify commented Jan 5, 2022

Uh oh!

marktoman commented Jan 7, 2022

Uh oh!

deetergp commented Jan 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marktoman commented Jan 14, 2022

Uh oh!

deetergp commented Jan 18, 2022

Uh oh!

marktoman commented Jan 19, 2022

Uh oh!

marktoman commented Jan 20, 2022

Uh oh!

marktoman commented Jan 23, 2022

Uh oh!

deetergp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deetergp commented Jan 29, 2022

Uh oh!

deetergp left a comment

Choose a reason for hiding this comment

Uh oh!

deetergp commented Feb 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marktoman commented Feb 3, 2022

Uh oh!

deetergp left a comment

Choose a reason for hiding this comment

Uh oh!

OSBotify commented Feb 3, 2022

Uh oh!

OSBotify commented Feb 4, 2022

marktoman commented Dec 20, 2021 •

edited

Loading

deetergp commented Jan 13, 2022 •

edited

Loading

deetergp commented Feb 3, 2022 •

edited

Loading

deetergp Feb 17, 2022 •

edited

Loading