[$500] Status - Error message & emoji status remains when clear status timer has elapsed

lanitochka17 commented 11 months ago

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.13-0 Reproducible in staging?: Y Reproducible in production?: N 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: Applause - Internal Team Slack conversation:

Issue found when executing PR https://github.com/Expensify/App/pull/24620

Action Performed:

Precondition: user should be Signed In

Open https://staging.new.expensify.com/
Click on Profile icon > Status
Set any status emoji and click on Save
Click on Status
Click on "Clear after" > Custom
Set the time 5 min ahead and click on Save button
Wait about 6 min or more
Open the Clear status option

Expected Result:

The status should be cleared when the clear timer has elapsed

Actual Result:

Please choose a time at least one minute ahead" error message is displayed, and the emoji status remains when the clear status timer has elapsed

Workaround:

Unknown

Platforms:

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

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/7644a577-f91c-42b3-b93c-9b7b310f3e82

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~013df2712d0a3d5327
Upwork Job ID: 1737192168446410752
Last Price Increase: 2024-01-09

github-actions[bot] commented 11 months ago

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

Identify the pull request that introduced this issue and revert it.
Find someone who can quickly fix the issue.
Fix the issue yourself.

melvin-bot[bot] commented 11 months ago

Triggered auto assignment to @deetergp (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

stitesExpensify commented 11 months ago

Looking into this

melvin-bot[bot] commented 11 months ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

abzokhattab commented 11 months ago

Proposal

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

The status is not sent to the backend in UTC format

What is the root cause of that problem?

we dont convert the clearAfter Date to UTC before its sent to the backend

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

There is two ways here:

either we convert the dates inside the dateutil functions so that the User.js inside the actions folder and the status components don't have to deal with the conversion of timezone .. however, this solution requires changes in multiple areas and might cause breaking changes. (you can find more details below)
or we convert the date into UTC only on sending the data to the database which means the currentUserPersonalDetails will always have the status as utc however the DRAFT status will be in local date because the draft status is not yet sent to the server so it shouldnt be parsed as UTC.

with that being said the changes of the second solution should not introduce breaking change so lets dig into it:

first, when writing the data to the server we need to convert it into UTC format so we need to change the update function to:

function updateCustomStatus(status) {
    const clearAfterInUtc = DateUtils.formatWithUTCTimeZone(status.clearAfter, 'yyyy-MM-dd HH:mm:ss');
    const newStatus = {
        text: status.text,
        emojiCode: status.emojiCode,
        clearAfter: clearAfterInUtc,
    };
    API.write('UpdateStatus', newStatus, {
        optimisticData: [
            {
                onyxMethod: Onyx.METHOD.MERGE,
                key: ONYXKEYS.PERSONAL_DETAILS_LIST,
                value: {
                    [currentUserAccountID]: {
                        status: newStatus,
                    },
                },
            },
        ],
    });
}

then for the reading part, we need to add a new function inside the DateUtils file that converts the UTC string into a local date string given a certain format (this is the opposite of formatWithUTCTimeZone)

so i have defined this funciton :

function formatUTCToLocal(dateString: string, dateFormat: string = CONST.DATE.FNS_FORMAT_STRING) {
    if (dateString === '' || !dateString) {
        return dateString;
    }

    const date = parse(dateString, dateFormat, new Date());
    // Check if the date is valid
    if (!isValid(date)) {
        return '';
    }

    const utcDate = zonedTimeToUtc(date, 'UTC');
    const localDate = zonedTimeToUtc(utcDate, timezone.selected);
    // the timezone.selected is the timezone that the user selected at profile/timezone
     return tzFormat(localDate, dateFormat, {timeZone: timezone.selected});
}

then we have two solutions:

either to add a new middleware function that would update the status.clearAfter data returned from the server to local time (this way is preferrable)

to do so we need to add a new middleware to this folder: https://github.com/Expensify/App/blob/82b76c1a36cf1a01b00c14510a5812a591e7f2fd/src/libs/Middleware then we add the following middleware file:

FormatStatusClearAfter.ts

```js import DateUtils from '@libs/DateUtils'; import type {Middleware} from '@libs/Request'; import ONYXKEYS from '@src/ONYXKEYS'; import type {PersonalDetailsList} from '@src/types/onyx'; /** * Middleware function to format the 'clearAfter' field in personal details list. * * @param {any} requestResponse - The response from the request. * @param {any} request - The original request data. * @param {boolean} isFromSequentialQueue - Flag to indicate if the request is from a sequential queue. * @returns {Promise} - The processed request response. */ const formatStatusClearAfter: Middleware = (requestResponse) => requestResponse.then((response) => { const responseOnyxData = response?.onyxData ?? []; responseOnyxData.forEach((onyxData) => { if (onyxData.key !== ONYXKEYS.PERSONAL_DETAILS_LIST) { return; } const personalDetailsList = onyxData.value as PersonalDetailsList; Object.keys(personalDetailsList).forEach((accountID) => { const status = personalDetailsList[accountID]?.status; const clearAfter = status?.clearAfter; if (!clearAfter || clearAfter === '') { return; } if (status) { status.clearAfter = DateUtils.formatUTCToLocal(clearAfter, 'yyyy-MM-dd HH:mm:ss'); } }); }); return response; }); export default formatStatusClearAfter; ```

then we need to add the new middleware to the index file, then we need to register it in the app file:

Request.use(Middleware.FormatClearAfterInPersonalDetails);

this will convert the value returned from the backend to a local date in the middle without the need to update the time in each component that uses it

or we update the time to localdate in the component that use the status. so we need to use the prev function in the StatusClearAfterPage, StatusPage, ReportActionItemSingle, and OptionRowLHN

like this:

    const clearAfter = DateUtils.formatUTCToLocal(lodashGet(currentUserPersonalDetails, 'status.clearAfter', ''), 'yyyy-MM-dd HH:mm:ss');

Alternatively

Converting the date inside DateUtils

This will require quite a lot of change here are the areas where we need to consider converting from UTC to local when showing the data to the user and visa versa when writing the date into the database: will introduce two functions `convertUtcToLocal` and `convertLocalToUtc` Both take a string date and input format and return the date in the same format but in utc or local date .. Here are the places where we need to convert the input string from utc to local time : 1. https://github.com/Expensify/App/blob/5049f8c604c76bb6e87a70763c3363c4d6b243e7/src/libs/DateUtils.ts#L493 4. https://github.com/Expensify/App/blob/5049f8c604c76bb6e87a70763c3363c4d6b243e7/src/libs/DateUtils.ts#L477 5. https://github.com/Expensify/App/blob/5049f8c604c76bb6e87a70763c3363c4d6b243e7/src/libs/DateUtils.ts#L629 6. https://github.com/Expensify/App/blob/5049f8c604c76bb6e87a70763c3363c4d6b243e7/src/libs/DateUtils.ts#L400 7. https://github.com/Expensify/App/blob/a3c4288a20f3c77e157cbe3ab0c0e26a8dbbe8e5/src/libs/ValidationUtils.ts#L392 8. https://github.com/Expensify/App/blob/5049f8c604c76bb6e87a70763c3363c4d6b243e7/src/libs/DateUtils.ts#L529 And here are the places where we need to convert the UTC to localtime to show the date to the user: 1. https://github.com/Expensify/App/blob/03685bb12025626b2afbff50699e3bf866aff998/src/pages/settings/Profile/CustomStatus/SetTimePage.js#L42-L47 4. https://github.com/Expensify/App/blob/03685bb12025626b2afbff50699e3bf866aff998/src/pages/settings/Profile/CustomStatus/SetDatePage.js#L60-L65

stitesExpensify commented 11 months ago

@abzokhattab have you tested that? I don't think that is the correct cause. We don't need to set the status when we are just creating a draft, and when you actually click save it looks like the request is working properly.

When you refresh the page after your timer, the status disappears. That leads me to believe that the issue is that we are not pushing the updates to all users properly.

IMO this is not a blocker even though it is genuinely broken. The Status feature that we merged was massive, and reverting it is not feasible. I will work on a fix in web-e first thing next week

abzokhattab commented 11 months ago

@stitesExpensify , I found out the comment wasn't working, so I hid it. After testing, I think the problem is related to time zones. I tried using a date from 6 hours ago, but the status was not cleared after that time had passed. However, when I checked it just now, the status was updated. It seems there's a mismatch in how the time zone is interpreted. I suspect the server, where the backend is running, is in the USA, causing the entered date to be parsed as a US date. This might be delaying the job trigger by a few hours.

My suggestion is to send the date from the frontend as UTC. This way, the backend can accurately parse and handle it. Additionally, it's worth noting that the backend doesn't have information about the client's timezone. The time string is sent to the backend without specifying the zone. In the provided data sample for updating the status, there is no information about the client's browser timezone:

To address this, two actions are recommended:

The frontend should either send the timezone along with the status time, OR the frontend should parse it as UTC before sending it to the backend OR we can use the user's timezone which is saved under profile/timezone but this will require some changes in parsing the dates because now we just call new Date() in parsing the clearAfterTime value.
The backend should parse the incoming date as UTC.

abzokhattab commented 11 months ago

Just tried to test it again .. this time the status is not cleared 😂 even after almost 11 hours of the custom date .. so i guess the backend doesnt clear it.

If we dont want to tackle the clearing process in the frontend we can prevent the status from being shown up in case the clearTime has passed.

abzokhattab commented 11 months ago

If we have decided to tackle the status clearing process in the frontend, here is a blueprint for how we can achieve that :

inside the User.js file we need to add this function that would return the current status and (update it if the clear date has passed):

function getUserStatus(userPersonalDetails) {
    const statusEmojiCode = lodashGet(userPersonalDetails, 'status.emojiCode', '');
    const statusText = lodashGet(userPersonalDetails, 'status.text', '');
    const statusClearAfter = lodashGet(userPersonalDetails, 'status.clearAfter', '');

    if (statusClearAfter !== '' && new Date(statusClearAfter) < new Date()) {
        if (userPersonalDetails.accountID === currentUserAccountID) {
            clearCustomStatus();
            clearDraftCustomStatus();
        }
        return {
            emojiCode: '',
            text: '',
            clearAfter: '',
        };
    }
    return {
        emojiCode: statusEmojiCode,
        text: statusText,
        clearAfter: statusClearAfter,
    };
}

then we need to replace this:

with:

const statusData = User.getUserStatus(currentUserPersonalDetails);
const currentUserEmojiCode = lodashGet(statusData, "emojiCode", "");
const currentUserStatusText = lodashGet(statusData, "text", "");
const currentUserClearAfter = lodashGet(statusData, "clearAfter", "");

same here:

change it to:

const status = User.getUserStatus(currentUserDetails);
const emojiCode = status.emojiCode;

and here change this:

to:

const { avatar, login, pendingFields, fallbackIcon } = personalDetails[actorAccountID] || {};
const status = User.getUserStatus(personalDetails[actorAccountID]);

and change this:

to:

const emojiStatus = User.getUserStatus(currentUserPersonalDetails).emojiCode;

and finally change this:

to

const statusData = User.getUserStatus(optionItem);
const emojiCode = lodashGet(statusData, "emojiCode", "");
const statusText = lodashGet(statusData, "text", "");
const statusClearAfterDate = lodashGet(statusData, "clearAfter", "");

OR

alternatively we can make the above change on connecting to the App so it will update status if needed as we currently do with the timezone

Result

https://github.com/Expensify/App/assets/59809993/3f491858-e4d9-451d-aef5-82dcd9839054

Pujan92 commented 11 months ago

Seems a backend issue where the status is not cleared after time expiration and the pusher event isn't receieved.

stitesExpensify commented 11 months ago

Seems a backend issue where the status is not cleared after time expiration and the pusher event isn't receieved.

I agree

stitesExpensify commented 11 months ago

I think there are actually 2 issues here. One is that we aren't sending the time to the server in UTC (frontend bug), and the second is that the user who set the status does not get a pusher notification (backend bug).

When the bedrock job gets created on the server, it uses local time so if you are west of UTC it runs instantly and clears your status, and if you are east of UTC it happens late.

Your status also disappears for other users, but not you.

melvin-bot[bot] commented 11 months ago

Job added to Upwork: https://www.upwork.com/jobs/~013df2712d0a3d5327

melvin-bot[bot] commented 11 months ago

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

melvin-bot[bot] commented 11 months ago

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

melvin-bot[bot] commented 11 months ago

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

stitesExpensify commented 11 months ago

@abzokhattab the server always assumes the time is in UTC so the frontend fix here is sending the datetime as UTC from the frontend as you mentioned here https://github.com/Expensify/App/issues/33128#issuecomment-1858637283

This will not fully fix the bug, but it will help

stitesExpensify commented 11 months ago

Can you write a formal proposal for that solution please @abzokhattab ?

dylanexpensify commented 11 months ago

thank you!

melvin-bot[bot] commented 10 months ago

@deetergp, @stitesExpensify, @abdulrahuman5196, @dylanexpensify Eep! 4 days overdue now. Issues have feelings too...

abdulrahuman5196 commented 10 months ago

@abzokhattab Is the proposal ready as requested here? https://github.com/Expensify/App/issues/33128#issuecomment-1863353295

dragnoir commented 10 months ago

@stitesExpensify any idea how the BE handle the "clear after"? Does the BE process an empty emoji after or we just need to hide it on UI? I checked the Onyx values after the time of the clear period and it's not changed. I assume the BE is not doing any action!

melvin-bot[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

@deetergp @stitesExpensify @abdulrahuman5196 @dylanexpensify this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

abzokhattab commented 10 months ago

Looking more into it today

melvin-bot[bot] commented 10 months ago

@deetergp, @stitesExpensify, @abdulrahuman5196, @dylanexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] commented 10 months ago

@deetergp, @stitesExpensify, @abdulrahuman5196, @dylanexpensify 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

abdulrahuman5196 commented 10 months ago

Looking more into it today

@abzokhattab Gentle ping

melvin-bot[bot] commented 10 months ago

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

abzokhattab commented 10 months ago

This requires lots of changes so I added a blueprint for the changes in my proposal

Please have a look at the proposal idea and let me know if we can go with this direction .. if you confirm i can create a draft PR and you can have a look at the code changes

dylanexpensify commented 10 months ago

@abdulrahuman5196 to review ^! cc @deetergp

abdulrahuman5196 commented 10 months ago

Reviewing now

melvin-bot[bot] commented 10 months ago

@deetergp @stitesExpensify @abdulrahuman5196 @dylanexpensify this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

stitesExpensify commented 10 months ago

@abzokhattab why do we need to convert in so many places? For things like getEndOfToday and validateDateTimeIsAtLeastOneMinuteInFuture can't we just use the local time for that, and then convert it right before we send it to the server?

stitesExpensify commented 10 months ago

I checked the Onyx values after the time of the clear period and it's not changed. I assume the BE is not doing any action!

@dragnoir Right, that's the first problem I mentioned here https://github.com/Expensify/App/issues/33128#issuecomment-1863349962

dylanexpensify commented 10 months ago

@abzokhattab to reply

melvin-bot[bot] commented 10 months ago

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

abzokhattab commented 10 months ago

@abzokhattab why do we need to convert in so many places? For things like getEndOfToday and validateDateTimeIsAtLeastOneMinuteInFuture can't we just use the local time for that, and then convert it right before we send it to the server?

I wanted to make the dateutil functions handle the conversion part themselves but this sounds like a lot of work and might cause breaking changes....

for that reason I am now considering the other way where we only change the clearAfter to UTC on update..

I have updated my proposal please have a look

stitesExpensify commented 10 months ago

@abdulrahuman5196 can you take a look at the newest proposal please?

abzokhattab commented 10 months ago

@stitesExpensify updated the proposal to use a middleware instead of parsing the date into a local date on the component level

abdulrahuman5196 commented 10 months ago

On looking @abzokhattab 's proposal here https://github.com/Expensify/App/issues/33128#issuecomment-1858139232 I don't see any strong RCA mentioned for the timezone issue, more it seems like a hypothesis.

Only on @stitesExpensify 's comment here it mentions some clear requirement from backend perspective that the frontend should provide the status date and time in UTC format.

@stitesExpensify Do we evaluate the proposal to just do the fix for One is that we aren't sending the time to the server in UTC (frontend bug) or request to do more Root cause analysis on this issue?

melvin-bot[bot] commented 10 months ago

@deetergp @stitesExpensify @abdulrahuman5196 @dylanexpensify this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

Decide whether any proposals currently meet our guidelines and can be approved as-is today
If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

melvin-bot[bot] commented 10 months ago

Current assignee @abdulrahuman5196 is eligible for the Internal assigner, not assigning anyone new.

abzokhattab commented 10 months ago

I don't see any strong RCA mentioned for the timezone issue, more it seems like a hypothesis.

don't get your comment can elaborate more ... The RCA is that we don't convert the clearAfter to a UTC before it's sent to the backend it's rather sent in the client's local time not UTC.

Brandon mentioned that he will fix the original backend bug which is that the status is not cleared after the time has passed however he requested also that the date sent to the backend has to be in UTC

dylanexpensify commented 10 months ago

@abdulrahuman5196 to confirm above

dylanexpensify commented 10 months ago

Bump @abdulrahuman5196

abdulrahuman5196 commented 10 months ago

Sorry for missing this out.

abdulrahuman5196 commented 10 months ago

I don't see any strong RCA mentioned for the timezone issue, more it seems like a hypothesis.

don't get your comment can elaborate more ... The RCA is that we don't convert the clearAfter to a UTC before it's sent to the backend it's rather sent in the client's local time not UTC.

Brandon mentioned that he will fix the original backend bug which is that the status is not cleared after the time has passed however he requested also that the date sent to the backend has to be in UTC

@abzokhattab I understand your mention of we need to convert the time to UTC before sending it to the backend.

But why I mentioned it doesn't have strong RCA is because even if we did that, we can't verify if the change has fixed this issue or not without fixing the backend issues where the pusher notification is not sent as pointed out here - https://github.com/Expensify/App/issues/33128#issuecomment-1863349962

During my testing, I converted my timezone to UTC and tested, but the issue still persisted(I assume its due to push failure). The same would be happening at your testing as well, I assume that's why you had pointed out with the hypothesis here https://github.com/Expensify/App/issues/33128#issuecomment-1858637283.

So I wanted to confirm with @stitesExpensify on this requirement, since we won't be able to verify the fix if push notification is not sent? Correct me if wrong @stitesExpensify

Do we evaluate the proposal to just do the fix for One is that we aren't sending the time to the server in UTC (frontend bug) as pointed out here.

stitesExpensify commented 10 months ago

This should still be testable without fixing the backend issue. If you log in on one account in a normal tab and one account in an incognito tab, you will see the status expire correctly from the other user's point of view

abdulrahuman5196 commented 9 months ago

Thanks for the info. Will check from other account

Expensify / App