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

[$250] Accounting tab appears then disappears after selecting an integration during onboarding #50637

Closed
6 tasks done
lanitochka17 opened this issue Oct 11, 2024 · 24 comments
Closed
6 tasks done
Assignees
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review

Comments

@lanitochka17
Copy link

lanitochka17 commented Oct 11, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 9.0.48-0
Reproducible in staging?: Y
Reproducible in production?: N/A Unable to check
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Sign up with a new Gmail account
  3. Select Manage my team's expenses on onboarding modal
  4. Select employee amount > Next
  5. Choose any integration > Next
  6. Complete the onborading
  7. Go to workspace settings

Expected Result:

Accounting tab will not appear and disappear

Actual Result:

Accounting tab appears then disappears

Workaround:

Unknown

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
Bug6631382_1728612818949.20241011_100847.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021844716640793633414
  • Upwork Job ID: 1844716640793633414
  • Last Price Increase: 2024-10-11
Issue OwnerCurrent Issue Owner: @allroundexperts
@lanitochka17 lanitochka17 added the DeployBlockerCash This issue or pull request should block deployment label Oct 11, 2024
Copy link

melvin-bot bot commented Oct 11, 2024

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

Copy link

melvin-bot bot commented Oct 11, 2024

💬 A slack conversation has been started in #expensify-open-source

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.

@lanitochka17 lanitochka17 added the DeployBlocker Indicates it should block deploying the API label Oct 11, 2024
@nkuoch
Copy link
Contributor

nkuoch commented Oct 11, 2024

I don't think this needs to be a blocker, so removing the labels, but yes let's find someone to work on it!

@nkuoch nkuoch added External Added to denote the issue can be worked on by a contributor Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment DeployBlocker Indicates it should block deploying the API labels Oct 11, 2024
Copy link

melvin-bot bot commented Oct 11, 2024

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

@melvin-bot melvin-bot bot changed the title Accounting tab appears then disappears after selecting an integration during onboarding [$250] Accounting tab appears then disappears after selecting an integration during onboarding Oct 11, 2024
@nkuoch nkuoch removed the Hourly KSv2 label Oct 11, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 11, 2024
Copy link

melvin-bot bot commented Oct 11, 2024

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

@NJ-2020
Copy link
Contributor

NJ-2020 commented Oct 11, 2024

Proposal

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

Accounting tab appears then disappears after selecting an integration during onboarding

What is the root cause of that problem?

Right here when we complete the onboarding we set the areConnectionsEnabled to true:

App/src/libs/actions/Report.ts

Lines 3647 to 3677 in abdfb72

if (userReportedIntegration) {
optimisticData.push({
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.POLICY}${onboardingPolicyID}`,
value: {
areConnectionsEnabled: true,
pendingFields: {
areConnectionsEnabled: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE,
},
},
});
successData.push({
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.POLICY}${onboardingPolicyID}`,
value: {
pendingFields: {
areConnectionsEnabled: null,
},
},
});
failureData.push({
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.POLICY}${onboardingPolicyID}`,
value: {
areConnectionsEnabled: getPolicy(onboardingPolicyID)?.areConnectionsEnabled,
pendingFields: {
areConnectionsEnabled: null,
},
},
});
}

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

We should remove this code

App/src/libs/actions/Report.ts

Lines 3647 to 3677 in abdfb72

if (userReportedIntegration) {
optimisticData.push({
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.POLICY}${onboardingPolicyID}`,
value: {
areConnectionsEnabled: true,
pendingFields: {
areConnectionsEnabled: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE,
},
},
});
successData.push({
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.POLICY}${onboardingPolicyID}`,
value: {
pendingFields: {
areConnectionsEnabled: null,
},
},
});
failureData.push({
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.POLICY}${onboardingPolicyID}`,
value: {
areConnectionsEnabled: getPolicy(onboardingPolicyID)?.areConnectionsEnabled,
pendingFields: {
areConnectionsEnabled: null,
},
},
});
}

What alternative solutions did you explore? (Optional)

@twilight2294
Copy link
Contributor

twilight2294 commented Oct 11, 2024

Edited by proposal-police: This proposal was edited at 2024-10-11 14:52:46 UTC.

Proposal

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

Accounting tab appears then disappears after selecting an integration during onboarding

What is the root cause of that problem?

We only set the areConnectionsEnabled value optimistically to true here, but then when we call the API, we only sent areConnectionsEnabled and not the policyID, so the BE won't update areConnectionsEnabled for that policy and when we get response from the api, the accounting is disabled.

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

In CompleteGuidedSetupParams pass onboardingPolicyID as well, this makes sure we are referencing the correct policyID.:

const parameters: CompleteGuidedSetupParams = {

Note: BE changes would also be needed to update the areConnectionsEnabled value of the policy.

We do the same thing in enablePolicyConnections here

What alternative solutions did you explore? (Optional)

@s77rt
Copy link
Contributor

s77rt commented Oct 11, 2024

Oh we forgot to send the policy id cc @nkdengineer. Please raise a PR for this since it's a regression

@marcaaron regarding #49161 (comment), what's the param that we should use to send the policy id over?

@s77rt
Copy link
Contributor

s77rt commented Oct 11, 2024

@allroundexperts I'd like to handle this being a "regression" from #49161

@nkdengineer
Copy link
Contributor

@marcaaron regarding #49161 (comment), what's the param that we should use to send the policy id over?

@s77rt I will raise the PR when we confirm this. Or we can do this in the second PR of the new feature when the task translation is confirmed.

@marcaaron
Copy link
Contributor

Should be policyID.

@marcaaron
Copy link
Contributor

marcaaron commented Oct 11, 2024

Or we can do this in the second PR of the new feature when the task translation is confirmed.

That sounds fine to me! Let's make sure to add that QA step.

@marcaaron
Copy link
Contributor

@nkuoch I will take this over as it's related to something I missed when working with these guys.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 11, 2024
@twilight2294
Copy link
Contributor

Hey this had Help Wanted Label attached and I posted a solution here which will be used in the PR, I guess contributors are compensated in such case right @marcaaron ?

@marcaaron
Copy link
Contributor

Hey @twilight294 I'd suggest bringing that up in Slack or shooting an email over to [email protected]. Typically, when something is a regression it's the responsibility of the person who implemented the PR. I appreciate the help and can provide this document to you review. And will also say that there are always exceptions. But I can't personally provide any guarantees right now.

@twilight2294
Copy link
Contributor

Thanks for responding, I will write a mail to [email protected] to discuss more on this 😄

@melvin-bot melvin-bot bot added the Overdue label Oct 14, 2024
@s77rt
Copy link
Contributor

s77rt commented Oct 14, 2024

Not overdue. Will be fixed in next PR

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Oct 14, 2024
@twilight2294
Copy link
Contributor

When is the PR for this issue coming?

@s77rt
Copy link
Contributor

s77rt commented Oct 17, 2024

@nkdengineer is working on it. It should be part of a PR related to #48745

Copy link

melvin-bot bot commented Oct 22, 2024

@s77rt, @marcaaron, @nkdengineer Huh... This is 4 days overdue. Who can take care of this?

@s77rt
Copy link
Contributor

s77rt commented Oct 22, 2024

Being fixed in #51070

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Oct 22, 2024
@marcaaron marcaaron added the Reviewing Has a PR in review label Oct 26, 2024
@melvin-bot melvin-bot bot removed the Overdue label Oct 26, 2024
Copy link

melvin-bot bot commented Nov 4, 2024

@s77rt, @marcaaron, @nkdengineer Eep! 4 days overdue now. Issues have feelings too...

@s77rt
Copy link
Contributor

s77rt commented Nov 4, 2024

This is fixed now. Can be closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

8 participants