-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Comments
Triggered auto assignment to @OfstadC ( |
Edited by proposal-police: This proposal was edited at 2023-10-03T22:20:00Z. ProposalPlease 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?
What changes do you think we should make in order to solve the problem?
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:What alternative solutions did you explore? (Optional)
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
What alternative solutions did you explore? (Optional)
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) );
}
|
So for the solution here we want to show an error for integrations that aren't configurable via NewDot: 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 😄 |
@OfstadC Huh... This is 4 days overdue. Who can take care of this? |
Job added to Upwork: https://www.upwork.com/jobs/~01aa5ae4b74b2af547 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat ( |
Reviewing... |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
@OfstadC, @parasharrajat Eep! 4 days overdue now. Issues have feelings too... |
@daledah Do you mind explaining your solutions in words as well? Also, how your alternative solutions are different from main one? |
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:
You can ignore it because it is added before I had the design |
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:
🎀 👀 🎀 C+ reviewed |
Triggered auto assignment to @puneetlath, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
📣 @daledah You have been assigned to this job! |
@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! |
@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:
and
|
Sure |
You can continue working on the PR with following while I get these confirmed
|
@parasharrajat Is the translations confirmed? PR is almost ready. |
Not yet. I will ping someone directly. |
You can mark the PR ready for review so that I can test and review it. We can change these later @daledah |
@parasharrajat PR is ready. |
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. |
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. |
I think I missed when the PR deployed 😅 - Sorry folks! Working on sorting payment today |
Payment Summary
|
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! |
@OfstadC Let's close it. I will request later. |
Payment requested as per #47797 (comment) |
$250 approved for @parasharrajat |
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:
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?
Screenshots/Videos
Add any screenshot/video evidence
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @parasharrajatThe text was updated successfully, but these errors were encountered: