[HOLD for payment 2024-11-11] [$750] Netsuite - App navigates to the first flow when refreshing the page while adding custom list

lanitochka17 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.42-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: N/A Email or phone of affected tester (no customers): applausetester+shsb22tet1122@applause.expensifail.com Issue reported by: Applause - Internal Team

Action Performed:

Log in to new dot with any account
Create a new workspace > Enable "Accounting" in the "More features" page
Navigate to "Accounting" > Connect to NetSuite and upgrade the workspace to Control > Enter credentials and finish
After the connection syncs go to Import > Custom Lists > Add custom list
Click on "Name" and select any of the options > Next > Type any ID name > Next
Before making any selection refresh the page

Expected Result:

App stays in the current flow ("How should this custom list be displayed in Expensify?")

Actual Result:

App navigates to the first step of the flow ("Choose a custom list" step)

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/7ae9a867-ea6e-47f9-b669-c965e017dd74

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~021842185001478860112
Upwork Job ID: 1842185001478860112
Last Price Increase: 2024-11-12
Automatic offers:

rayane-djouah | Reviewer | 104455519
allgandalf | Contributor | 104455521

Issue Owner

Current Issue Owner: @sonialiap

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @sonialiap (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 commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

allgandalf commented 1 month ago

Proposal

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

Form navigates to first step on refresh

What is the root cause of that problem?

There are actually 2 bug in the flow:

We do not save the current step anywhere in the flow. And we always default to the first step of the flow, that is why on refresh we go to the first step:

https://github.com/Expensify/App/blob/99f280b0edae75ff59614a1c5e98c47b39800aa5/src/pages/workspace/accounting/netsuite/import/NetSuiteImportCustomFieldNew/NetSuiteImportAddCustomListPage.tsx#L155

We also do not save the draft values in the flow.

So even if we fix 1, there would be a regression that values won't be saved once refreshed. So we need to fix both the bugs here.

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

Like the refactor we did in :

https://github.com/Expensify/App/pull/49452

We need to refactor the full form to save draft values of the form as well as save the current state of the form (sub-step) using the util getInitialSubstep. We would also need to refactor NetSuiteImportAddCustomSegmentPage as both use the same substeps. and also update any similar places where this bug exists

[!NOTE] The scope of work is vast as this would be full form refactor i.e. adding draft states to Onyx, adding draft values to FORM input of both NetSuiteImportAddCustomSegmentPage and NetSuiteImportAddCustomListPage, creating new utils, changing the form structure completely, creating related types files. So i think that for this issue a fair compensation would be $750 considering the amount of work involved

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 1 month ago

@sonialiap, @rayane-djouah Whoops! This issue is 2 days overdue. Let's get this updated quick!

rayane-djouah commented 1 month ago

@allgandalf's proposal looks good to me

:ribbon::eyes::ribbon: C+ reviewed

melvin-bot[bot] commented 1 month ago

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

allgandalf commented 1 month ago

thanks @rayane-djouah , @danieldoglas please take a look at the note, i have explained the scope of work and requested an increase in bounty, please consider that request 😄

rayane-djouah commented 1 month ago

I agree that a payment increase would be fair given the scope of work

allgandalf commented 1 month ago

bump @danieldoglas for assignment

danieldoglas commented 1 month ago

I missed this issue, my bad. I've read the proposal and I initially agree with it, but I'll refer to @yuwenmemon since he was one of the people working on this functionality before moving on.

melvin-bot[bot] commented 1 month ago

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

rayane-djouah commented 1 month ago

@yuwenmemon Kind reminder

allgandalf commented 1 month ago

I checked their slack, they are out of office until tuesday, @danieldoglas how should we proceed here?

danieldoglas commented 1 month ago

I think it's fine to wait for an answer in this case, I prefer not to change it without consulting the person who built it first.

rayane-djouah commented 1 month ago

Not overdue

melvin-bot[bot] commented 4 weeks ago

@danieldoglas @sonialiap @rayane-djouah 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!

rayane-djouah commented 4 weeks ago

Waiting on @yuwenmemon review

sonialiap commented 3 weeks ago

@yuwenmemon was OOO end of last week and this monday but hopefully will have time to review the proposal this week

yuwenmemon commented 3 weeks ago

Yep, @allgandalf's proposal looks good.

From the design doc:

Similar to how these fields function in the token auth setup, each field will be sticky in the sense that if the admin left Expensify and came back, everything would stay present. And, it’s important to keep these fields sticky such that admins can edit them.

So yes, draft values should be getting saved. cc @mananjadhav

melvin-bot[bot] commented 3 weeks ago

📣 @rayane-djouah 🎉 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 3 weeks ago

📣 @allgandalf 🎉 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 📖

allgandalf commented 3 weeks ago

@sonialiap can you bump the price for this issue please 🙏

mananjadhav commented 3 weeks ago

Thanks for picking this folks. I missed this one from the design doc. Let me know if any other help/secondary review is needed.

allgandalf commented 3 weeks ago

@rayane-djouah PR ready for review, there might be some style changes which i am working on currently , but you can test the functionality it works smooth

allgandalf commented 3 weeks ago

@rayane-djouah fixed the style changes as well, PR should be ready for final review

rayane-djouah commented 1 week ago

PR on staging

melvin-bot[bot] commented 1 week ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 1 week ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.56-9 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

https://github.com/Expensify/App/pull/51109

If no regressions arise, payment will be issued on 2024-11-11. :confetti_ball:

For reference, here are some details about the assignees on this issue:

@rayane-djouah requires payment automatic offer (Reviewer)
@allgandalf requires payment automatic offer (Contributor)

melvin-bot[bot] commented 1 week ago

@rayane-djouah / @allgandalf @sonialiap The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

allgandalf commented 1 week ago

@sonialiap can you bump the payment to $750 here please ;)

rayane-djouah commented 1 day ago

BugZero Checklist:

[x] Classify the bug:

Bug classification
Source of bug: - [ ] 1a. Result of the original design (eg. a case wasn't considered) - [x] 1b. Mistake during implementation - [ ] 1c. Backend bug - [ ] 1z. Other: Where bug was reported: - [x] 2a. Reported on production - [ ] 2b. Reported on staging (deploy blocker) - [ ] 2c. Reported on a PR - [ ] 2z. Other: Who reported the bug: - [ ] 3a. Expensify user - [ ] 3b. Expensify employee - [ ] 3c. Contributor - [x] 3d. QA - [ ] 3z. Other:
[x] 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/44942#issuecomment-2469192515
[x] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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: N/A
[x] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.
[x] @sonialiap Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

Link to issue: https://github.com/Expensify/Expensify/issues/443451

Regression Test Proposal

#### Precondition:

- Workspace connected to the NetSuite integration.

#### Tests:

Test 1:

1. Navigate to Accounting > NetSuite Import > Custom Lists > Add New List.
2. Enter a custom list name and click "Next"; this takes you to the Transaction ID page.
3. Refresh the page.
4. Verify that upon refresh, you remain on the Transaction ID page.
5. Navigate back to the previous page.
6. Verify that the previously selected value persists.

Test 2:

1. Navigate to Accounting > NetSuite Import > Custom Segments > Add New Segment.
2. Select a custom record and click "Next".
3. Enter a custom record name and click "Next"; this takes you to the Internal ID page.
4. Refresh the page.
5. Verify that upon refresh, you remain on the Internal ID page.
6. Navigate back to the previous page.
7. Verify that the previously entered information persists.

Do we agree 👍 or 👎

allgandalf commented 20 hours ago

@sonialiap friendly bump for the bounty increase 😄

sonialiap commented 20 hours ago

Chatted with Daniel and confirmed increase to $750 due to scope of work

Payment summary:

@rayane-djouah $750 - amount updated on accepted contract and paid ✔️
@allgandalf $750 - original contract not yet accepted so sent a new one with updated amount - paid ✔️

melvin-bot[bot] commented 20 hours ago

Upwork job price has been updated to $750

sonialiap commented 20 hours ago

All paid 🎉

Expensify / App