[HOLD for payment 2023-11-09] [$500] Same page has different error text color for different errors

@dhanashree-sawant

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: 1.3.85-1
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1697540779491219

Action Performed:

Open the app
Click on plus and click on request money
In manual, without typing any numbers, click next
Observe error text color is red
Switch to distance and without selecting any waypoint, click next
Observe that error text color is white

Expected Result:

App should maintain same error text color on same pages

Actual Result:

App displays manual request money errors in red but distance errors in white even though they are on same RHN page

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Android: Native

Android.native.same.page.different.error.color.mp4

Android: mWeb Chrome

Android.chrome.same.page.different.error.color.mp4

iOS: Native

ios.native.same.page.different.error.color.mov

iOS: mWeb Safari

ios.safari.same.page.different.error.color.mov

MacOS: Chrome / Safari

mac.chrome.same.page.different.error.colors.mov

Recording.112.mp4

MacOS: Desktop

mac.desktop.same.page.different.error.color.mov

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01f1225cd87883fded
Upwork Job ID: 1714343355796758528
Last Price Increase: 2023-10-17
Automatic offers:

Victor-Nyagudi | Contributor | 27357473
dhanashree-sawant | Reporter | 27357475

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

@alexpensify

Triggered auto assignment to @alexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

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

Proposal

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

Same page has different error text color for different errors

What is the root cause of that problem?

We are using different components to show error.
for example

App/src/components/DistanceRequest/index.js

Lines 227 to 233 in 75a2cc9

    
           {((hasError && _.size(validatedWaypoints) < 2) || hasRouteError) && ( 
        
               <DotIndicatorMessage 
        
                   style={[styles.mh4, styles.mv3]} 
        
                   messages={getError()} 
        
                   type="error" 
        
               /> 
        
           )}

App/src/pages/iou/steps/MoneyRequestAmountForm.js

Lines 265 to 271 in cc6a1e3

    
           {!_.isEmpty(formError) && ( 
        
               <FormHelpMessage 
        
                   style={[styles.ph5]} 
        
                   isError 
        
                   message={translate(formError)} 
        
               /> 
        
           )}

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

We can either use DotIndicatorMessage component in both the places
the solution would look something like this

    {!_.isEmpty(formError) && (
                    <DotIndicatorMessage
                        style={[styles.mh4, styles.mv3]}
                        messages={translate(formError)}
                        type="error"
                    />
                )}

What alternative solutions did you explore? (Optional)

We can also use FormHelpMessage compoenent in both the places
the solution would look something like this

    {((hasError && _.size(validatedWaypoints) < 2) || hasRouteError)  && (
                <FormHelpMessage
                    style={[styles.ph5]}
                    isError
                    message={getError()}
                />
            )}

@thesahindia

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

Proposal

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

Inconsistent error message styling between the distance request page and the manual request pages.

What is the root cause of that problem?

In the manual request page we use a component called FormHelpMessage that has the red dot and a white text when it's an error. But on the Distance page we are currently using another component called DotIndicatorMessage.

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

In the MoneyRequestAmountForm we can display use the DotIndicatorMessage to have the same styling

<DotIndicatorMessage
       style={[styles.ph5]}
       messages={{0: formError}}
       type="error"
/>

What alternative solutions did you explore? (Optional)

Depending on which one should be the standard look, there is also the option of using FormHelpMessage in the distance request page, but I personally prefer to keep using the DotIndicatorMessage component, as it's used in many other forms.

DotIndicator.mp4

N/A

Proposal

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

Same page has different error text color for different errors

What is the root cause of that problem?

The error shown during a manual money request flow is from FormHelpMessage component while the one shown during a distance request comes from DotIndicatorMessage.

The manual money request error text is set to red by styles.formError.

App/src/styles/styles.ts

Lines 1193 to 1194 in 9d79fd1

    
           formError: { 
        
               color: theme.textError,

App/src/styles/themes/default.ts

Line 53 in 9d79fd1

textError: colors.red,

The distance request error text, however, is set to light greyish-green by styles.offlineFeedback.text.

App/src/styles/styles.ts

Lines 2871 to 2872 in 9d79fd1

    
           text: { 
        
               color: theme.textSupporting,

App/src/styles/themes/default.ts

Line 19 in 9d79fd1

textSupporting: colors.darkSupportingText,

App/src/styles/colors.js

Line 21 in 9d79fd1

darkSupportingText: '#AFBBB0',

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

I propose setting the text color to red (same as the one in manual money request error, for consistency's sake) in DotIndicatorMessage by changing the color property if props.type is equal to error.

style={[styles.offlineFeedback.text, props.type === 'error' && {color: styles.formError.color}]}

This prop is also what we're currently using to determine the color of the dot indicator (line 60).

App/src/components/DotIndicatorMessage.js

Lines 58 to 61 in 9d79fd1

    
           <Icon 
        
               src={Expensicons.DotIndicator} 
        
               fill={props.type === 'error' ? themeColors.danger : themeColors.success} 
        
           />

The duplicate error check could potentially be extracted to a variable like hasError and used in both places to keep the code DRY.

Short video after making changes

expensify-distance-request-error-red.mov

What alternative solutions did you explore? (Optional)

I was genuinely curious why the error text in a distance request was made light greyish-green in the first place whereas the other errors I've seen so far have been red.

I replaced the style property of the text in DotIndicatorMessage to be style={styles.formError}, and this also worked fine.

Both styles.formError, currently used by FormHelpMessage, and styles.offlineFeedback.text, currently used by DotIndicatorMessage, adjust the font size (to the same value) and color.

The main difference is styles.formError also adjusts the line height and margin bottom while styles.offlineFeedback.text only adjusts the vertical text alignment in addition.

However, after further investigation, I discovered the light greyish-green text is most likely for success messages given the dot indicator has two colors - red for error, and green for success.

Proposal

Updated

@Victor-Nyagudi

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

Fix/29812: Same page has different error text color for different errors #30325

If no regressions arise, payment will be issued on 2023-11-09. 🎊

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:

@Victor-Nyagudi requires payment offer (Contributor)
@thesahindia does not require payment (Eligible for Manual Requests)
@dhanashree-sawant requires payment offer (Reporter)

@thesahindia

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:

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

@thesahindia

@thesahindia - to prepare for the payment date, can you please complete the checklist? Thanks!

It was an improvement. I think we just have to add a generic test case.

Steps -

Go to any page and reproduce some error
Verify the error message is in red

@dhairyasenjaliya

Here is the payment summary:

External issue reporter @dhairyasenjaliya $50
Contributor that fixed the issue @Victor-Nyagudi $500
Contributor+ that helped on the issue and/or PR @thesahindia $500

Upwork Job: https://www.upwork.com/jobs/~01f1225cd87883fded

*If applicable, the bonuses will be applied on the final payment

Extra Notes regarding payment: This one was before the pre-urgency bonus change, but it was merged in 4 business days-- no bonus here.

I'll complete the payment process tomorrow since it will be 11-09.

@alexpensify

@alexpensify Please take a look at the following with regard to the PR timeline and urgency bonus.

Exact time I got assigned

Exact time PR was marked ready for review

The PR was ready for review in roughly 8 hours (7h 49min), as seen from the above screenshots.

Exact time I bumped C+

By the time I bumped the C+, over 2 days had already elapsed since I marked my PR ready for review. The PR was ready on a Wednesday, and I bumped the C+ on a Friday.

Amount of time between when PR was ready and when I bumped C+

The feedback finally came on a Saturday, but the timer doesn't count weekends - only business days.

Exact time C+ provided initial feedback

I was available at the time and acted on the feedback immediately.

Mind you, I had already fixed this issue, but I had discovered other areas the bug occurred and was looking for feedback on if we should tackle those other areas as well.

After making the changes to address those other areas and fixing merge conflicts, the C+ approved the PR.

Exact time C+ approved the changes

It only took roughly 17 hours between the time I got some feedback to the time the C+ approved the changes.

Time between feedback and C+ approval

Given just how fast the changes were approved, had I gotten the feedback even one day sooner, the PR would've been merged within 3 business. days, and therefore eligible for the urgency bonus.

The PR got merged the following day after approval.

Exact time PR got merged

I humbly request you to take into account this information and to kindly reconsider your stance on the urgency bonus.

@Victor-Nyagudi

Thank you @Victor-Nyagudi for this summary. After reviewing the payment summary again, I confirmed with the team that the PR for this one was created after the announcement (on October 24) related to the discontinuation of the urgency bonus. The conversation is here:

https://expensify.slack.com/archives/C01GTK53T8Q/p1698191269281379

My apology for not catching this sooner, but the base payment will remain the same.

Closing - everyone has been paid via Upwork. Thank you!

@alexpensify

@alexpensify Ah, I just missed the date then.

No worries. Thank you for the timely payment.

@thesahindia

$500 payment approved for @thesahindia based on this summary.

Navigation Menu

Repository navigation

[HOLD for payment 2023-11-09] [$500] Same page has different error text color for different errors #29812

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

Bug0 Triage Checklist (Main S/O)

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Proposal

30 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	{((hasError && _.size(validatedWaypoints) < 2) \|\| hasRouteError) && (
	<DotIndicatorMessage
	style={[styles.mh4, styles.mv3]}
	messages={getError()}
	type="error"
	/>
	)}

	{!_.isEmpty(formError) && (
	<FormHelpMessage
	style={[styles.ph5]}
	isError
	message={translate(formError)}
	/>
	)}

	<Icon
	src={Expensicons.DotIndicator}
	fill={props.type === 'error' ? themeColors.danger : themeColors.success}
	/>

[HOLD for payment 2023-11-09] [$500] Same page has different error text color for different errors #29812

Description

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

Activity

melvin-bot commented on Oct 17, 2023

melvin-bot commented on Oct 17, 2023

melvin-bot commented on Oct 17, 2023

Bug0 Triage Checklist (Main S/O)

saranshbalyan-1234 commented on Oct 17, 2023

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

melvin-bot commented on Oct 17, 2023

abdel-h66 commented on Oct 17, 2023

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Victor-Nyagudi commented on Oct 18, 2023

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Victor-Nyagudi commented on Oct 18, 2023

Proposal

30 remaining items

melvin-bot commented on Nov 2, 2023

melvin-bot commented on Nov 2, 2023

alexpensify commented on Nov 3, 2023

thesahindia commented on Nov 3, 2023

alexpensify commented on Nov 9, 2023

alexpensify commented on Nov 9, 2023

Victor-Nyagudi commented on Nov 9, 2023

alexpensify commented on Nov 9, 2023

alexpensify commented on Nov 9, 2023

Victor-Nyagudi commented on Nov 9, 2023

JmillsExpensify commented on Nov 13, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions