Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
4.03k stars 3.03k forks source link

[$250] Sage Intacct - Preferred exporter remains the same after changing the role of admin to member #55729

Open lanitochka17 opened 3 weeks ago

lanitochka17 commented 3 weeks ago

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.89-2 Reproducible in staging?: Y Reproducible in production?: Y If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Y 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): htad26+ri@gmail.com Issue reported by: Applause - Internal Team

Action Performed:

Prerequisite An account with a workspace connected to Sage Intacct

  1. Go to members > Invite > Choose the role as Admin > Finish the flow
  2. Navigate to accounting > Export > Preferred exporter > Change the exporter to the newly invited admin
  3. Go to members > Change the role of the newly invited admin to member
  4. Go back to Preferred exporter page and check the exporter

Expected Result:

The preferred exporter is changed back to the owner/admin of the workspace since only admins can be selected as preferred exporter

Actual Result:

The non-admin member is still selected as the preferred exporter even if it is not listed in the preferred exporter page. User has to manually change the exporter

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/4c80c027-390e-487f-8b82-d331b5d4169d

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021882884778254645916
  • Upwork Job ID: 1882884778254645916
  • Last Price Increase: 2025-02-14
Issue OwnerCurrent Issue Owner: @yuwenmemon
melvin-bot[bot] commented 3 weeks ago

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

FitseTLT commented 3 weeks ago

🚨 Edited by proposal-police: This proposal was edited at 2025-01-24 21:04:07 UTC.

Proposal

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

Sage Intacct - Preferred exporter remains the same after changing the role of admin to member

What is the root cause of that problem?

We are displaying exporter config without checking if it is an admin https://github.com/Expensify/App/blob/fc199faf777dc7f1710b3d5cb4ab7abf6106309c/src/pages/workspace/accounting/intacct/export/SageIntacctExportPage.tsx#L32 but we don't see it selected in the list when opening preferred exporter page because we only include list of admins of the workspace https://github.com/Expensify/App/blob/fc199faf777dc7f1710b3d5cb4ab7abf6106309c/src/pages/workspace/accounting/intacct/export/SageIntacctPreferredExporterPage.tsx#L31

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

In updateWorkspaceMembersRole we should check if it is the preferred exporter (policy?.connections?.intacct?.config.export.exporter) and the role is being demoted from admin role then we should either

  1. update exporter to the police owner optimistically like prepareOnyxDataForExportUpdate and make the corresponding BE update on UPDATE_WORKSPACE_MEMBERS_ROLE API
  2. or if we don't want to make a BE change we can use the existing updateSageIntacctExporter to set the owner as the exporter

These problems also for the other accounting integrations so similar fix can be applied. Similarly, this changes can be applied for the case of the admin member removal from a workspace if the user was the preferred exporter.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

We can create tests for updateWorkspaceMembersRole to assert it properly updates exporter when the default exporter is demoted from admin role.

What alternative solutions did you explore? (Optional)

If we don't want to change the exporter to the owner at least we have to avoid showing the exporter in SageIntacctExportPage if it is not an admin because it will not currently be displayed in SageIntacctPreferredExporterPage as we only include admins in the list.

melvin-bot[bot] commented 3 weeks ago

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

melvin-bot[bot] commented 3 weeks ago

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

ugogiordano commented 2 weeks ago

@lanitochka17, how can an external contributor test an accounting system connected to the workspace?

ugogiordano commented 2 weeks ago

Proposal

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

The preferred exporter in SageIntacctPreferredExporterPage doesn’t update when changing the user’s role from admin to member.

What is the root cause of that problem?

The issue is that SageIntacctPreferredExporterPage is using WithPolicyProps, which does not define the isConnectionDataFetchNeeded variable:

https://github.com/Expensify/App/blob/7ae69fff36627624632882de95b42551402ad98f/src/pages/workspace/accounting/intacct/export/SageIntacctPreferredExporterPage.tsx#L25

https://github.com/Expensify/App/blob/7ae69fff36627624632882de95b42551402ad98f/src/pages/workspace/withPolicy.tsx#L57-L65

This variable is defined by WithPolicyConnectionsProps:

https://github.com/Expensify/App/blob/7ae69fff36627624632882de95b42551402ad98f/src/pages/workspace/withPolicyConnections.tsx#L15-L17

and is needed to trigger the connection data fetch when the role changes. Without this variable, the exporter doesn't update as expected.

In contrast, all the other pages for the account systems use WithPolicyConnectionsProps,

https://github.com/Expensify/App/blob/7ae69fff36627624632882de95b42551402ad98f/src/pages/workspace/accounting/xero/export/XeroPreferredExporterSelectPage.tsx#L26

https://github.com/Expensify/App/blob/7ae69fff36627624632882de95b42551402ad98f/src/pages/workspace/accounting/netsuite/export/NetSuiteTaxPostingAccountSelectPage.tsx#L21

https://github.com/Expensify/App/blob/7ae69fff36627624632882de95b42551402ad98f/src/pages/workspace/accounting/qbd/export/QuickbooksDesktopPreferredExporterConfigurationPage.tsx#L24

https://github.com/Expensify/App/blob/7ae69fff36627624632882de95b42551402ad98f/src/pages/workspace/accounting/qbo/export/QuickbooksPreferredExporterConfigurationPage.tsx#L24

, which correctly includes this variable and ensures the update is triggered when the role changes.

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

Change SageIntacctPreferredExporterPage to use WithPolicyConnectionsProps instead of WithPolicyProps. This will ensure the component gets the isConnectionDataFetchNeeded variable and triggers the data fetch when the role changes.

...
import type {WithPolicyConnectionsProps} from '@pages/workspace/withPolicyConnections';
...

function SageIntacctPreferredExporterPage({policy}: WithPolicyConnectionsProps) {
...

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

What alternative solutions did you explore? (Optional)

N/A

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

melvin-bot[bot] commented 2 weeks ago

@lschurr, @dominictb Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

lanitochka17 commented 2 weeks ago

@ugogiordano If you don't have access to Sage Intacct then you won't be able to check this.

melvin-bot[bot] commented 2 weeks ago

@lschurr, @dominictb Huh... This is 4 days overdue. Who can take care of this?

dominictb commented 2 weeks ago

Just able to reproduce. Reviewing.

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 1 week ago

@lschurr, @dominictb Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

dominictb commented 1 week ago

UpdateWorkpsaceMembersRole and DeleteMembersFromWorkspace API do not update the preferred exporter accordingly. This is a BE issue but we still need to work on optimistic data as well.

I need confirmation on the expected behavior.

Context: The integration's Preferred exporter should always be a workspace admin. If the current preferred exporter's role is no longer an Admin, or get removed from the workspace, what should happen?

  1. Fallback to the workspace owner or another admin
  2. Clear the preferred exporter and let the user select another one later

🎀👀🎀

melvin-bot[bot] commented 1 week ago

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

cristipaval commented 1 week ago

@yuwenmemon, I see you know about Sage Intacct integration in NewDot. Could you please manage this one? 🙏

Feel free to reassign it to me if you're full.

dominictb commented 1 week ago

@yuwenmemon Mind giving us some ideas here https://github.com/Expensify/App/issues/55729#issuecomment-2633047640?

melvin-bot[bot] commented 1 week ago

@yuwenmemon @lschurr @dominictb 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!

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 6 days ago

@yuwenmemon, @lschurr, @dominictb Whoops! This issue is 2 days overdue. Let's get this updated quick!

yuwenmemon commented 6 days ago

It should be the workspace owner, correct. This is what we do in Expensify Classic as well:

Image

dominictb commented 6 days ago

@yuwenmemon Do we want to show such message modal as well in ND? Or just fallback to the workspace owner siliently is fine?

dominictb commented 3 days ago

@yuwenmemon Need 👀 on ^

melvin-bot[bot] commented 2 days ago

@yuwenmemon, @lschurr, @dominictb Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 2 days ago

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