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

[QBO] Chart of accounts page does not fit in RHP properly #41754

Closed
6 tasks done
lanitochka17 opened this issue May 7, 2024 · 28 comments
Closed
6 tasks done

[QBO] Chart of accounts page does not fit in RHP properly #41754

lanitochka17 opened this issue May 7, 2024 · 28 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering Internal Requires API changes or must be handled by Expensify staff Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented May 7, 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: 1.4.71-0
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to workspace chat
  3. Go to Accounting
  4. Connect to QBO
  5. Click Import
  6. Click Chart of accounts

Expected Result:

Chart of accounts page will fit in RHP properly

Actual Result:

Chart of accounts page does not fit in RHP properly

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

Bug6473650_1715080171257.QBO.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01442744b26c0bc6bf
  • Upwork Job ID: 1791012116292186112
  • Last Price Increase: 2024-05-16
@lanitochka17 lanitochka17 added the DeployBlockerCash This issue or pull request should block deployment label May 7, 2024
Copy link

melvin-bot bot commented May 7, 2024

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

Copy link
Contributor

github-actions bot commented May 7, 2024

👋 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
Copy link
Author

@arosiclair 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

@lanitochka17
Copy link
Author

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

@kbecciv kbecciv added the DeployBlocker Indicates it should block deploying the API label May 7, 2024
@neonbhai
Copy link
Contributor

neonbhai commented May 7, 2024

Proposal

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

QBO - Chart of accounts page does not fit in RHP properly

What is the root cause of that problem?

This happens as we are missing contentContainerStyles in ConnectionLayout here:

<ConnectionLayout
displayName={QuickbooksChartOfAccountsPage.displayName}
headerTitle="workspace.accounting.accounts"
title="workspace.qbo.accountsDescription"
accessVariants={[CONST.POLICY.ACCESS_VARIANTS.ADMIN]}
policyID={policyID}
featureName={CONST.POLICY.MORE_FEATURES.ARE_CONNECTIONS_ENABLED}
>

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

We will add

contentContainerStyle={[styles.pb2, styles.ph5]}

here:

<ConnectionLayout
displayName={QuickbooksChartOfAccountsPage.displayName}
headerTitle="workspace.accounting.accounts"
title="workspace.qbo.accountsDescription"
accessVariants={[CONST.POLICY.ACCESS_VARIANTS.ADMIN]}
policyID={policyID}
featureName={CONST.POLICY.MORE_FEATURES.ARE_CONNECTIONS_ENABLED}
>

Result

This works perfectly:

Before Screenshot 2024-05-07 at 8 33 07 PM
After Screenshot 2024-05-07 at 8 30 22 PM

@arosiclair
Copy link
Contributor

Seems to be from #41547.

@neonbhai's proposal seems fine, but I wonder why its necessary to add those styles now. It looked fine on the PR. @aldo-expensify any idea?

@marcaaron
Copy link
Contributor

Maybe a bad merge? @neonbhai how fast can you get a fix up for this?

@neonbhai
Copy link
Contributor

neonbhai commented May 7, 2024

@marcaaron hi, can raise the PR in a few minutes

@marcaaron
Copy link
Contributor

Yeah let's do it.

@neonbhai
Copy link
Contributor

neonbhai commented May 7, 2024

working on the PR!

@neonbhai
Copy link
Contributor

neonbhai commented May 7, 2024

PR is ready for review!

cc: @marcaaron @arosiclair

@rayane-djouah
Copy link
Contributor

@marcaaron @arosiclair - Should I go ahead with the PR review or will @mananjadhav review it?
I don't have access to accounting beta.

@arosiclair
Copy link
Contributor

I can add you to the beta just need your account email. Though I'm not sure how you can setup a QBO connection.

@mananjadhav
Copy link
Collaborator

mananjadhav commented May 7, 2024

I don't think we need this PR. I put up a fix for this one in my PR itself. Just waiting on approval

#41690 (comment)

@mananjadhav
Copy link
Collaborator

Also when I reviewed the PR #41547, the padding issue for ConnectionLayout (PR) wasn't merged.

By the time we got to merge the PR, ConnectionLayout was updated.

@marcaaron
Copy link
Contributor

@mananjadhav Can you maybe help us understand what we need to merge in order to unblock the deploy? You said this is fixed in this PR, but we're on a merge freeze at the moment so I can't CP those changes onto staging.

@mananjadhav
Copy link
Collaborator

mananjadhav commented May 7, 2024

@marcaaron Few options that I can think of:

  1. We still merge and CP my PR (Xero is under active development and behind beta, so shouldn't impact anyone)
  2. I can whip up a separate PR only to fix this screen and CP it.
  3. Use @neonbhai's PR and CP it.
  4. Remove deploy blocker from this one as I understand QBO is also under beta, and we merge my PR after the freeze is lifted on Friday.

@marcaaron
Copy link
Contributor

Ok, I like 4 and if this is actually under a beta then no Deploy Blocker should have been added in the first place.

@marcaaron marcaaron added Bug Something is broken. Auto assigns a BugZero manager. and removed DeployBlockerCash This issue or pull request should block deployment DeployBlocker Indicates it should block deploying the API labels May 7, 2024
Copy link

melvin-bot bot commented May 7, 2024

Triggered auto assignment to @greg-schroeder (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.

@mananjadhav
Copy link
Collaborator

I've linked this issue in my PR, so we can track it when merged.

@arosiclair arosiclair added Weekly KSv2 and removed Reviewing Has a PR in review Daily KSv2 labels May 8, 2024
@greg-schroeder
Copy link
Contributor

Merge freeze but PR looks approved

@trjExpensify trjExpensify changed the title Acccounting - QBO - Chart of accounts page does not fit in RHP properly [QBO] Chart of accounts page does not fit in RHP properly May 10, 2024
@trjExpensify
Copy link
Contributor

@greg-schroeder I think you can pay out @neonbhai $250 for the PR we didn't end up needing, but was prepared nevertheless, and then close this issue. It was taken care of elsewhere.

@greg-schroeder
Copy link
Contributor

Okay, got it. Will take care of that today

@greg-schroeder greg-schroeder added the Internal Requires API changes or must be handled by Expensify staff label May 16, 2024
Copy link

melvin-bot bot commented May 16, 2024

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

Copy link

melvin-bot bot commented May 16, 2024

Current assignee @mananjadhav is eligible for the Internal assigner, not assigning anyone new.

@greg-schroeder
Copy link
Contributor

Creating a job w/ the Internal label to send an offer to @neonbhai

Once you accept I'll pay you ASAP!

@neonbhai
Copy link
Contributor

@greg-schroeder Thank you, accepted!

@greg-schroeder
Copy link
Contributor

This was paid btw!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering Internal Requires API changes or must be handled by Expensify staff Weekly KSv2
Projects
No open projects
Archived in project
Development

No branches or pull requests

9 participants