App icon indicating copy to clipboard operation
App copied to clipboard

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

Open lanitochka17 opened this issue 1 year ago β€’ 27 comments

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] 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): [email protected] 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
  • [x] MacOS: Chrome / Safari
  • [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/393d0ddb-262e-4d74-be2c-43748195c22c

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 avatar Aug 15 '24 12:08 lanitochka17

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.

melvin-bot[bot] avatar Aug 15 '24 12:08 melvin-bot[bot]

@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 avatar Aug 15 '24 12:08 lanitochka17

We think that this bug might be related to #wave-control

lanitochka17 avatar Aug 15 '24 12:08 lanitochka17

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

sakluger avatar Aug 16 '24 00:08 sakluger

Weird. Will take a look.

yuwenmemon avatar Aug 16 '24 03:08 yuwenmemon

@yuwenmemon any ideas?

sakluger avatar Aug 19 '24 16:08 sakluger

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

yuwenmemon avatar Aug 21 '24 20:08 yuwenmemon

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.

yuwenmemon avatar Aug 27 '24 04:08 yuwenmemon

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?

mananjadhav avatar Aug 27 '24 05:08 mananjadhav

@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.

yuwenmemon avatar Aug 27 '24 17:08 yuwenmemon

@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.

mananjadhav avatar Aug 27 '24 17:08 mananjadhav

Yes πŸ˜„

yuwenmemon avatar Aug 28 '24 17:08 yuwenmemon

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?

yuwenmemon avatar Aug 28 '24 22:08 yuwenmemon

I can pick this.

mananjadhav avatar Aug 29 '24 02:08 mananjadhav

I am here for review if needed.

shubham1206agra avatar Aug 29 '24 02:08 shubham1206agra

@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!

melvin-bot[bot] avatar Aug 29 '24 18:08 melvin-bot[bot]

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

sakluger avatar Aug 30 '24 17:08 sakluger

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

melvin-bot[bot] avatar Sep 02 '24 18:09 melvin-bot[bot]

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

melvin-bot[bot] avatar Sep 03 '24 21:09 melvin-bot[bot]

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

melvin-bot[bot] avatar Sep 03 '24 21:09 melvin-bot[bot]

@mananjadhav The IS portion is deployed! We should be able to use the lastSync object for NetSuite like we do for the rest of the integrations now πŸŽ‰

Assigning to you for implementation.

yuwenmemon avatar Sep 03 '24 21:09 yuwenmemon

πŸ“£ @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 πŸ“–

melvin-bot[bot] avatar Sep 03 '24 21:09 melvin-bot[bot]

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.

yuwenmemon avatar Sep 03 '24 21:09 yuwenmemon

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?

mananjadhav avatar Sep 04 '24 17:09 mananjadhav

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

yuwenmemon avatar Sep 04 '24 18:09 yuwenmemon

Okay thanks for confirming.

mananjadhav avatar Sep 04 '24 18:09 mananjadhav

@mananjadhav Bump here for PR

shubham1206agra avatar Sep 07 '24 16:09 shubham1206agra

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

mananjadhav avatar Sep 08 '24 18:09 mananjadhav

Thanks for the update @mananjadhav

greg-schroeder avatar Sep 09 '24 06:09 greg-schroeder

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 ?

mananjadhav avatar Sep 09 '24 12:09 mananjadhav