[HOLD for Payment 2024-08-26][$400] Accounting - NetSuite - No error message if no option is selected for segment display

[izarutskaya]

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.14-2
Reproducible in staging?: Y
Reproducible in production?: Y
Found when validating PR : #46155
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

1. Open the app
2. Log in with a new expensifail account
3. Click on FAB - New workspace
4. Enable "Accounting" in the "More features" page.
5. Navigate to "Accounting"
6. Connect to NetSuite and upgrade the workspace to Control when asked
7. Wait for the sync to finish
8. Navigate to Accounting - Import - Custom Segments/records
9. Click on the "Add custom segment/record" button
10. Start adding a custom segment with any rendom data until the "How should this custom segment be displayed in Expensify?" step
11. Click on the "Next" button without making a selection
12. Click on "fix the errors"

Expected Result:

There should be an error message to choose Tags or Report fields.

Actual Result:

No error message given if no option is selected for segment display. Nothing happens if I click on "fix the errors".

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

Bug6557338_1722345767013.bandicam_2024-07-30_15-19-57-971.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~015eb22121c664b750
• Upwork Job ID: 1819084414296998813
• Last Price Increase: 2024-08-26
• Automatic offers:
• ikevin127 | Reviewer | 103426964
• cretadn22 | Contributor | 103426965

Issue Owner

Current Issue Owner: @ikevin127

[melvin-bot]

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

[izarutskaya]

We think this issue might be related to the #wave-control.

[melvin-bot]

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

[melvin-bot]

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

[ikevin127]

@cretadn22 Thank you for the proposal. Unfortunately, your proposed solution does not attempt to address the issue as per the Expected result since it's proposing changing the type of form / form functionality instead of fixing the fix the errors button which currently does not seem to do anything to notify the user that they should choose one of the available options.

I will ask the design team their take on this and once we have a more detailed idea of what we want to do here from a visual POV, then you can update your proposal to match the expectations if still interested 🙂

@Expensify/design Do we have a specific design / behaviour for a form of this type when clicking fix the errors ?

If there's no predefined / expected behaviour, I'm wondering: what if when clicking fix the errors we highlight the first option, same behaviour like when the option is hovered over (see video below) but maybe with a blink animation or something similar to draw attention.

The main idea here is to fulfil the Expected result which says:

There should be an error message to choose Tags or Report fields.

Which, as a developer I translate to:
Wwhen the user clicks on fix the errors - we should notify them visually that they have to choose one of the options.

Wdyt ?

Screen.Recording.2024-08-02.at.18.35.52.mov

[cretadn22]

Step 5:

Step 1:

Yes, I’ve noticed that in the initial step, the "Next" button doesn’t appear if the page is a selection list. Therefore, I recommend hiding the "Next" button on the Step 5 page and users move to the next page immediately after selecting an option.

[dubielzyk-expensify]

We generally have the first option pre-selected, but in this case I'm not sure that's desirable.

I'd at least want the error message to simply just say "Please select an option above" or something like that.

Keen to hear @Expensify/design thoughts too

[shawnborton]

Yeah, I agree with Jon's suggestion here.

[cretadn22]

@shawnborton @dubielzyk-expensify Should we follow the same procedure as in step one? Please review this comment

[cretadn22]

I'm uncertain about the need to show the Next button in step 5. For consistency with step 1, I believe we should hide the Next button in step 5.

[shawnborton]

Yes, I’ve noticed that in the initial step, the "Next" button doesn’t appear if the page is a selection list. Therefore, I recommend hiding the "Next" button on the Step 5 page and users move to the next page immediately after selecting an option.

I forget where we landed with this, but I thought we always wanted to show a Next button for selection lists when we are in one of these sequential stepper flows.

[cretadn22]

@shawnborton There are many places where landed with this.

[shawnborton]

How do we avoid this scenario then where an option is preselected and there's no way to advance to the next step?

[cretadn22]

@shawnborton Got it. Do you think we should add the Next button to the step 1 page? If so, I'll include that into my updated proposal.

[cretadn22]

Edited by proposal-police: This proposal was edited at 2024-08-06 14:57:37 UTC.

Proposal

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

What is the root cause of that problem?

New logic (we haven't implemented the error message for this case)

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

We need to show a new message such as "Please select an option above" and hide the old message "Please fix the errors....."

Currently, this functionality is supported only for TextInput fields. We need to extend this logic to display an error message in NetSuiteCustomFieldMappingPicker.

Step 1:

App/src/pages/workspace/accounting/netsuite/import/NetSuiteImportCustomFieldNew/NetSuiteImportAddCustomSegmentPage.tsx

Lines 124 to 125 in 1eef6f5

	case CONST.NETSUITE_CUSTOM_FIELD_SUBSTEP_INDEXES.CUSTOM_SEGMENTS.MAPPING:
	return ValidationUtils.getFieldRequiredErrors(values, [INPUT_IDS.MAPPING]);

In the validate function, a new error message should be added to the validate function when the value is empty.

                case CONST.NETSUITE_CUSTOM_FIELD_SUBSTEP_INDEXES.CUSTOM_SEGMENTS.MAPPING:
                    if (!ValidationUtils.isRequiredFulfilled(values[INPUT_IDS.MAPPING])) {
                        errors[INPUT_IDS.MAPPING] = "Please select an option above"
                    }
                    return errors;

Step 2: Since errorText is already passed to the child component by FormProvider, we only need to use the errorText prop in NetSuiteCustomFieldMappingPicker and display the new message.

Note: As mentioned earlier, errorText is automatically provided by FormProvider.

            {!!errorText && (
                <View style={styles.ph5}>
                    <FormHelpMessage
                        isError={!!errorText}
                        message={errorText}
                    />
                </View>
            )}

Additionally, add the shouldHideFixErrorsAlert prop to FormProvider to hide the old error message

What alternative solutions did you explore? (Optional)

The first option to be pre-selected. In NetSuiteCustomFieldMappingPicker, I recommend using the same logic as we applied on other pages.

[cretadn22]

@ikevin127 I've posted a new proposal and hidden the old one. Could you please review it?

[ikevin127]

@cretadn22 I don't see any clear statement from @Expensify/design confirming which way we should go with the implementation.

Given this, please drop a video of the behaviour from your existing proposals for us to see and decide which version is desirable here.

Note: The reason why I suggested not rushing in a new proposal / assuming a decision before we have a clear response from the design team is because it might end up not being the desired behaviour.

In the future you can simply post a few vids of the behaviour of different versions - as that might be easier to review by design, and once confirmed you can safely update / post a new proposal.

[shawnborton]

I think that would be my vote, since this is a sequential stepper flow, I like having the Next button at the bottom. Curious what the rest of the @Expensify/design team thinks though!

[dannymcclain]

Makes sense to me to treat them the same!

since this is a sequential stepper flow, I like having the Next button at the bottom

We may want to go ahead and make this The Law™: Stepper flows always use a next button at the bottom and never auto-advance upon selection.

[dubielzyk-expensify]

I'm okay with that. Just be aware that in another flow around workspace cards we break this The Law™ then 😅 We can take that separate though.

[cretadn22]

@shawnborton @dannymcclain @dubielzyk-expensify Could someone assist in confirming the Spanish translation for the error message: "Please select an option above"?

[shawnborton]

Usually design doesn't handle translation requests, cc @blimpich

My best guess would be Por favor elige una opción arriba though

[cretadn22]

Asking here: https://expensify.slack.com/archives/C01GTK53T8Q/p1723118396756249

[melvin-bot]

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

[blimpich]

Asking here: expensify.slack.com/archives/C01GTK53T8Q/p1723118396756249

responded in thread

[ikevin127]

PR is in Review and we're closing in for merge! 🚀

[slafortune]

This is being worked on, it will have a 7-day regression period, so I don't feel the need to add another BZ person to this.
I'll be out until 8/21 and will check in on this then.

[ikevin127]

⚠️ Automation failed here -> this should be on [HOLD for Payment 2024-08-26] according to yesterday’s production deploy from #47091 (comment).

cc @slafortune

[cretadn22]

Request to increase payment amount

The PR extends beyond the scope of the original bug and the additional fix represents a significant modification. Below is my summary:

The original fix: adding a new error message on step 5 of the "Add Custom Segment" page

The additional fix (the issue description does not mention this):

• Adding a new error message and a new button on step 1 of the "Add Custom Segment" page (detail here)
• Fix highlight bugs on three places (detail here and here)

[blimpich]

@cretadn22 I agree, and I'm happy that we fixed a lot of small issues in addition to the original issue here. How about we bump it up to $400?

[ikevin127]

cc @slafortune

[melvin-bot]

Upwork job price has been updated to $400

[slafortune]

@ikevin127 please complete the checklist -
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@ikevin127 ] The PR that introduced the bug has been identified. Link to the PR:
• [@ikevin127 ] 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:
• [@ikevin127 ] 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:
• [@ikevin127 ] Determine if we should create a regression test for this bug.
• [@ikevin127 ] 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.
• [@slafortune ] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[slafortune]

@cretadn22 paid via Upworks

[ikevin127]

• [@ikevin127] The PR that introduced the bug has been identified. Link to the PR: [#Wave-Control: Add NetSuite]: Settings Configuration in NewDot: Import - Add Custom Records/Lists #44942
• [@ikevin127] 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/files#r1731682353
• [@ikevin127] 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: N/A.
• [@ikevin127] Determine if we should create a regression test for this bug.
• [@ikevin127] 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. Click on FAB - New workspace.
2. Enable "Accounting" in the "More features" page.
3. Navigate to "Accounting".
4. Connect to NetSuite and upgrade the workspace to Control when asked.
5. Wait for the sync to finish.
6. Navigate to Accounting - Import - Custom Segments/records.
7. Click on the "Add custom segment/record" button.
8. In the first step, Click on the "Next" button without making a selection.
9. Verify that a new error message appear: "Please select an option above.".
10. Start adding a custom segment until the "How should this custom segment be displayed in Expensify?" step.
11. Click on the "Next" button without making a selection.
12. Verify that a new error message appear: "Please select an option above.".

Do we agree 👍 or 👎.

cc @slafortune

[slafortune]

@ikevin127 paid via Upworks