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
3.53k stars 2.88k forks source link

[Payment Due 4 Oct] [$250] Workspace connected to integration (NetSuite) should not allow editing of GL codes on imported tags. #49179

Closed m-natarajan closed 1 month ago

m-natarajan commented 1 month 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.33-1 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: @twisterdotcom Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1726160556242179

Action Performed:

  1. User have Control workspace connected to Netsuite
  2. Select the workspace from settings(Connected to Netsuite)
  3. Click Tags > Classifications > Accessories
  4. Add GL code and save
  5. Resync the connection
  6. Repeat step 3

    Expected Result:

    Shouldn't allow adding a GL code for imported Tags

    Actual Result:

    Able to add GL code for imported code

    Workaround:

    Unknown

    Platforms:

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

    • [ ] Android: Native
    • [ ] Android: mWeb Chrome
    • [ ] iOS: Native
    • [ ] iOS: mWeb Safari
    • [x] MacOS: Chrome / Safari
    • [ ] MacOS: Desktop

Screenshots/Videos

image

Snip - (2) New Expensify - Google Chrome

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021836971613968760708
  • Upwork Job ID: 1836971613968760708
  • Last Price Increase: 2024-09-20
  • Automatic offers:
    • c3024 | Reviewer | 104092521
Issue OwnerCurrent Issue Owner: @RachCHopkins
melvin-bot[bot] commented 1 month ago

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

twisterdotcom commented 1 month ago

I think the Expected and Actual result are incorrect here. We just shouldn't allow adding a GL code for imported Tags. We shouldn't persist them after sync.

RachCHopkins commented 1 month ago

Yeah I will update that GH title, it's not quite right.

RachCHopkins commented 1 month ago

Expected Result:

Shouldn't allow manual adding a non-imported GL code

Actual Result:

Able to add GL code manually

mkzie2 commented 1 month ago

Edited by proposal-police: This proposal was edited at 2024-09-16 05:07:41 UTC.

Proposal

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

Able to add GL code for imported code

What is the root cause of that problem?

We always allow the user to edit the GL code without checking whether we've connected to an accounting or not.

https://github.com/Expensify/App/blob/b69577a55bc536a411d8c1e51fc47fc0fc00d389/src/pages/workspace/tags/TagSettingsPage.tsx#L146-L149

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

We should use PolicyUtils.hasAccountingConnections function to check the workspace has an accounting or not. Then we can pass interactive prop as this check here. We can also display the right icon as lock icon

const hasAccountingConnections = PolicyUtils.hasAccountingConnections(policy);
iconRight={hasAccountingConnections ? Expensicons.Lock : undefined}
interactive={!hasAccountingConnections}

https://github.com/Expensify/App/blob/b69577a55bc536a411d8c1e51fc47fc0fc00d389/src/pages/workspace/tags/TagSettingsPage.tsx#L146-L149

OPTIONAL: We can also display the not found page for TagGLCodePage if we've connected to an accounting.

We can do the same fix for category GL code if we should also not allow adding them.

What alternative solutions did you explore? (Optional)

If we only don't allow adding GL code for tag when we connected to Netsuite, we can update the hasAccountingConnections check to only return true if we connected to Netsuite

RachCHopkins commented 1 month ago

Hmmm. I just went to reproduce this, and it looks like we are not wiping the default categories and replacing them with the NetSuite chart immediately either. I had to do a subsequent sync to bring in the NetSuite categories.

I also cannot see the Tags at all, despite setting it to import 3 different kinds. If I try to enable them from the workspace, it tells me they are managed from the integration. But it doesn't actually show what has been imported.

image

image

image

image

I think we might have a number of problems, bigger than just appending tags.

RachCHopkins commented 1 month ago

Posted in Wave-Control channel for some clarity. https://expensify.slack.com/archives/C06ML6X0W9L/p1726160568530669

RachCHopkins commented 1 month ago

We have a number of problems here, just confirming if I need to separate them out on to two more bug reports.

RachCHopkins commented 1 month ago

Ok, I have done two more bug reports, so we can just focus on the appending tags here.

RachCHopkins commented 1 month ago

Ok, I can reproduce this now. I got this wrong. It's not appending manual tags, it's allowing you to edit the GL code.

The expected result here is that when a tag is imported, the GL code field should not be editable, even if it is blank. It is not editable in Expensify classic.

It's correct that it should not persist after a sync, because the code does not come in via the sync.

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

RachCHopkins commented 1 month ago

Commenting re the proposal above - this is relevant to all connections, not just NetSuite.

c3024 commented 1 month ago

@mkzie2's proposal looks good to me.

It should be applied for all connections as specified above.

OPTIONAL: We can also display the not found page for TagGLCodePage if we've connected to an accounting.

Agreed.

I think we should not allow editing GL codes for Categories also as suggested in his proposal. This needs to be confirmed.

🎀 👀 🎀 C+ Reviewed

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

📣 @c3024 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 1 month ago

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

mkzie2 commented 1 month ago

@c3024 The PR is ready

RachCHopkins commented 1 month ago

Automation is borked on this.

@c3024 what's the payment date on this one? Can you do the checklist please?

@mkzie2 did you have an offer to accept in Upwork?

c3024 commented 1 month ago
  • [x] [@c3024] The PR that introduced the bug has been identified. Link to the PR: #43155
  • [x] [@c3024] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/43155/files#r1780496384
  • [x] [@c3024] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: No discussion was started because this could not have been identified earlier.
  • [x] [@c3024] Determine if we should create a regression test for this bug. Yes
  • [x] [@c3024] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression test proposal

  1. Create a workspace
  2. Click on "More features" in the workspace settings page left hand panel
  3. Scroll down and go to "Organize" section and switch on the "Tags" switch
  4. Scroll down and switch the "Accounting" switch to on
  5. Click on "Accounting" from the left hand panel
  6. Click on "Netsuite"
  7. Upgrade the workspace to Control if prompted
  8. Complete the "Netsuite" connection flow
  9. Click on the 3 dot menu on the "Netsuite" account added and click on "Sync now"
  10. Go to "Tags" and click on any tag
  11. Verify that the GL code option is disabled in the opened tag settings page
c3024 commented 1 month ago

Payment is due on 4-Oct.

mkzie2 commented 1 month ago

@mkzie2 did you have an offer to accept in Upwork?

@RachCHopkins Hi not yet, can you please send the offer to my account https://www.upwork.com/freelancers/~019f73367b03c6d784

Thanks in advance

RachCHopkins commented 1 month ago

Offer sent @mkzie2

mkzie2 commented 1 month ago

@RachCHopkins I have accepted

RachCHopkins commented 1 month ago

Thanks @mkzie2 will action the payment on the 4th!

RachCHopkins commented 1 month ago

Payment Summary:

Upwork job here

RachCHopkins commented 1 month ago

Contributors have been paid, the contracts have been completed, and the Upwork post has been closed.