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] RBR shows in NewDot on connections that are only configurable in OldDot #47797

Closed
6 tasks done
m-natarajan opened this issue Aug 21, 2024 · 33 comments
Closed
6 tasks done
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Monthly KSv2 Reviewing Has a PR in review

Comments

@m-natarajan
Copy link

m-natarajan commented Aug 21, 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: 9.0.23-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @dubielzyk-expensify
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1724202135633669

Action Performed:

  1. On OldDot, go to Settings -> Workspaces
  2. Choose a Control or Collect Workspace
  3. Go to Connections -> QuickBooks Desktop -> Click Connect to QuickBooks Desktop
  4. Click Shared Company Drive as an option and close the modal once it's giving you next steps.
  5. Refresh the page and see the error "The connection could not be completed" appear
  6. Open NewDot and see a RBR on your profile photo in the bottom navigation bar
  7. Tap on Profile photo in the navbar -> Workspaces -> Same workspace as above -> Accounting
  8. See an empty list with nothing to indicate the RBR error

Expected Result:

Expecting to either not see RBR or a note that this was configured in OldDot

Actual Result:

See an empty list with nothing to indicate the RBR error

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

www expensify com_policy_param={%22policyID%22_%2243BC6076D3D1418B%22}(MacBook Pro)

Add any screenshot/video evidence

Snip - (6) New Expensify - Google Chrome (2)

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01aa5ae4b74b2af547
  • Upwork Job ID: 1828156369928229332
  • Last Price Increase: 2024-09-02
Issue OwnerCurrent Issue Owner: @parasharrajat
@m-natarajan m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 21, 2024
Copy link

melvin-bot bot commented Aug 21, 2024

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

@daledah
Copy link
Contributor

daledah commented Aug 22, 2024

Edited by proposal-police: This proposal was edited at 2023-10-03T22:20:00Z.

Proposal

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

See an empty list with nothing to indicate the RBR error

What is the root cause of that problem?

  • We don't display the quickbooksDesktop in policy accounting page.

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

  • In this page, introduce a new variable hasConnectedIntegrationNotSupportInND to check whether there is a connected integration which is not support in ND:
    const hasSyncError = PolicyUtils.hasSyncError(policy);
    const allAccoutingIntegrations = [...accountingIntegrations, 'quickbooksDesktop'];
    const connectedIntegration = getConnectedIntegration(policy, accountingIntegrations) ?? connectionSyncProgress?.connectionName;
    const allConnectedIntegration = getConnectedIntegration(policy, allAccoutingIntegrations) ?? connectionSyncProgress?.connectionName;
    const hasConnectedIntegrationNotSupportInND = !connectedIntegration && allConnectedIntegration;
        if (isEmptyObject(policy?.connections) && !isSyncInProgress && !(hasConnectedIntegrationNotSupportInND && hasSyncError)) {

so if we have an integration that is only supported in OD and encounter an error, we still return 4 integration options in other sections.

                            {!(hasConnectedIntegrationNotSupportInND && hasSyncError) &&
                                connectionsMenuItems.map((menuItem) => (
                            {(otherIntegrationsItems || (hasConnectedIntegrationNotSupportInND && hasSyncError)) && (
                            {hasConnectedIntegrationNotSupportInND && hasSyncError && (
                                <FormHelpMessage
                                    isError
                                    shouldShowRedDotIndicator
                                    message={
                                        <Text style={[{color: theme.textError}]}>
                                            There's an error with a connection that's been set up in Expensify Classic.
                                            <TextLink
                                                onPress={() => {
                                                    // Go to Expensify Classic.
                                                }}
                                            >
                                                {' Go to Expensiy Classic to fix this issue.'}
                                            </TextLink>
                                        </Text>
                                    }
                                    style={styles.menuItemError}
                                />
                            )}

Result:

image

What alternative solutions did you explore? (Optional)

  • In this page, create:
    const hasSyncError = PolicyUtils.hasSyncError(policy);
    const allAccoutingIntegrations = [...accountingIntegrations, 'quickbooksDesktop'];
    const connectedIntegration = getConnectedIntegration(policy, accountingIntegrations) ?? connectionSyncProgress?.connectionName;
    const allConnectedIntegration = getConnectedIntegration(policy, allAccoutingIntegrations) ?? connectionSyncProgress?.connectionName;
    const hasConnectedIntegrationNotSupportInND = !connectedIntegration && allConnectedIntegration;
            if (hasConnectedIntegrationNotSupportInND) {
                return [
                    {
                        title: 'Other integration',
                        errorText: hasSyncError ? 'There was an error' : '',
                        shouldShowRightComponent: true,
                        rightComponent: (
                            <Button
                                onPress={() => {}}
                                text={'Redirect to OD'}
                                style={styles.justifyContentCenter}
                                small
                                isDisabled={isOffline}
                            />
                        ),
                        errorTextStyle: [styles.mt5],
                        shouldShowRedDotIndicator: true,
                        interactive: false,
                        wrapperStyle: [styles.sectionMenuItemTopDescription, hasSyncError && styles.pb0],
                    },
                ];
            }
            return [];
                                integrationToDisconnect: allConnectedIntegration
  • It is a demo code, the detail can be improved later.

What alternative solutions did you explore? (Optional)

  • If we want to hide the RBR, update:
    function hasSyncError(policy: OnyxEntry<Policy>): boolean {
    return (Object.keys(policy?.connections ?? {}) as ConnectionName[]).some((connection) => !!getSynchronizationErrorMessage(policy, connection, false));
    }

    to:
function hasSyncError(policy: OnyxEntry<Policy>): boolean {
    const accountingIntegrations = Object.values(CONST.POLICY.CONNECTIONS.NAME);
    return (Object.keys(policy?.connections ?? {}) as ConnectionName[]).some((connection) => !!getSynchronizationErrorMessage(policy, connection, false) && accountingIntegrations.includes(connection) );
}
  • Then do the same in the main solution to indicate that there is another connection that is only supported in OD.

@daledah
Copy link
Contributor

daledah commented Aug 22, 2024

Proposal updated

@dubielzyk-expensify
Copy link
Contributor

So for the solution here we want to show an error for integrations that aren't configurable via NewDot:

image

We don't want to hide the RBR or change anything about any other screen than the one above. Simple say that the integration error needs to be fixed in Expensify Classic. If this error is about the integrations that we support in NewDot, then we don't wanna do anything.

Hope that clarifies it 😄

@daledah
Copy link
Contributor

daledah commented Aug 26, 2024

Proposal updated

Copy link

melvin-bot bot commented Aug 26, 2024

@OfstadC Huh... This is 4 days overdue. Who can take care of this?

@OfstadC OfstadC added the External Added to denote the issue can be worked on by a contributor label Aug 26, 2024
@melvin-bot melvin-bot bot changed the title RBR shows in NewDot on connections that are only configurable in OldDot [$250] RBR shows in NewDot on connections that are only configurable in OldDot Aug 26, 2024
Copy link

melvin-bot bot commented Aug 26, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 26, 2024
Copy link

melvin-bot bot commented Aug 26, 2024

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

@parasharrajat
Copy link
Member

Reviewing...

Copy link

melvin-bot bot commented Sep 2, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@melvin-bot melvin-bot bot added the Overdue label Sep 2, 2024
Copy link

melvin-bot bot commented Sep 2, 2024

@OfstadC, @parasharrajat Eep! 4 days overdue now. Issues have feelings too...

@parasharrajat
Copy link
Member

parasharrajat commented Sep 2, 2024

@daledah Do you mind explaining your solutions in words as well? Also, how your alternative solutions are different from main one?

@melvin-bot melvin-bot bot removed the Overdue label Sep 2, 2024
@daledah
Copy link
Contributor

daledah commented Sep 4, 2024

@parasharrajat

Do you mind explaining your solutions in words as well?

My solution is to check if there is an integration that is not supported in ND and is encountering an error while connecting. If so, we will display the below UI:

image

Also, how your alternative solutions are different from main one

You can ignore it because it is added before I had the design

@parasharrajat
Copy link
Member

parasharrajat commented Sep 4, 2024

Thanks for the details. I think it will be simpler to just check that whether there is a connection in policy that is not part of accountingIntegrations instead of step 1 in your proposal.

Rest, overall @daledah's proposal looks fine. We can do the cleanup in the PR.

Suggestions:

  1. Please explain each step in your proposal, just showing the code is not enough.

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Sep 4, 2024

Triggered auto assignment to @puneetlath, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

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

melvin-bot bot commented Sep 4, 2024

📣 @daledah 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 📖

Copy link

melvin-bot bot commented Sep 4, 2024

@puneetlath @OfstadC @parasharrajat @daledah this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

@daledah
Copy link
Contributor

daledah commented Sep 5, 2024

@parasharrajat Currently I don't have access to Expensify Slack channel. Can you help to ask for new text translations? Looks like these phrases does not exist yet:

There's an error with a connection that's been set up in Expensify Classic.

and

Go to Expensiy Classic to fix this issue.

@parasharrajat
Copy link
Member

Sure

@parasharrajat
Copy link
Member

You can continue working on the PR with following while I get these confirmed

There's an error with a connection that's been set up in Expensify Classic. (Hay un error con una conexión que se ha configurado en Expensify Classic.)
and
Go to Expensiy Classic to fix this issue. (Ve a Expensify Classic para solucionar este problema.)

@daledah
Copy link
Contributor

daledah commented Sep 8, 2024

@parasharrajat Is the translations confirmed? PR is almost ready.

@melvin-bot melvin-bot bot added the Overdue label Sep 8, 2024
@parasharrajat
Copy link
Member

Not yet. I will ping someone directly.

@melvin-bot melvin-bot bot removed the Overdue label Sep 8, 2024
@parasharrajat
Copy link
Member

You can mark the PR ready for review so that I can test and review it. We can change these later @daledah

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Sep 9, 2024
@daledah
Copy link
Contributor

daledah commented Sep 9, 2024

@parasharrajat PR is ready.

Copy link

melvin-bot bot commented Sep 18, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

Copy link

melvin-bot bot commented Sep 18, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@OfstadC
Copy link
Contributor

OfstadC commented Oct 1, 2024

I think I missed when the PR deployed 😅 - Sorry folks! Working on sorting payment today

@OfstadC
Copy link
Contributor

OfstadC commented Oct 1, 2024

Payment Summary

@daledah
Copy link
Contributor

daledah commented Oct 1, 2024

@daledah could you please accept the offer here? 😃

@OfstadC Done!

@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Oct 2, 2024
Copy link

melvin-bot bot commented Oct 2, 2024

This issue has not been updated in over 15 days. @puneetlath, @OfstadC, @parasharrajat, @daledah 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!

@parasharrajat
Copy link
Member

@OfstadC Let's close it. I will request later.

@OfstadC OfstadC closed this as completed Oct 2, 2024
@github-project-automation github-project-automation bot moved this from Release 3: Fall 2024 (Nov) to Done in [#whatsnext] #expense Oct 2, 2024
@parasharrajat
Copy link
Member

Payment requested as per #47797 (comment)

@JmillsExpensify
Copy link

$250 approved for @parasharrajat

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. External Added to denote the issue can be worked on by a contributor Monthly KSv2 Reviewing Has a PR in review
Projects
Status: Done
Development

No branches or pull requests

7 participants