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] Netsuite - Able to connect Netsuite with incorrect tokens #47493

Closed
1 of 6 tasks
lanitochka17 opened this issue Aug 15, 2024 · 36 comments
Closed
1 of 6 tasks

[$250] Netsuite - Able to connect Netsuite with incorrect tokens #47493

lanitochka17 opened this issue Aug 15, 2024 · 36 comments
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

@lanitochka17
Copy link

lanitochka17 commented Aug 15, 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.20-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: https://expensify.testrail.io/index.php?/tests/view/4859063&group_by=cases:section_id&group_order=asc&group_id=314239
Email or phone of affected tester (no customers): applausetester+gm1094@applause.expensifail.com
Issue reported by: Applause - Internal Team

Action Performed:

Precondition:

  1. Login with an account that is an admin of a control workspace (or create one in OD if needed)
  2. Enable accounting in workspace settings
    Steps
  3. Navigate to Accounting
  4. Click on the connect button next to NetSuite
  5. Reach the credentials step
  6. Enter incorrect the NetSuite tokens

Expected Result:

When credentials are entered incorrectly a sync error should be displayed

Actual Result:

User is able to connect Netsuite with incorrect tokens. No error message is displayed

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

Bug6572733_1723709133681.Recording__3717.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01f1ac27b5bef67232
  • Upwork Job ID: 1831077669368516851
  • Last Price Increase: 2024-09-03
  • Automatic offers:
    • shubham1206agra | Contributor | 103803673
Issue OwnerCurrent Issue Owner: @rushatgabhane
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 15, 2024
Copy link

melvin-bot bot commented Aug 15, 2024

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

@lanitochka17
Copy link
Author

@sakluger 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-control

@sakluger
Copy link
Contributor

Hey @yuwenmemon, any idea why we're not throwing a connection error here?

@yuwenmemon yuwenmemon self-assigned this Aug 16, 2024
@yuwenmemon
Copy link
Contributor

Weird. Will take a look.

@melvin-bot melvin-bot bot added the Overdue label Aug 19, 2024
@sakluger
Copy link
Contributor

@yuwenmemon any ideas?

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Aug 19, 2024
@yuwenmemon
Copy link
Contributor

No update yet, other issues have been taking precedence but will look soon!

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Aug 21, 2024
@yuwenmemon
Copy link
Contributor

So... we never really built a way to display error messages from NetSuite other than the generic sync error message (cc @mananjadhav @shubham1206agra) - we kind of kicked the can down the road on that one. But I'm thinking now might be a good time to get this going and we can use the traditional lastSync object that all the other integrations use.

If anything, we can do this just for the error message for now, and then maybe move NetSuite over to using it just like the rest of the integrations, at least for NewDot.

@melvin-bot melvin-bot bot removed the Overdue label Aug 27, 2024
@mananjadhav
Copy link
Collaborator

Agreed. We started this discussion here but didn't prioritize. One of the recommendations was lastSync.wrongCredentials = true, so that we can differentiate between a general error or auth error. We could also probably change it to enum and handle it accordingly. wdyt?

@yuwenmemon
Copy link
Contributor

@mananjadhav We actually have an isAuthentiationError property now on the lastSync object that can help with that too. I can make sure we start adding it for NetSuite.

@mananjadhav
Copy link
Collaborator

@mananjadhav We actually have an isAuthentiationError property now on the lastSync object that can help with that too. I can make sure we start adding it for NetSuite.

Yeah that would help. I am assuming isAuthenticationError is correct property name.

@yuwenmemon
Copy link
Contributor

Yes 😄

@yuwenmemon
Copy link
Contributor

Okay I've put in a PR above to update the NetSuite connection to start using the lastSync object like all the other integrations. We can flip this to be external once that's deployed. @mananjadhav @SzymczakJ or @war-in Would any of you be interested in working on that?

@mananjadhav
Copy link
Collaborator

I can pick this.

@shubham1206agra
Copy link
Contributor

I am here for review if needed.

Copy link

melvin-bot bot commented Aug 29, 2024

@yuwenmemon @sakluger this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

@sakluger
Copy link
Contributor

@yuwenmemon - does your PR use the same methods that are being used in the QBO Export & Error Handling project?

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

melvin-bot bot commented Sep 3, 2024

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

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@yuwenmemon
Copy link
Contributor

Apologies for the melvin madness @rushatgabhane @shubham1206agra @mananjadhav @sakluger

As per the conversation above, @mananjadhav will take on the implementation here and @shubham1206agra will do the review.

@mananjadhav
Copy link
Collaborator

Thanks for assigning. I'll have the PR Draft ready by tomorrow. @yuwenmemon just confirming once again based on the isAuthenticationError property is set, we'll show an invalid auth message, right? do we have any text/copy that we have finalized/used?

@yuwenmemon
Copy link
Contributor

Let's follow the model used by Intacct. An error message will be stored in the lastSync object itself as well:

Screenshot 2024-09-04 at 11 53 29 AM Screenshot 2024-09-04 at 11 57 41 AM

@mananjadhav
Copy link
Collaborator

Okay thanks for confirming.

@shubham1206agra
Copy link
Contributor

@mananjadhav Bump here for PR

@melvin-bot melvin-bot bot removed the Overdue label Sep 7, 2024
@mananjadhav
Copy link
Collaborator

mananjadhav commented Sep 8, 2024

Working on it, will be ready by tomorrow. I wasn't available on the weekend.

@greg-schroeder
Copy link
Contributor

Thanks for the update @mananjadhav

@mananjadhav
Copy link
Collaborator

SageIntacct adds an error message and a Learn more link which leads to the help.expensify page. For NetSuite should we use this link https://help.expensify.com/articles/expensify-classic/connections/netsuite/Netsuite-Troubleshooting#expensierror-ns0109-failed-to-login-to-netsuite-please-verify-your-credentials ?

@yuwenmemon
Copy link
Contributor

Yeah that would be a nice addition @mananjadhav

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Sep 10, 2024
@melvin-bot melvin-bot bot removed the Weekly KSv2 label Oct 4, 2024
Copy link

melvin-bot bot commented Oct 4, 2024

This issue has not been updated in over 15 days. @mananjadhav, @yuwenmemon, @sakluger, @shubham1206agra 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!

@melvin-bot melvin-bot bot added the Monthly KSv2 label Oct 4, 2024
@sakluger
Copy link
Contributor

sakluger commented Oct 8, 2024

Summarizing payment on this issue:

Contributor: @mananjadhav $250, please request on Newdot
Contributor+: @shubham1206agra $250, sent offer via Upwork (https://www.upwork.com/nx/wm/offer/104323832)

@shubham1206agra I had to send a new offer - please let me know once you've accepted it. Thanks!

@shubham1206agra
Copy link
Contributor

@sakluger Accepted offer

@garrettmknight
Copy link
Contributor

$250 approved for @mananjadhav

@sakluger
Copy link
Contributor

sakluger commented Oct 9, 2024

Thanks everyone! I completed payment to @shubham1206agra so we're good to close this one out.

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
Archived in project
Development

No branches or pull requests

8 participants