Closed IuliiaHerets closed 1 week ago
Triggered auto assignment to @alexpensify (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.
We think that this bug might be related to #wave-control
@alexpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors
Edited by proposal-police: This proposal was edited at 2024-09-18 16:04:34 UTC.
The category title link displays "undefined settings."
undefined
for the QBO
when using desktop because the quickbooksDesktop
option is not added to the CONNECTIONS
names list or the routes list.
https://github.com/Expensify/App/blob/f2a4b87675eccb73c143c8b217a90884bc3ff3d9/src/CONST.ts#L2275-L2287Add QuickBooks Desktop alongside other supported connections from this list: https://github.com/Expensify/App/blob/f2a4b87675eccb73c143c8b217a90884bc3ff3d9/src/CONST.ts#L2287-L2296
Update this condition to support QuickBooks Desktop
updated other hardcoded connection objects across the project to include also the QuickBooks Desktop
Another solution would be to avoid displaying the categories below are imported from...
message if the connection name is undefined
. If the connection is undefined
, it indicates that it's not a supported connection.This can be achieved by changing this condition:
https://github.com/Expensify/App/blob/4f695c627e8d0125a3e9698255f5961039a318e0/src/pages/workspace/categories/WorkspaceCategoriesPage.tsx#L283 to check for currentConnectionName
instead of checking the length of policy?.connections
, as this list could contain unsupported connections.
This change should also be applied here, here and here
OR
Instead of displaying undefined
for unsupported connections, we can still retrieve the connection name regardless of whether it is supported, as Tom mentioned here .
To achieve this, we have two options:
CONST.POLICY.CONNECTIONS.NAME_USER_FRIENDLY
, which contains both supported and unsupported connection names, and remove CONST.POLICY.CONNECTIONS.NAME
:function getCurrentConnectionName(policy?: Policy): string | undefined {
if (!policy?.connections) {
return undefined;
}
const connectionNames = CONST.POLICY.CONNECTIONS.NAME_USER_FRIENDLY;
for (const key of Object.keys(connectionNames)) {
if (policy.connections[key]) {
return connectionNames[key];
}
}
return undefined;
}
{hasUnsupportedNDIntegration && !hasSyncError && (
<FormHelpMessage
isError
shouldShowRedDotIndicator
>
<Text style={[{color: theme.textError}]}>
{translate('workspace.accounting.unSupportedIntegration')}
<TextLink
onPress={() => {
Link.openOldDotLink(CONST.OLDDOT_URLS.POLICY_CONNECTIONS_URL(policyID));
}}
>
{translate('workspace.accounting.manageYourSettings')}
</TextLink>
</Text>
</FormHelpMessage>
)}
note: we might not need to warp it with FormHelpMessage
as the user didn't make anything wrong ( the connection is not supported form our side), so it shouldn't be shown as error, we can just show the TextLink
for without wrapping it.
Edited by proposal-police: This proposal was edited at 2024-09-24 09:51:46 UTC.
function getCurrentConnectionName(policy: Policy | undefined, includeUnsupportedConnection = false): string | undefined {
const accountingIntegrations = Object.values(CONST.POLICY.CONNECTIONS.NAME);
if (includeUnsupportedConnection) {
accountingIntegrations.push(...Object.values(CONST.POLICY.UNSUPPORTED_CONNECTIONS.NAME));
}
const connectionKey = accountingIntegrations.find((integration) => !!policy?.connections?.[integration]);
return connectionKey ? CONST.POLICY.CONNECTIONS.NAME_USER_FRIENDLY[connectionKey] : undefined;
}
By introducing a includeUnsupportedConnection
param, we can make sure our change in this PR does not break other logics.
const currentConnectionName = PolicyUtils.getCurrentConnectionName(policy, true);
hasSyncError
in:
https://github.com/Expensify/App/blob/ace39e86b7f6a5015f1cc5722f8b739ff99ebab9/src/pages/workspace/categories/WorkspaceCategoriesPage.tsx#L74 const [connectionSyncProgress] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY_CONNECTION_SYNC_PROGRESS}${policy?.id}`);
const isSyncInProgress = isConnectionInProgress(connectionSyncProgress, policy);
const hasSyncError = PolicyUtils.hasSyncError(policy, isSyncInProgress);
The text "The categories below are imported from your Quickbooks Desktop settings" should not be displayed if the connection encountered an error. We will use hasSyncError
to decide whether we should display that text or not.
{!hasSyncError && isConnectedToAccounting ? (
If the connection is not supported and successful.
if the connection is not supported and there is an error.
To do it, add another logic to handle that case to: https://github.com/Expensify/App/blob/1c39bd2bd896a573373b7ab357edc42e6ea48246/src/pages/workspace/accounting/PolicyAccountingPage.tsx#L495
{hasUnsupportedNDIntegration && !hasSyncError && (
<FormHelpMessage shouldShowRedDotIndicator={false}>
<Text>
<TextLink
onPress={() => {
// Go to Expensify Classic.
Link.openOldDotLink(CONST.OLDDOT_URLS.POLICY_CONNECTIONS_URL(policyID));
}}
>
{"Go to Expensify Classic to manage your settings"}
</TextLink>
</Text>
</FormHelpMessage>
)}
Remove hasSyncError
conditions in:
https://github.com/Expensify/App/blob/1c39bd2bd896a573373b7ab357edc42e6ea48246/src/pages/workspace/accounting/PolicyAccountingPage.tsx#L462
Remove !(hasUnsupportedNDIntegration && hasSyncError)
in:
https://github.com/Expensify/App/blob/1c39bd2bd896a573373b7ab357edc42e6ea48246/src/pages/workspace/accounting/PolicyAccountingPage.tsx#L380
Update hasUnsupportedNDIntegration
variable to:
const hasUnsupportedNDIntegration = !isEmptyObject(policy?.connections) && PolicyUtils.hasUnsupportedIntegration(policy, accountingIntegrations);
in case "the connection is not supported and successful".
Job added to Upwork: https://www.upwork.com/jobs/~021837252793117015940
Triggered auto assignment to Contributor-plus team member for initial proposal review - @brunovjk (External
)
@brunovjk when you get a chance, can you please review the proposals? Thanks!
@brunovjk when you get a chance, can you please review the proposals? Thanks!
Sure!
I don't think QBD is implemented at all. cc @lakchote @dylanexpensify
Appreciate the insights @shubham1206agra! I will hold off on the proposal review until the last comment has been addressed :D
While adding QuickBooks Desktop (QBD
) would resolve the issue for that connection, this "undefined settings" error may still occur with other unsupported connections. To ensure broader coverage and prevent similar issues, I recommend considering the alternative solution proposed, which addresses undefined connections more comprehensively.
I'm confused, this GH is about QBO, not QBD. Why did we pivot to talk about QBD?
I don't understand why we're talking about QBD either here. We're talking about QBO. QBD is not yet implemented in NewDot.
It seems the confusion stems from the fact that this issue was initially labeled as QBO, but the actual functionality being tested in the video and steps involves QBD (QuickBooks Desktop), not QBO (QuickBooks Online).
Here's a summary:
I reviewed both proposals (using the alternative solution from the first one), and they are quite similar. While the first proposal from @abzokhattab suggests "avoiding the display of the 'categories below are imported from...' message if the connection name is undefined," I believe the second one from @mkzie2 —which involves adding a message informing the user that certain features like QBD categories and tags are not yet supported—is more appropriate, provided that this issue is confirmed.
@alexpensify @lakchote @shubham1206agra @IuliiaHerets Does this make sense or am I missing something? Thank you :D
I have a question: if the connection setup fails, will the categories still be imported from the failed connection?
If not, I think it makes sense not to display that label here .
Instead, we could add another label warning the user to check the connection page for a potential failed connection, rather than informing them that the categories were imported
It does make sense @brunovjk, thanks!
The issue's title is misleading, it's indeed about QBD and not QBO cc @IuliiaHerets
QBD in NewDot will soon be supported, and is already supported in OldDot.
Regarding what to do between these 2 choices:
categories below are imported from...
message if the connection name is undefined
I'd go with helping the user by informing him on the current status of the integration. If it failed to connect, use a specific message. If it's not supported yet, do the same.
I'm going to tag @JmillsExpensify @trjExpensify what would you go with?
QBD in NewDot will soon be supported, and is already supported in OldDot.
How far away are we from building QBD workspace settings out in NewDot? I'm a bit mindful of investing in throwaway work with QBD coming soon. That said, we could hit the "undefined" message with Certina (FinancialForce) as well I guess which is a little further out - but not a million miles away.
Can't we know from the policy object which accounting solution is connected to the workspace already, and show it's name instead of undefined
on the categories and tags page here like normal?
Then on the accounting page replace this message with a generic _"Go to Expensify Classic to manage your
How far away are we from building QBD workspace settings out in NewDot? I'm a bit mindful of investing in throwaway work with QBD coming soon. That said, we could hit the "undefined" message with Certina (FinancialForce) as well I guess which is a little further out - but not a million miles away.
That's a fair concern. QBD should start to be implemented by the end of the month/early October. We could do this in advance yes, as it can impact other integrations down the line if they're not supported.
Can't we know from the policy object which accounting solution is connected to the workspace already, and show it's name instead of undefined on the categories and tags page here like normal? Yes, we can.
What you suggest make sense:
The categories below are imported from QuickBooks Desktop
(or the name of the connected integration, even if it's not yet supported) to make the messaging consistent. Go to Expensify Classic to manage your settings.
text.Updated my alternative solution to show the correct connection name regardless of whether the connection is supported or not
Display The categories below are imported from QuickBooks Desktop (or the name of the connected integration, even if it's not yet supported) to make the messaging consistent. On the policy's accounting page, show the Go to Expensify Classic to manage your settings. text.
I updated the main solution
I will review it later today.
I liked the update from @abzokhattab's proposal for its simplicity, I tested it and it solves our main issue.
I did a quick test on the app with those changes and didn't came across any issue, @abzokhattab you commented "Or, we can create a new function that uses the same logic as above but with a different function name. This would help avoid potential issues elsewhere in the code.", did you find anything in your tests? Also @abzokhattab your proposal is incomplete, it needs to be taken into consideration with the second comment from trjExpensify here: "Then on the accounting page replace this message with a generic "Go to Expensify Classic to manage your settings." which we'll remove in turn for QBD and Certina as those are built:"
@mkzie2 's proposal also seems ok, more robust, but I'm afraid it's bit over-engineered. @mkzie2 Correct me if I'm wrong, but I didn't see that we should "The text "The categories below are imported from your Quickbooks Desktop settings" should not be displayed if the connection encountered an error" as mentioned in your proposal.
did you find anything in your tests?
I have checked that deeply now and i think we can safely modify the existing method instead of creating another one or modifying the existing one by adding a new arg
needs to be taken into consideration with the second comment from
Updated the proposal to handle this point
I didn't see that we should "The text "The categories below are imported from your Quickbooks Desktop settings" should not be displayed if the connection encountered an error" as mentioned in your proposal.
I'm afraid it's bit over-engineered
Display The categories below are imported from QuickBooks Desktop (or the name of the connected integration, even if it's not yet supported) to make the messaging consistent. On the policy's accounting page, show the Go to Expensify Classic to manage your settings. text.
Thank you both for the updates and hard work. I appreciate the thoughtful approaches and carefully reviewed both proposals.
@abzokhattab: Although I like to look for the simplest solution, your proposal feels incomplete: It’s unclear how it handles scenarios like successful vs. failed connections.
@mkzie2: Apologies for my earlier wording—I wasn’t suggesting your solution was over-engineered, I was concern about not to do it. After a closer look, I see it’s robust, handling both current and future integrations well. I also liked the use of hasSyncError
for clearer user messaging.
I think we can move forward with @mkzie2's proposal, but I’d love that @justinpersaud take a look at the proposal and share their thoughts before moving on. Thank you.
🎀👀🎀 C+ reviewed
Triggered auto assignment to @justinpersaud, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
Sounds good to me!
📣 @brunovjk 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!
Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑💻 Keep in mind: Code of Conduct | Contributing 📖
📣 @mkzie2 You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑💻 Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing 📖
Woops I double clicked on the assignee button and hit the clear asignees button, so upwork job should actually be for @mkzie2
@justinpersaud, @alexpensify, @brunovjk, @mkzie2 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
Not overdue, PR being raised. @mkzie2 do you have an estimated deadline? Thanks.
Update: PR is in review
Weekly Update: PR is still in review
If this GitHub requires an urgent update, please ask for help in the #expensify-open-source Slack Room. If the inquiry can wait, I'll review it when I return online.
This shouldn't be part of the #migration project as it isn't related to one of the upcoming cohorts. #retain is probably a better place for this issue.
I agree it shouldn't be in #migrate, but it should be in #expense where QBD is being actively built. We're just waiting out the regression period here. Hit staging on Friday.
Reviewing
label has been removed, please complete the "BugZero Checklist".
The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.51-4 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2024-10-29. :confetti_ball:
For reference, here are some details about the assignees on this issue:
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
@brunovjk Please note that as of today we already support QBD integration, only this test case is valid:
- In OD, try to connect to Quickbooks Desktop but do not complete the set up to trigger error
- In ND, open workspace settings > Categories
- Verify
Get a better overview of where money is being spent...
message shows under Categories title
BugZero Checklist:
Regression Test Proposal
Setup on OldDot (OD):
"The connection could not be completed"
.Verification on NewDot (ND) with Connection Error:
"There's an error with the connection..."
."Get a better overview of where money is being spent..."
).Reconnect and Verify Successful Integration:
Return to OD and fully complete the connection setup with QuickBooks Desktop.
In ND, go to Workspace settings
Confirm that each tab now shows the message: "The [Categories/Report Fields/Taxes/Tags] below are imported from QuickBooks Desktop settings."
Click QuickBooks Desktop settings on any tab, and verify that the message "Go to Expensify Classic to manage your settings."
appears without any integration menu (Import, Export, Advanced...) or error message.
Do we agree 👍 or 👎
Payouts due: 2024-10-29
Upwork jobs are above.
All set, the payments are complete here. @justinpersaud do you agree with this regression test? If yes, I can create the GH. Thanks!
yep, sounds good to me
Ok, I'll work on the test process.
Other GHs have been a priority; I need to circle back here and create the regression test GH.
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: 9.0.37-0 Reproducible in staging?: Y Reproducible in production?: Y Issue was found when executing this PR: https://github.com/Expensify/App/pull/48759 Email or phone of affected tester (no customers): applausetester+kh010901@applause.expensifail.com Issue reported by: Applause Internal Team
Action Performed:
Precondition:
Expected Result:
The link in the header will not show "undefined settings".
Actual Result:
The link in the header shows "undefined settings". The same issue goes for Tags and Report fields.
Workaround:
Unknown
Platforms:
Screenshots/Videos
https://github.com/user-attachments/assets/f0b8352f-c955-4c2b-bb39-91f45758265b
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @alexpensify