Tag settings: Billable #47019 #48184
Conversation
|
PR is working correctly like it should, there are 3 issues to clarify To clarify:
|
|
After change in 
|
|
Also @shawnborton if you could take a peek if the switches look good 👀🙏 |
|
Design-wise this looks okay. Why does the toggle become locked out of curiosity? |
|
I'm not sure about the top switch because I didn't get into it; this PR is only about 'Track billable expenses' one |
|
Cool cool, well that toggle looks good to me :) |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid.mp4Android: mWeb Chromemchrome.mp4iOS: Nativeios.moviOS: mWeb Safarimsafari.movMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
👍
👍
Right, we really meant, "toggled off". And no, it should never be "disabled". |
|
Also, on the request of @WojtekBoman I quickly fixed #48266 bug, it required changing list items' order, but doesn't affect this PR :) Screen.Recording.2024-08-30.at.10.41.52.mov |
…-fork into Guccio/47019-TagSettingsBillable
| if (policy?.disabledFields?.defaultBillable) { | ||
| return policy?.pendingFields?.disabledFields ?? policy?.pendingFields?.defaultBillable; | ||
| } | ||
| return policy?.pendingFields?.disabledFields; |
There was a problem hiding this comment.
Could we add some comment here to explain the slightly confusing interaction between disabledFields.defaultBillable and defaultBillable? In particular, it's hard to see why we would fallback to a pendingFields.defaultBillable.
There was a problem hiding this comment.
Just added, please take a peek if it's clear
There was a problem hiding this comment.
I was looking more for something like "why" it works this way. I am not sure if I understood why we have a billableExpensesPending() method instead of checking for our possible "pending" situations.
To give some explanation of the feature itself:
if we ever have policy.disabledFields.defaultBillable that means the feature is disabled entirely. If we have policy.defaultBillable === true then policy.disabledFields.defaultBillable must be false and that means we will always "default to billable". If policy.defaultBillable === false then we will default to "non-billable".
After explaining that, does it make sense to have a fallback to policy?.pendingFields?.defaultBillable when we are "disabled"? I would think not because the only value that matters is the policy.pendingFields.disabledFields.defaultBillable for that particular case.
There was a problem hiding this comment.
So I think the logic would be more like:
// The field is disabled - only show the "pending" for the disabled pending action because the defaultBillable state is not relevant in this case.
if (policy.disabledFields.defaultBillable) {
return policy.pendingFields.disabledFields;
}
// Otherwise, we are changing the `defaultBillable` so look at this field only.
return policy.pendingFields.defaultBillable;WDYT?
|
Did not know that, @marcaaron asked for it in slack. Of course 2 translators means 2 different translations. |
Co-authored-by: Ionatan Wiznia <iwiznia@hotmail.com>
Co-authored-by: Ionatan Wiznia <iwiznia@hotmail.com>
Co-authored-by: Marc Glasser <marc.aaron.glasser@gmail.com>
|
No problem, I just added requested changes, @marcaaron they |
…-fork into Guccio/47019-TagSettingsBillable
| // }, | ||
| // ], | ||
| // [searchAdvancedFilters, translate, cardList, taxRates, personalDetails, reports], | ||
| // ); |
There was a problem hiding this comment.
Hmm, what is happening here? 🤔
There was a problem hiding this comment.
Ah yes, like I mentioned above this is a leftover after my previous PR; I was resolving conflicts at the Friday evening and left it by mistake so I'm deleting it here as the first occasion 😅
| if (policy?.disabledFields?.defaultBillable) { | ||
| return policy?.pendingFields?.disabledFields ?? policy?.pendingFields?.defaultBillable; | ||
| } | ||
| return policy?.pendingFields?.disabledFields; |
There was a problem hiding this comment.
I was looking more for something like "why" it works this way. I am not sure if I understood why we have a billableExpensesPending() method instead of checking for our possible "pending" situations.
To give some explanation of the feature itself:
if we ever have policy.disabledFields.defaultBillable that means the feature is disabled entirely. If we have policy.defaultBillable === true then policy.disabledFields.defaultBillable must be false and that means we will always "default to billable". If policy.defaultBillable === false then we will default to "non-billable".
After explaining that, does it make sense to have a fallback to policy?.pendingFields?.defaultBillable when we are "disabled"? I would think not because the only value that matters is the policy.pendingFields.disabledFields.defaultBillable for that particular case.
| if (policy?.disabledFields?.defaultBillable) { | ||
| return policy?.pendingFields?.disabledFields ?? policy?.pendingFields?.defaultBillable; | ||
| } | ||
| return policy?.pendingFields?.disabledFields; |
There was a problem hiding this comment.
So I think the logic would be more like:
// The field is disabled - only show the "pending" for the disabled pending action because the defaultBillable state is not relevant in this case.
if (policy.disabledFields.defaultBillable) {
return policy.pendingFields.disabledFields;
}
// Otherwise, we are changing the `defaultBillable` so look at this field only.
return policy.pendingFields.defaultBillable;WDYT?
All good, thanks. Yes, please next time leave either a note on the PR that the translations were approved, or tag the PR reviewers in Slack on the thread, or have the translations reviewer on the PR itself. Any of those would work. Thanks! |
I left both values to check because both are edited, so I wanted to be sure both are not pending. Moreover I think it looks a bit more understandable for someone without context why we check them both and not only one, but I'm not strongly against change. If you think it would be cleaner with only one check I can change it in no time ;) |
Co-authored-by: Marc Glasser <marc.aaron.glasser@gmail.com>
Your reasoning is fine. I think there's enough context here now in case someone is really confused in the future 😄 |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/marcaaron in version: 9.0.29-0 🚀
|
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.0.29-12 🚀
|
Details
This PR adds new switch do
Tag settingsthat allows user to easily enable/disable tracking billing expenses:It also changes
Tagsdescription in Workspace settings and fixes some bugs with alignment and API props.Fixed Issues
$ #47019
PROPOSAL:
Tests
Track billable expensescouple times -DisablePolicyBillableExpensesonoffandSetPolicyBillableModeononshould be visible in network debugger, switch should stay on\off after changingTrack billable expensesswitch should grey out after clicking (but not without click)Offline tests
N/A
QA Steps
N/A
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android.mov
Android: mWeb Chrome
mWeb-android.mov
iOS: Native
ios.mov
iOS: mWeb Safari
mWeb-ios.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov