Only show "Go to Expensify Classic to manage your settings" for Certinia.#68781
Conversation
…nia. Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2025-08-29.at.00.43.55.movAndroid: mWeb ChromeScreen.Recording.2025-08-29.at.00.43.55.mp4iOS: HybridAppWe can not mock connection on this platform Screen.Recording.2025-08-29.at.00.24.01.mp4iOS: mWeb SafariScreen.Recording.2025-08-29.at.00.16.24.mp4MacOS: Chrome / SafariScreen.Recording.2025-08-28.at.23.42.49.mp4Screen.Recording.2025-08-28.at.23.34.20.mp4MacOS: Desktop |
| */ | ||
| function hasAccountingConnections(policy: OnyxEntry<Policy>) { | ||
| return !isEmptyObject(policy?.connections); | ||
| return !isEmptyObject(policy?.connections) && (!!getCurrentConnectionName(policy) || hasUnsupportedIntegration(policy)); |
There was a problem hiding this comment.
The context of this function is to check whether a connection exists, and we should check hasUnsupportedIntegration only where necessary, rather than making it a default within this function, since this function is used in many places, adding it here could make regressions harder to control.
| return !isEmptyObject(policy?.connections) && (!!getCurrentConnectionName(policy) || hasUnsupportedIntegration(policy)); | |
| return !isEmptyObject(policy?.connections) && (!!getCurrentConnectionName(policy)); |
There was a problem hiding this comment.
I have intentionally made that change because this function is used to disable features when we have an accounting connection which is supported in ND but now we need to also disabled the features/options when we have an unsupported accounting connection.
There was a problem hiding this comment.
Let me check again and make sure that this isn't affecting any other part of the app where we are using hasAccountingConnections.
There was a problem hiding this comment.
I have checked again and I think we should be fine with this change because we are using hasAccountingConnections just to prevent enabling of workspace features when we have a accounting which is supported in ND or we have a unsupported connection.
There was a problem hiding this comment.
Please take a look at these cases that are not included in the description — we don't want to introduce regressions for them
and more .....
There was a problem hiding this comment.
@suneox ^ thats why I added && (!!getCurrentConnectionName(policy) || hasUnsupportedIntegration(policy)); condition, because we want to only show locked icons on QBO, QBD, XERO, NETSUITE, SAGE_INTACCT, financialForce but not on any other connections. Please check and let me know, I'll add that condition back.
There was a problem hiding this comment.
@Krishna2323 From your example mock data:
- At 1:50, we don't have a connection with the value
anyother. - At 2:40, the value is
generic_indirect_connection, which means the user still hasn’t connected to any accounting system — this comes from selecting the Other option in OldDot (don't show lock icon is correct)
Actually, only Certinia can connect to Expensify, as mentioned in this comment.
So the current condition check for hasAccountingConnection excluding generic_indirect_connection, still looks correct.
There was a problem hiding this comment.
@suneox as the comment states, we don’t want to show locked icons when the connection is not one of QBO, QBD, XERO, NETSUITE, SAGE_INTACCT, financialForce. However, the current (on main) function hasAccountingConnections returns !isEmptyObject(policy?.connections), which will also return true for other connections like dynamics or SAP.
We updated it to !isEmptyObject(policy?.connections) && !hasUnsupportedIntegration(policy);, but this will still return true for connections like dynamics or SAP, since !hasUnsupportedIntegration(policy) only excludes generic_indirect_connection. As a result, dynamics or SAP still evaluate to true, which incorrectly shows locked icons.
This behavior doesn’t match the expected outcome mentioned below:
If the connected integration is not one of
QBO, QBD, XERO, NETSUITE, SAGE_INTACCT, financialForce
- Don’t show the locked icon on categories, tags, etc. (because there shouldn’t be anything preventing you from configuring coding on these "indirect" integrations)
- Don’t show the "Go to Expensify Classic to manage your settings" link (for the same reason above)
There was a problem hiding this comment.
@Krishna2323 I have double-checked with the available options to connect accounting from OldDot. I can confirm that although some options cannot complete the accounting setup but the selected value in policy.connections is still returned in NewDot.
This behavior doesn’t match the expected outcome mentioned below:
Therefore, I agree that we should proceed with your previous change to handle this case.
There was a problem hiding this comment.
@suneox updated, please check and let me know if you face any issues. Thanks!
|
|
||
| const onDisabledOrganizeSwitchPress = useCallback(() => { | ||
| if (!hasAccountingConnection) { | ||
| if (!hasReportAccountingConnections) { |
There was a problem hiding this comment.
using the same PolicyAccountingPage.tsx
const hasUnsupportedNDIntegration = !isEmptyObject(policy?.connections) && hasUnsupportedIntegration(policy);
There was a problem hiding this comment.
why we should use hasUnsupportedIntegration here?
There was a problem hiding this comment.
since we will remove the condition outside the hasAccountingConnection function
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
…orce. Signed-off-by: krishna2323 <belivethatkg@gmail.com>
trjExpensify
left a comment
trjExpensify
left a comment
There was a problem hiding this comment.
I raised this bug, agree with what we're fixing. 👍
suneox
left a comment
suneox
left a comment
There was a problem hiding this comment.
I verified the latest changes, and everything works as expected with all unsupported options from OldDot and mock data
Screen.Recording.2025-09-03.at.22.26.45.mp4
| */ | ||
| function hasAccountingConnections(policy: OnyxEntry<Policy>) { | ||
| return !isEmptyObject(policy?.connections); | ||
| return !isEmptyObject(policy?.connections) && (!!getCurrentConnectionName(policy) || hasSupportedOnlyOnOldDotIntegration(policy)); |
There was a problem hiding this comment.
Why are we changing this?
There was a problem hiding this comment.
We now only want to show locked icons and features restriction when the accounting connection is either supported in NewDot or is Certinia (financialForce). Ref: #68562 (comment)
There was a problem hiding this comment.
Ah okay I see getCurrentConnectionName only returns names for connections that are supported in NewDot. Can we add a comment about that? It's not obvious here.
Also, can't we simplify the condition to just this?
| return !isEmptyObject(policy?.connections) && (!!getCurrentConnectionName(policy) || hasSupportedOnlyOnOldDotIntegration(policy)); | |
| return !!getCurrentConnectionName(policy) || hasSupportedOnlyOnOldDotIntegration(policy); |
There was a problem hiding this comment.
updated the comment and simplified the condition. Thanks for the suggestion, it looks cleaner now.
|
|
||
| const onDisabledOrganizeSwitchPress = useCallback(() => { | ||
| if (!hasAccountingConnection) { | ||
| if (!hasReportAccountingConnections) { |
There was a problem hiding this comment.
| if (!hasReportAccountingConnections) { | |
| if (!hasAccountingConnections) { |
Rename this variable please
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
arosiclair
left a comment
arosiclair
left a comment
There was a problem hiding this comment.
Thanks for the changes
|
@Krishna2323 can you try pulling main to fix the perf tests? |
|
I think we're fixed. Merge time? 🎉 |
|
✋ 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/arosiclair in version: 9.2.9-0 🚀
|
|
🚀 Deployed to staging by https://github.com/arosiclair in version: 9.2.11-0 🚀
|
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.2.12-4 🚀
|
5 participants
Explanation of Change
Fixed Issues
$ #68562
PROPOSAL: #68562 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)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_native.mp4
Android: mWeb Chrome
android_chrome.mp4
iOS: Native
ios_native.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web_chrome.mp4
MacOS: Desktop
desktop_app.mp4