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

[m-natarajan]

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: 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:

1. Create a new status
2. 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?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

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

[melvin-bot]

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.

[DylanDylann]

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

[allgandalf]

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)

[Krishna2323]

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:

1. 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;
   	}

2. 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)

[DylanDylann]

@Krishna2323

PLEASE PRIORITIZING YOUR PR BEFORE POSTING NEW PROPOSAL

[nkdengineer]

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:

1. 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;
	}

2. 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

[ShridharGoel]

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

[ShridharGoel]

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

[shawnborton]

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.

[shawnborton]

So I think we just need a better error message.

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @shmaxey (Waiting for copy), see https://stackoverflow.com/c/expensify/questions/7025/ for more details.

[garrettmknight]

@DylanDylann added you for C+ after the external label. Please review the proposals - to @shawnborton's point, I think we'll need a better error message like:
"Please choose a valid time between 12:00 AM and 11:59 PM"
or
"Please choose a valid time on a 12-hour AM/PM clock."

@shmaxey what do you think?

[ShridharGoel]

Proposal

Updated

[melvin-bot]

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

[DylanDylann]

The selected one doesn't handle the changes in getTimeValidationErrorKey which is an important part.

Why do you think it is important

[arosiclair]

@ShridharGoel there's no RCA in your proposal. @nkdengineer's proposal LGTM

[melvin-bot]

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

[nkdengineer]

@DylanDylann we have open PR here
Currently we have an error in en.ts like this
invalidTimeRange: Please enter a time using the 12-hour clock format (e.g., 2:30 PM).
and I have defined in es.ts like this:
invalidTimeRange: 'Introduzca una hora utilizando el formato de reloj de 12 horas (por ejemplo, 2:30PM).',
Please confirm again @arosiclair @garrettmknight

[arosiclair]

@garrettmknight I can ask in #espanol-translations for a translation, but should we ask for english copy first?

[garrettmknight]

Thanks! Here's the english copy from @shmaxey

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

[arosiclair]

Ah okay missed that thanks. Reviewing the Spanish copy here

[melvin-bot]

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

[melvin-bot]

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)

[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:

• [@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:

[garrettmknight]

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

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

Do we agree 👍 or 👎

[garrettmknight]

All set