Skip to content
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

[HOLD for payment 2024-06-11] [$250] [Xero] Xero appears under "Other integrations" when it is manually synced #41320

Closed
6 tasks done
kbecciv opened this issue Apr 30, 2024 · 28 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@kbecciv
Copy link

kbecciv commented Apr 30, 2024

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: 1.4.68-0
Reproducible in staging?: y
Reproducible in production?: n
Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to Account settings.
  3. Go to Workspaces > any workspace.
  4. Go to Accounting.
  5. Set up Xero.
  6. Expand "Other integrations" section.
  7. Click 3-dot menu next to Xer.
  8. Click Sync now.

Expected Result:

Xero will not appear under "Other integrations" when it is manually synced

Actual Result:

Xero appears under "Other integrations" when it is manually synced

Workaround:

n/a

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6466198_1714451205481.20240430_122403.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0174cf9740ce3f5618
  • Upwork Job ID: 1785320209758396416
  • Last Price Increase: 2024-04-30
  • Automatic offers:
    • rayane-djouah | Reviewer | 0
    • ShridharGoel | Contributor | 0
@kbecciv kbecciv added the DeployBlockerCash This issue or pull request should block deployment label Apr 30, 2024
Copy link

melvin-bot bot commented Apr 30, 2024

Triggered auto assignment to @amyevans (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@kbecciv
Copy link
Author

kbecciv commented Apr 30, 2024

We think that this bug might be related to #wave-collect - Release 1

@mananjadhav
Copy link
Collaborator

I don't think this is a deploy blocker. @mountiny can chime in on this one.

@ShridharGoel
Copy link
Contributor

ShridharGoel commented Apr 30, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

Sync functionality implementation is missing in Xero and quickbooks actually gets synced

What is the root cause of that problem?

syncConnection method is implemented only in QuickBooksOnline and the same is called via PolicyAccountingPage even if Xero is being synced.

function syncConnection(policyID: string) {
const optimisticData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.POLICY_CONNECTION_SYNC_PROGRESS}${policyID}`,
value: {
stageInProgress: CONST.POLICY.CONNECTIONS.SYNC_STAGE_NAME.STARTING_IMPORT,
connectionName: CONST.POLICY.CONNECTIONS.NAME.QBO,
},
},
];
const failureData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.SET,
key: `${ONYXKEYS.COLLECTION.POLICY_CONNECTION_SYNC_PROGRESS}${policyID}`,
value: null,
},
];
const parameters: SyncPolicyToQuickbooksOnlineParams = {
policyID,
idempotencyKey: policyID,
};
API.read(READ_COMMANDS.SYNC_POLICY_TO_QUICKBOOKS_ONLINE, parameters, {optimisticData, failureData});
}

This enables the (isSyncInProgress && integration !== connectionSyncProgress?.connectionName) condition and otherIntegrations contain Xero also while syncing.

const otherIntegrationsItems = useMemo(() => {
if (isEmptyObject(policy?.connections) && !isSyncInProgress) {
return;
}
const otherIntegrations = accountingIntegrations.filter(
(integration) => (isSyncInProgress && integration !== connectionSyncProgress?.connectionName) || integration !== connectedIntegration,
);
return otherIntegrations.map((integration) => {
const integrationData = accountingIntegrationData(integration, policyID, translate, true, connectedIntegration);
const iconProps = integrationData?.icon ? {icon: integrationData.icon, iconType: CONST.ICON_TYPE_AVATAR} : {};
return {
...iconProps,
title: integrationData?.title,
rightComponent: integrationData?.setupConnectionButton,
interactive: false,
shouldShowRightComponent: true,
wrapperStyle: styles.sectionMenuItemTopDescription,
};
});
}, [connectedIntegration, connectionSyncProgress?.connectionName, isSyncInProgress, policy?.connections, policyID, styles.sectionMenuItemTopDescription, translate]);

What changes do you think we should make in order to solve the problem?

Move syncConnection to a common place and pass valid connectionName (as of now it's using QBO always). Backend changes might also be needed if we want a different API for SyncPolicyToXero.

Update the overflow menu option:

{
    icon: Expensicons.Sync,
    text: translate('workspace.accounting.syncNow'),
    onSelected: () => syncConnection(policyID, connectedIntegration),
    disabled: isOffline,
}

Updated syncConnection:

function syncConnection(policyID: string, connectionName: string) {
    const optimisticData: OnyxUpdate[] = [
        {
            onyxMethod: Onyx.METHOD.MERGE,
            key: `${ONYXKEYS.COLLECTION.POLICY_CONNECTION_SYNC_PROGRESS}${policyID}`,
            value: {
                stageInProgress: CONST.POLICY.CONNECTIONS.SYNC_STAGE_NAME.STARTING_IMPORT,
                connectionName: connectionName,
            },
        },
    ];
    const failureData: OnyxUpdate[] = [
        {
            onyxMethod: Onyx.METHOD.SET,
            key: `${ONYXKEYS.COLLECTION.POLICY_CONNECTION_SYNC_PROGRESS}${policyID}`,
            value: null,
        },
    ];
    const parameters: SyncPolicyToQuickbooksOnlineParams = {
        policyID,
        idempotencyKey: policyID,
    };
    
    if (connectionName === CONST.POLICY.CONNECTIONS.NAME.QBO) {
        API.read(READ_COMMANDS.SYNC_POLICY_TO_QUICKBOOKS_ONLINE, parameters, {optimisticData, failureData});
    } else if (connectionName === CONST.POLICY.CONNECTIONS.NAME.XERO) {
        API.read(READ_COMMANDS.SYNC_POLICY_TO_XERO, parameters, {optimisticData, failureData});
    }
}

@mananjadhav
Copy link
Collaborator

@ShridharGoel's proposal looks good. @mountiny @amyevans Should we be making this external?

@amyevans amyevans added Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Apr 30, 2024
@amyevans
Copy link
Contributor

Agreed it doesn't need to be a deploy blocker. I'm not sure what the plan is for Xero polish and @mountiny is OOO. @lakchote do you know? Should we make this external or do y'all have alternate plans for handling follow up issues?

@lakchote lakchote added External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors labels Apr 30, 2024
@melvin-bot melvin-bot bot changed the title Xero - Xero appears under "Other integrations" when it is manually synced [$250] Xero - Xero appears under "Other integrations" when it is manually synced Apr 30, 2024
Copy link

melvin-bot bot commented Apr 30, 2024

Job added to Upwork: https://www.upwork.com/jobs/~0174cf9740ce3f5618

Copy link

melvin-bot bot commented Apr 30, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @rayane-djouah (External)

@lakchote
Copy link
Contributor

Agreed it doesn't need to be a deploy blocker. I'm not sure what the plan is for Xero polish and @mountiny is OOO. @lakchote do you know? Should we make this external or do y'all have alternate plans for handling follow up issues?

Let's make this external, as it doesn't need to be internal only.

We'll still need to make backend changes and create a new API command SyncPolicyToXero.

@ShridharGoel's proposal LGTM.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 30, 2024
@lakchote lakchote self-assigned this Apr 30, 2024
Copy link

melvin-bot bot commented Apr 30, 2024

📣 @rayane-djouah 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

Copy link

melvin-bot bot commented Apr 30, 2024

📣 @ShridharGoel 🎉 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 📖

@lakchote
Copy link
Contributor

Assigning @mananjadhav as he has the context on the issue.

@trjExpensify trjExpensify changed the title [$250] Xero - Xero appears under "Other integrations" when it is manually synced [$250] [Xero] Xero appears under "Other integrations" when it is manually synced May 14, 2024
@trjExpensify trjExpensify moved this to Release 1.5: XeroCon 2024 (June 12th) in [#whatsnext] #wave-collect May 14, 2024
@trjExpensify
Copy link
Contributor

PR merged into the freeze branch!

@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels May 27, 2024
Copy link

melvin-bot bot commented May 27, 2024

This issue has not been updated in over 15 days. @mananjadhav, @amyevans, @lakchote, @ShridharGoel eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Monthly KSv2 labels Jun 4, 2024
@melvin-bot melvin-bot bot changed the title [$250] [Xero] Xero appears under "Other integrations" when it is manually synced [HOLD for payment 2024-06-11] [$250] [Xero] Xero appears under "Other integrations" when it is manually synced Jun 4, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jun 4, 2024
Copy link

melvin-bot bot commented Jun 4, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Jun 4, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.78-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-06-11. 🎊

For reference, here are some details about the assignees on this issue:

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jun 10, 2024
Copy link

melvin-bot bot commented Jun 11, 2024

Issue is ready for payment but no BZ is assigned. @lschurr you are the lucky winner! Please verify the payment summary looks correct and complete the checklist. Thanks!

@lschurr
Copy link
Contributor

lschurr commented Jun 11, 2024

Payment summary:

Contributor+: @mananjadhav (No payment as discussed here)
Contributor: $250 @ShridharGoel (Paid in Upwork)

@mananjadhav
Copy link
Collaborator

@trjExpensify quick check, if we this should be included centrally? or separately for me.

@trjExpensify
Copy link
Contributor

Centrally yeah, this was part of the Xero imp!

@mananjadhav
Copy link
Collaborator

@lschurr There won't be any payout for me. Just @ShridharGoel.

@lschurr
Copy link
Contributor

lschurr commented Jun 11, 2024

Great, thanks! I've updated the payment summary.

@lschurr
Copy link
Contributor

lschurr commented Jun 13, 2024

@ShridharGoel please accept the offer in Upwork so that we can pay and close.

@melvin-bot melvin-bot bot added the Overdue label Jun 17, 2024
@lschurr
Copy link
Contributor

lschurr commented Jun 17, 2024

Closing this one. @ShridharGoel please comment here when you've accepted the offer so that we can pay.

@lschurr lschurr closed this as completed Jun 17, 2024
@melvin-bot melvin-bot bot removed the Overdue label Jun 17, 2024
@github-project-automation github-project-automation bot moved this from Release 1.5: XeroCon 2024 (June 12th) to Done in [#whatsnext] #wave-collect Jun 17, 2024
@lschurr
Copy link
Contributor

lschurr commented Jun 18, 2024

Paid!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor
Projects
No open projects
Archived in project
Development

No branches or pull requests

8 participants