[HOLD for payment 2023-10-23] [$500] [distance] Don't disable the Next button. Show error message

[JmillsExpensify]

Edited by @neil-marcellini :

Context

Currently to get valid waypoints we do the following:

• Remove waypoints with an empty address
• Remove adjacent duplicates

Problem

If there are < 2 valid waypoints we disable the next button.

Solution

Instead of doing that we should show an error message. If the original waypoints have an empty start or finish the error message should say something like “Please enter at least two waypoints”. Otherwise the error message should say that there are duplicate waypoints.

Tests / expected behavior

Empty

1. Empty waypoints
2. Click next
3. Verify an error appears asking you to enter waypoints

One empty stop

1. Start waypoint A
2. empty waypoint
3. Finish waypoint B
4. Click next
5. Verify that you are navigated without errors

Same start and finish

1. start waypoint A
2. finish waypoint A
3. Click next
4. Verify an error appears about duplicate waypoints

Duplicate start

1. start waypoint A
2. stop waypoint A
3. finish waypoint B
4. Click next
5. Verify that you are navigated without errors

Duplicate start, duplicate stop

1. start waypoint A
2. stop waypoint B
3. stop waypoint B
4. finish waypoint A
5. Click next
6. Verify that you are navigated without errors

cc @shawnborton @hayata-suenaga

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01a0151b41111ef451
• Upwork Job ID: 1700190648329519104
• Last Price Increase: 2023-09-22
• Automatic offers:
• BhuvaneshPatil | Contributor | 26874748

[melvin-bot]

Current assignee @JmillsExpensify is eligible for the Bug assigner, not assigning anyone new.

[melvin-bot]

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

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

Current assignee @JmillsExpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[BhuvaneshPatil]

Proposal

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

Bottom Dock the next button and keep it enabled all the time, in case of invalid data show message

What is the root cause of that problem?

• Map is not taking full height
  We have set maximum height to mapViewContainer, we shall remove it so that it will take remaining space vertically.
  As map will take all the space vertically the next button will always be at the end
• For button being disabled
  We are applying disabled prop to button in DistanceRequest component.

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

• Remove maxHeight from mapViewContainer styles.
• To fix the the next button at bottom and always visible when we scroll other content, we shall take it out of ScrollView.
  We shall wrap the whole content in DistanceRequest with View, then put the Button in separate view (outside
  ScrollView). And apply flex1 style to the wrapper view so it takes full screen height.
• We have also decided to remove scroll behaviour for waypoints, for that we shall remove this styling
  

  App/src/components/DistanceRequest.js

  Line 207 in 04215cb

  style={styles.distanceRequestContainer(scrollContainerMaxHeight)}

• To handle the error message upon clicking the next, we shall add DotIndicatorMessage component with Button and wrap them in separate view

       <View style={[styles.pt2]}>
               {((userTriedSubmitting && _.size(validatedWaypoints) < 2) || hasRouteError) && (
                   <DotIndicatorMessage
                       type="error"
                       style={[styles.mb2, styles.ml4]}
                       messages={getError()}
                   />
               )}
               <Button
                   success
                   style={[styles.w100, styles.mb2, styles.ph4, styles.flexShrink0]}
                   onPress={() => {
                       setUserTriedSubmitting(true);
                       if (_.size(validatedWaypoints) < 2 || hasRouteError) {
                           return;
                       }
                       navigateToNextPage();
                   }}
                   isDisabled={isOffline}
                   text={translate('common.next')}
               />
           </View>

  The criteria for displaying error message would be to check if user tried submitting the details, userTriedSubmitting state will manage that. And if we have error message we show that. However we use hasRouteError below the waypoints as well, so we shall discuss that as well. getError() will give us correct message.
  When user tries submitting, we check for required conditions and then only execute navigateToNextPage()
  As mentioned [HOLD for payment 2023-10-23] [$500] [distance] Don't disable the Next button. Show error message #27045 (comment) we want to shift the error near next button, for that we above code will work and we need to remove this code block -

  App/src/components/DistanceRequest.js

  Lines 252 to 257 in f39e93f

  	{hasRouteError && (
  	<DotIndicatorMessage
  	style={[styles.mh5, styles.mv3]}
  	messages={ErrorUtils.getLatestErrorField(transaction, 'route')}
  	type="error"
  	/>

Result on desktop web

Screen.Recording.2023-09-21.at.1.13.01.PM.mov

Result on IOS

Screen.Recording.2023-09-21.at.1.16.52.PM.mov

Error Handling

Screen.Recording.2023-09-21.at.3.59.40.PM.mov

PS - I have added padding top in IOS (realised it later)

What alternative solutions did you explore? (Optional)

[sicarius97]

Proposal

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

Button shows as disabled which violates the form rules and also needs to snap to the bottom of the scroll view.

What is the root cause of that problem?

The root cause is that a quick fix was issued to fix certain screen sizes rather than having a more robust solution, which causes the layout to break on small screens. Since this was a quick fix as well, the button never adhered to the form rules

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

Rather than like above proposal, which changes the layout of that container and restricts the map size, instead I think that button should be anchored to the bottom of the scrollview container in an alternate fashion that doesn't also change the layout as mentioned instead.

As for the button being disabled, we simply need to make a couple of adjustments on what happens based on its state and show an error message when clicked at an improper time (via a tooltip or simple indicator component, whichever is more consistent throughout the app). We could also create an error/validation component to make this more extensible and reusable throughout the app in the future for button inputs

What alternative solutions did you explore? (Optional)

[yongwangd]

Proposal

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

1. Place the Next button at the bottom of the screen
2. Keep the Next button always active. Show an error if two waypoints are missing.

What is the root cause of that problem?

for problem 1 - the next button is not placed at the bottom

App/src/components/DistanceRequest.js

Lines 166 to 174 in 5c13816

	return (
	<ScrollView contentContainerStyle={styles.flexGrow1}>
	<View
	style={styles.distanceRequestContainer(scrollContainerMaxHeight)}
	onLayout={(event = {}) => setScrollContainerHeight(lodashGet(event, 'nativeEvent.layout.height', 0))}
	>
	<ScrollView
	onContentSizeChange={(width, height) => {
	if (scrollContentHeight < height && numberOfWaypoints > numberOfPreviousWaypoints) {

The inner ScrollView takes the whole screen height and it has flex:1. All its children will align from top to bottom.

for problem 2 - the next button is not always active.

App/src/components/DistanceRequest.js

Lines 263 to 269 in 5c13816

	<Button
	success
	style={[styles.w100, styles.mb4, styles.ph4, styles.flexShrink0]}
	onPress={() => IOU.navigateToNextPage(iou, iouType, reportID, report)}
	isDisabled={_.size(validatedWaypoints) < 2}
	text={translate('common.next')}
	/>

The button has a isDisabled property that will disable the button when the size of validatedWaypoints is < 2

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

For addressing problem 1

position the button at the bottom of the screens with sufficient vertical space. If the screen is extremely short. Using position: fixed is not a good solution, as the button will always take up valuable vertical space and will lead to a bad user experience. See this image (bad user experience on short screens)

Proposed Solution: Implement a 'margin-top: auto' style property to the button, which in our codebase can be achieved by applying the 'styles.mtAuto' class to the button element. This solution ensures that the button is positioned at the bottom on taller screens and remains hidden within the scroll view on extremely short screens.

For addressing problem 2

Proposed Solution: It's a good user experience to always show the button as active, but only show the error message after the user clicks on the button. Always showing the error message without the user even interacting with the form can frustrate the user.
To achieve it, we need to keep track of whether the button has been clicked, if so, use the same condition to show the error message

exp-issue-27045.mov

What alternative solutions did you explore? (Optional)

[JmillsExpensify]

@BhuvaneshPatil for this question.

Shall we show error only when user tries to click on next button ( for this we will need to add new state for tracking if user has clicked the button or not}

We show the only error when the Next button is tapped, per our form guidelines.

[hayata-suenaga]

Shall we show error only when user tries to click on next button ( for this we will need to add new state for tracking if user has clicked the button or not

@BhuvaneshPatil I have a question about your proposal. Why do we need to add a new state for this?

[BhuvaneshPatil]

About that @hayata-suenaga , The state will be used like following - When have to show the error only if user has clicked the button, to achieve this we will need a state.
user clicks the button -> check if waypoints are valid -> if not ( set the state to true and show error)

[hayata-suenaga]

Can we just check if the error exists or not?

[BhuvaneshPatil]

as @JmillsExpensify mentioned, we only want to show error when user clicks on button, we have information about errors, we need to show that in UI only when user clicks the button. Here is code snippet -

<View>
                {_.size(validatedWaypoints) < 2 && userTriedSubmitting && (
                    <DotIndicatorMessage
                        style={[styles.mh5, styles.mv3]}
                        messages={{8217392: 'Minimum two routes required'}}
                        type="error"
                    />
                )}
                <Button
                    success
                    style={[styles.w100, styles.mb4, styles.ph4, styles.flexShrink0]}
                    onPress={() => {
                        setUserTriedSubmitting(true);
                        if (_.size(validatedWaypoints) < 2) return;
                        IOU.navigateToNextPage(iou, iouType, reportID, report);
                    }}
                    text={translate('common.next')}
                />
            </View>

in summary, we know what to show we need state for when to show.

[melvin-bot]

@JmillsExpensify, @allroundexperts Whoops! This issue is 2 days overdue. Let's get this updated quick!

[hayata-suenaga]

ah I see thank you for the explanation.

@allroundexperts could you review proposals?

[melvin-bot]

Triggered auto assignment to @shawnborton (Design), see these Stack Overflow questions for more details.

[JmillsExpensify]

Still waiting for @allroundexperts to review proposals though I also added design for a second set of eyes.

[allroundexperts]

For the first problem, I think we have a built in component that achieves the bottom docking. I would rather go with a proposal that makes use of it instead of re-inventing the wheel.

[melvin-bot]

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

• feat: enable next button #29125

If no regressions arise, payment will be issued on 2023-10-23. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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

• @BhuvaneshPatil requires payment offer (Contributor)
• @allroundexperts does not require payment (Eligible for Manual Requests)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

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:

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

[melvin-bot]

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

• feat: enable next button #29125

If no regressions arise, payment will be issued on 2023-10-23. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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

• @BhuvaneshPatil requires payment offer (Contributor)
• @allroundexperts does not require payment (Eligible for Manual Requests)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

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:

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

[JmillsExpensify]

@allroundexperts mind kicking us off with the BZ checklist?

[allroundexperts]

Yep. On it today!

[JmillsExpensify]

@JmillsExpensify Can you please consider timings for bonus here, given the issue was put on hold 2-3 times. And the PR got merged in 3 days from creation(l1-2 hr late ig because of lint test). Thanks.

Can you expound on this outside of this PR?

[allroundexperts]

@JmillsExpensify I think that @BhuvaneshPatil is correct. The PR got raised on Oct 10 and got merged on Oct 13 (Even then it was about 2 hours late because our lint checks were running slow that day).

[allroundexperts]

Checklist

1. N/A (Since this was a behaviour change that we wanted to do after the original PR got merged)
2. N/A
3. N/A
4. Regression test would be good. The steps mentioned here should be good enough!

[neil-marcellini]

That's a regression ^ btw

[allroundexperts]

@BhuvaneshPatil Can you please check?

[BhuvaneshPatil]

As the code is already deployed to production, I reverted my PR on latest main and tested.
The button is not disabled, hence the same waypoints were considered as valid before adding my changes.

Screen.Recording.2023-10-19.at.7.25.10.AM.mov

So I think this is not a regression but an undetected bug. Rest is upto you to decide.

[JmillsExpensify]

@neil-marcellini Any thoughts on that last comment? I believe our policy is that whether it's a regression or an undetected issue, both are subject to a decrease in pay. I'd love a confirmation though, thanks!

[neil-marcellini]

I reverted my PR on latest main and tested.
The button is not disabled, hence the same waypoints were considered as valid before adding my changes.

I see. Thank you @BhuvaneshPatil for testing and providing evidence. It definitely wasn't a regression from your PR. It would have been a good case to test, since I did say this in the issue description solution.

If the original waypoints have an empty start or finish the error message should say something like “Please enter at least two waypoints”.

That being said, I didn't consider that we could have 2 valid waypoints and the start or finish waypoint could be empty, so I didn't see it coming either. @JmillsExpensify I don't think we should reduce anyone's pay for this.

[JmillsExpensify]

Thanks! Payout summary as follows:

• Issue reporter: N/A
• Contributor: @BhuvaneshPatil $750 (incl. bonus for this case)
• Contributor+: @allroundexperts $750 (incl. bonus)

[JmillsExpensify]

@allroundexperts in the future, please paste the regression steps in this issue. That makes it just a little bit easier to tackle next steps.

[JmillsExpensify]

Ah, you're just linking to the OP above. Disregard my last comment, all good!

[JmillsExpensify]

$750 payment approved for @allroundexperts based on summary.

[JmillsExpensify]

Regression test created and Contributor contract paid. Closing this issue.