-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: accounting is not enable when connection made from OD #45667
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
Sorry I do not have any recording video for MacOS, IOS, and Android, because I have a billing issue when trying to purchase Scaleway |
@techievivek Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Assigned @carlosmiceli since they are the assigned internal engineer for this issue. |
const [, setEnabledItemState] = useState(enabledItem); | ||
|
||
useEffect(() => { | ||
setEnabledItemState(enabledItem); | ||
}, [enabledItem]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to prevent infinite rendering for the HighlightableMenuItem file
When toggling features inside the More Features tab the enabled item value changed and the highlight property will be true and re-render the WorkspaceInitialPage and the enabled item will be empty again
But when the Accountant tab is enabled the enabled item doesn't get empty again so the highlight property returns true and it causes infinite rendering
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NJ-2020 Could you explain how the setEnabledItemState
is preventing the infinite rendering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the Accountant tab is enabled the enabled item doesn't get empty again so inside the highlight property when we're checking if enabledItem.routeName is equal to the current item routeName it will return true and cause infinite rendering, to avoid that we can re-render the WorkspaceInitialPage by using setEnabledItemState
so the enabledItem will get empty again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding this issue, I don't think force re-render is the correct solution. If the issue is not caused by this PR, we should handle the issue separately and try to find the right root cause.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is not caused by my PR, but just need your thoughts first.
So when the accountant tab is enabled, if we go back from the Workspace clear the cache & restart, and go back to the workspace, the accountant tab is not showing until we go to another page to re-render the WorkspaceInitialPage
This is because here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mollfpr How do you think?
Please let me know your thought
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay.
I understand the issue, but it's not only happening on the accountant tab and I don't think this is the correct solution moreover just a workaround.
So, I think we should do nothing here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mollfpr Ok.
I've removed the code
Thank you
@mallenexpensify Hey Matt, thought I'd get your advice here to make sure it's ok to review this PR already? Considering @NJ-2020 is new and there's no videos, just thought I'd confirm with you first. |
@mollfpr 👀 plz since you're the assigned C+ on the issue. Unsure why you weren't requested for review here.
Yes, we want vids for tests on all platforms (emulators can be used if needed) |
Sorry, I didn't get the notification for the PR. I'll finish the review in few hours. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an issue with toggling off the accounting feature after enabling it. On staging we can still toggle it on/off.
Disconnecting the connections still disabled the toggle too.
Screen.Recording.2024-07-21.at.22.14.47.mp4
src/libs/PolicyUtils.ts
Outdated
@@ -413,6 +413,9 @@ function isPolicyFeatureEnabled(policy: OnyxEntry<Policy>, featureName: PolicyFe | |||
if (featureName === CONST.POLICY.MORE_FEATURES.ARE_TAXES_ENABLED) { | |||
return !!policy?.tax?.trackingEnabled; | |||
} | |||
if (featureName === CONST.POLICY.MORE_FEATURES.ARE_CONNECTIONS_ENABLED) { | |||
return policy?.[featureName] ? !!policy?.[featureName] : !!policy?.connections; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double negation on empty array will return true
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
const [, setEnabledItemState] = useState(enabledItem); | ||
|
||
useEffect(() => { | ||
setEnabledItemState(enabledItem); | ||
}, [enabledItem]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NJ-2020 Could you explain how the setEnabledItemState
is preventing the infinite rendering?
@@ -85,7 +85,7 @@ function WorkspaceMoreFeaturesPage({policy, route}: WorkspaceMoreFeaturesPagePro | |||
const {isSmallScreenWidth} = useWindowDimensions(); | |||
const {translate} = useLocalize(); | |||
const {canUseReportFieldsFeature, canUseWorkspaceFeeds} = usePermissions(); | |||
const hasAccountingConnection = !!policy?.areConnectionsEnabled && !isEmptyObject(policy?.connections); | |||
const hasAccountingConnection = policy?.areConnectionsEnabled ? policy?.areConnectionsEnabled : !isEmptyObject(policy?.connections); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using logical OR is enough here. Is there any difference using the ternary operator here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry my mistake, Done
@@ -225,7 +225,7 @@ function WorkspaceMoreFeaturesPage({policy, route}: WorkspaceMoreFeaturesPagePro | |||
icon: Illustrations.Accounting, | |||
titleTranslationKey: 'workspace.moreFeatures.connections.title', | |||
subtitleTranslationKey: 'workspace.moreFeatures.connections.subtitle', | |||
isActive: !!policy?.areConnectionsEnabled, | |||
isActive: policy?.areConnectionsEnabled ? policy?.areConnectionsEnabled : !isEmptyObject(policy?.connections), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use hasAccountingConnection
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
const isConnectionDataFetchNeeded = | ||
!isOffline && | ||
props.policy && | ||
(props.policy.areConnectionsEnabled ? props.policy.areConnectionsEnabled : !isEmptyObject(props.policy.connections)) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question with the logical OR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@mollfpr This is because we already have connections for our Accountant tab if we do the same thing inside the staging Expensify website it will be the exact same 2024-07-22.12-01-26.mp4 |
Even when there are no connections the toggle is still disabled. You can observed the test on staging that after disconecting the connection, the toggle is not disabled. Screen.Recording.2024-07-22.at.12.20.16.mp4 |
@NJ-2020 I think we should update the disabled property here.
We should disable the toggle if there are connections made before. - disabled: hasAccountingConnection,
+ disabled: !isEmptyObject(policy?.connections), |
@mollfpr Thank you I just pushed a new commit and it resolved the issue |
Sorry above request, accidentally mistaken |
@NJ-2020 No worries! I think the changes look good, I just need to finish the recordings. |
@NJ-2020 Could you complete the recordings on all platforms? It's part of the checklist. Thanks! |
@mollfpr Sorry, I am using Windows right now, and when trying to purchase Scaleway or other platforms I have a billing issue. It's my first PR. Is that okay? |
@NJ-2020 I'm personally okay. Maybe you can record the platform you can on Windows. |
Reviewer Checklist
Screenshots/VideosAndroid: Native45667.Android.mp4Android: mWeb Chrome45667.mWeb-Chrome.mp4iOS: Native45667.iOS.moviOS: mWeb Safari45667.mWeb-Safari.movMacOS: Chrome / Safari45667.Web.mp4MacOS: Desktop45667.Desktop.mp4 |
@carlosmiceli I have completed the reviewer checklist and you can remove the checklist #45667 (comment) so melvin can check the updated checklist. |
Looks like some typescript test is not passing. |
@carlosmiceli But the typescript failing is not from my changes |
@carlosmiceli Any update? |
@NJ-2020 can you try merging main again? I think we had a few issues with performance regressions that I think are fixed now. |
@carlosmiceli Done |
✋ 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/carlosmiceli in version: 9.0.15-0 🚀
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.0.15-9 🚀
|
Details
Fixed Issues
$ #45080
PROPOSAL: #45080 (comment)
Tests
Accounting features are enabled
Offline tests
QA Steps
Accounting features are enabled
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop