[HOLD for payment 2024-05-06] [$250] Can't update the custom time of the status to 00:59

@DylanDylann

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.4.63-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:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @DylanDylann
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1713463886729749

Action Performed:

Create a new status
Set up a custom clear time for a status to 00:59

Expected Result:

We should update successfully

Actual Result:

Error: Please choose a time at least one minute ahead

Workaround:

unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

Recording.2987.mp4

Screen.Recording.2024-04-19.at.00.31.22.mov

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~0187eaa0221ed74ecf
Upwork Job ID: 1781292572093497344
Last Price Increase: 2024-04-19
Automatic offers:

DylanDylann | Contributor | 0
nkdengineer | Contributor | 0

@garrettmknight

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

@garrettmknight

@garrettmknight I investigated this bug while discovering it. Could I take over this one as C+ role?

Proposal

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

The bug occurs not just for 00:59, it occurs for every time less than 1 hour. We cannot save the time

What is the root cause of that problem?

The bug occurs because the condition hour === 0 in the original code only checks if the hour part of the time is exactly 0. However, it doesn't consider the minutes part of the time. As a result, the condition hour === 0 allows times like 00:59 to pass through, even though they are less than one minute in the future.

We call validate below:

App/src/components/TimePicker/TimePicker.tsx

Lines 130 to 135 in 9033ccb

    
           const validate = useCallback( 
        
               (time: string) => { 
        
                   const isValid = DateUtils.isTimeAtLeastOneMinuteInFuture({timeString: time || `${hours}:${minutes} ${amPmValue}`, dateTimeString: defaultValue}); 
        
                   setError(!isValid); 
        
                   return isValid; 
        
               },

Which calls the below util:

App/src/libs/DateUtils.ts

Line 664 in 9033ccb

    
           const isTimeAtLeastOneMinuteInFuture = ({timeString, dateTimeString}: {timeString?: string; dateTimeString: string}): boolean => {

And we return false if hour is 0:

App/src/libs/DateUtils.ts

Lines 672 to 674 in 9033ccb

    
           if (hour === 0) { 
        
               return false; 
        
           }

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

To fix this bug, we need to adjust the condition to also consider the minutes part of the time. By checking if both the hour is 0 and the minutes are less than or equal to 59, we ensure that times like 00:59 are properly detected as less than one minute in the future.

We need to add a && checks well to see if value is less than < 59 for minutes

const isTimeAtLeastOneMinuteInFuture = ({ timeString, dateTimeString }: { timeString?: string; dateTimeString: string }): boolean => {
    let dateToCheck = dateTimeString;
    
    if (timeString) {
        const [hourStr, minuteStr] = timeString.split(/[:\s]+/);
        const hour = parseInt(hourStr, 10);
        const minute = parseInt(minuteStr, 10);

        // Check if time is 00:00
        if (hour === 0 && minute === 0) {
            return false;
        }

        dateToCheck = combineDateAndTime(timeString, dateTimeString);
    }

    const now = new Date();

    return isAfter(new Date(dateToCheck), addMinutes(now, 1));
};

What alternative solutions did you explore? (Optional)

Proposal

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

Can't update the custom time of the status to 00:59

What is the root cause of that problem?

There are two issue we need to handle here:

The hour === 0 check, this doesn' make a lot of sense here, I don't think we need that check.

App/src/libs/DateUtils.ts

Lines 672 to 674 in 9033ccb

if (hour === 0) {

return false;

}

The parsing issue, '00:59 AM' is not a standard representation of time. In a 12-hour clock system. Therefore, '00:59 AM' doesn't follow standard conventions because '00' usually represents midnight.

App/src/libs/DateUtils.ts

Lines 576 to 582 in 9033ccb

    
           } else if (updatedTime.includes(':')) { 
        
               // it's in "hh:mm a" format 
        
               const tempTime = parse(updatedTime, 'hh:mm a', new Date()); 
        
               if (isValid(tempTime)) { 
        
                   parsedTime = tempTime; 
        
               } 
        
           }

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

We need to first remove the hour === 0 check and then we need to make some change to turn the time into standard representation of time in a 12-hour clock system.

    } else if (updatedTime.includes(':')) {
        // it's in "hh:mm a" format
        const [time, ampm] = updatedTime.split(' ');
        const [hours, minutes] = time.split(':');
        let hour = parseInt(hours, 10);
        if (ampm === 'PM' && hour !== 12) {
            hour += 12;
        } else if (ampm === 'AM' && hour === 12) {
            hour = 0;
        }
        const tempTime = parse(`${hour}:${minutes}`, 'HH:mm', new Date());
        
        if (isValid(tempTime)) {
            parsedTime = tempTime;
        }
    }

What alternative solutions did you explore? (Optional)

@Krishna2323

@Krishna2323

PLEASE PRIORITIZING YOUR PR BEFORE POSTING NEW PROPOSAL

Proposal

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

Error: Please choose a time at least one minute ahead

What is the root cause of that problem?

We have 2 problems:

The value of the hour field is not accepted if it is 0

App/src/libs/DateUtils.ts

Lines 672 to 674 in 68e6962

    
           if (hour === 0) { 
        
               return false; 
        
           }

parse function of date-fns lib return invalid date if the hour is 0 because the range of this format in date-fns is 1->12, they use 12 instead of 0

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

First we need to create new field in en.tsx and es.tsx to introduce new error

invalidTimeRange: Please enter a time using the 12-hour clock format (e.g., 2:30 PM).

In TimePicker Component we should introduce new state called errorMessage

Then we need to update validate function

App/src/components/TimePicker/TimePicker.tsx

Line 130 in 64695c8

const validate = useCallback(

const validate = useCallback(
        (time: string) => {
            const timeString = time || `${hours}:${minutes} ${amPmValue}`
            const [hourStr] = timeString.split(/[:\s]+/);
            const hour = parseInt(hourStr, 10);
            if (hour === 0) {
                setError(true);
                setErrorMessage("common.error.acceptTerms")
                return false;
            }
            const isValid = DateUtils.isTimeAtLeastOneMinuteInFuture({timeString, dateTimeString: defaultValue});
            setError(!isValid);
            setErrorMessage("common.error.invalidTimeRange")
            return isValid;
        },
        [hours, minutes, amPmValue, defaultValue],
    );

Finally, we update the message field here to use new state errorMessage

App/src/components/TimePicker/TimePicker.tsx

Line 524 in 64695c8

message="common.error.invalidTimeShouldBeFuture"

Optional: We also should remove this condition because it is redundant

App/src/libs/DateUtils.ts

Lines 672 to 674 in 9033ccb

    
           if (hour === 0) { 
        
               return false; 
        
           }

What alternative solutions did you explore? (Optional)

NA

Proposal

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

Custom time of the status can't be set to any value with hour as 00.

What is the root cause of that problem?

We can add a new state errorMessage and update it via handleSubmit

const handleSubmit = () => {
    if (hours === '00') {
        setError(true); 
        setErrorMessage('common.error.useValidAmPmTime') // Add new key in en.ts and es.ts
        return; 
    }

    const time = `${hours}:${minutes} ${amPmValue}`;

    const isValid = validate(time);

    if (isValid) {
        onSubmit(time);
    }
};

Code can be polished.

Psuedo code for FormHelpMessage

<FormHelpMessage
    isError={isError}
    message={errorMessage ?? "common.error.invalidTimeShouldBeFuture"}
    style={styles.pl5}
/>

Same change will be needed in getTimeValidationErrorKey in DateUtils.

App/src/libs/DateUtils.ts

Lines 709 to 715 in 9a115b6

    
           const getTimeValidationErrorKey = (inputTime: Date): string => { 
        
               const timeNowPlusOneMinute = addMinutes(new Date(), 1); 
        
               if (isBefore(inputTime, timeNowPlusOneMinute)) { 
        
                   return 'common.error.invalidTimeShouldBeFuture'; 
        
               } 
        
               return ''; 
        
           };

const getTimeValidationErrorKey = (inputTime: Date): string => {
    const timeNowPlusOneMinute = addMinutes(new Date(), 1);
    if (inputTime.getHours() === 0) {
        return 'common.error.useValidAmPmTime';
    }
    if (isBefore(inputTime, timeNowPlusOneMinute)) {
        return 'common.error.invalidTimeShouldBeFuture';
    }
    return '';
};

What alternate solutions did you explore?

Below suggestions are from my original proposal, which have been moved to this section because design team mentioned that we only need to update the message.

Option 1:

We shouldn't be allowing the user to type 00.

As of now, when user types 14, it gets changed to 02 with PM. Same for other numbers > 12. Likewise, when user types 00, it should be changed to 12 AM.

App/src/components/TimePicker/TimePicker.tsx

Lines 221 to 227 in 305f12c

    
           const newHourNumber = Number(newHour); 
        
           if (newHourNumber > 24) { 
        
               newHour = hours; 
        
           } else if (newHourNumber > 12) { 
        
               newHour = String(newHourNumber - 12).padStart(2, '0'); 
        
               setAmPmValue(CONST.TIME_PERIOD.PM); 
        
           }

In the above, add the below condition:

else if (newHourNumber === 0) {
    newHour = String(12).padStart(2, '0')
    setAmPmValue(CONST.TIME_PERIOD.AM)
}

(Option 1 isn't good because when the user would delete the value for entering a new one, it will start showing 12 instead of 00)

Option 2:

When the user saves, we can change '00' in hours to '12', update to AM and then continue. Update the handleSubmit method in TimePicker.

const handleSubmit = () => {
    if (hours === '00') {
        setHours('12')
        setAmPmValue(CONST.TIME_PERIOD.AM);
    }

    const time = `${hours === '00' ? '12' : hours}:${minutes} ${hours === '00' ? CONST.TIME_PERIOD.AM : amPmValue}`;

    const isValid = validate(time);

    if (isValid) {
        onSubmit(time);
    }
};

Code can be polished.

Video for the above option 2

TimeChange.mov

@shawnborton

@shawnborton @dannymcclain Any thoughts on the flow shown in the video above?

Given that we are clearly using AM/PM here, I would think we'd just throw an error saying that you can't enter "00" for the hour.

So I think we just need a better error message.

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

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

@DylanDylann

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

Fix can't update the custom time of the status to 00:59 #40853

If no regressions arise, payment will be issued on 2024-05-06. 🎊

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

@DylanDylann requires payment automatic offer (Contributor)
@nkdengineer requires payment automatic offer (Contributor)
@DylanDylann requires payment automatic offer (Contributor)

@DylanDylann

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:

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

@DylanDylann

All paid out - @DylanDylann can you complete the checklist when you get a chance?

@DylanDylann

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:

[@DylanDylann] The PR that introduced the bug has been identified. Link to the PR: #24620
[@DylanDylann] 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: #24620 (comment)
[@DylanDylann] 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: NA
[@DylanDylann] Determine if we should create a regression test for this bug. Yes
[@DylanDylann] 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

Create a new status
Set up a custom clear time for a status to 00:59
Verify that: error Please enter a time using the 12-hour clock format is displayed

Do we agree 👍 or 👎

All set

Navigation Menu

Repository navigation

[HOLD for payment 2024-05-06] [$250] Can't update the custom time of the status to 00:59 #40495

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

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)

PLEASE PRIORITIZING YOUR PR BEFORE POSTING NEW PROPOSAL

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 alternate solutions did you explore?

44 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	const validate = useCallback(
	(time: string) => {
	const isValid = DateUtils.isTimeAtLeastOneMinuteInFuture({timeString: time \|\| `${hours}:${minutes} ${amPmValue}`, dateTimeString: defaultValue});
	setError(!isValid);
	return isValid;
	},

	} else if (updatedTime.includes(':')) {
	// it's in "hh:mm a" format
	const tempTime = parse(updatedTime, 'hh:mm a', new Date());
	if (isValid(tempTime)) {
	parsedTime = tempTime;
	}
	}

	const getTimeValidationErrorKey = (inputTime: Date): string => {
	const timeNowPlusOneMinute = addMinutes(new Date(), 1);
	if (isBefore(inputTime, timeNowPlusOneMinute)) {
	return 'common.error.invalidTimeShouldBeFuture';
	}
	return '';
	};

	const newHourNumber = Number(newHour);
	if (newHourNumber > 24) {
	newHour = hours;
	} else if (newHourNumber > 12) {
	newHour = String(newHourNumber - 12).padStart(2, '0');
	setAmPmValue(CONST.TIME_PERIOD.PM);
	}

[HOLD for payment 2024-05-06] [$250] Can't update the custom time of the status to 00:59 #40495

Description

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

Activity

melvin-bot commented on Apr 18, 2024

DylanDylann commented on Apr 18, 2024

allgandalf commented on Apr 18, 2024

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)

Krishna2323 commented on Apr 18, 2024

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)

DylanDylann commented on Apr 19, 2024

PLEASE PRIORITIZING YOUR PR BEFORE POSTING NEW PROPOSAL

nkdengineer commented on Apr 19, 2024

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)

ShridharGoel commented on Apr 19, 2024

Proposal

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

What is the root cause of that problem?

What alternate solutions did you explore?

ShridharGoel commented on Apr 19, 2024

shawnborton commented on Apr 19, 2024

shawnborton commented on Apr 19, 2024

melvin-bot commented on Apr 19, 2024

44 remaining items

melvin-bot commented on Apr 29, 2024

melvin-bot commented on Apr 29, 2024

melvin-bot commented on Apr 29, 2024

garrettmknight commented on May 9, 2024

DylanDylann commented on May 10, 2024

garrettmknight commented on May 10, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions