Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[$250] Categorizing - Date picker on confirmation page does not show the selected date #46763

Closed
6 tasks done
lanitochka17 opened this issue Aug 3, 2024 · 23 comments
Closed
6 tasks done
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@lanitochka17
Copy link

lanitochka17 commented Aug 3, 2024

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: 9.0.16-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: https://expensify.testrail.io/index.php?/tests/view/4807531
Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to self DM
  3. Track a manual expense
  4. Go to transaction thread
  5. Click Date and change to another date
  6. Return to main chat
  7. Click Categorize it > Select any category
  8. Click Date on confirmation page
  9. Change to a different date and save it
  10. Click Date again

Expected Result:

The date picker will display the latest selected date in Step 9

Actual Result:

The date picker displays the selected date in Step 5, which is the date edited after tracking the expense and before categorizing
When saving the date without changing it, the latest date in Step 9 remains selected instead of the date shown in Step 10

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

Bug6560402_1722620558526.20240803_013609.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~013cac8c72a835b3bf
  • Upwork Job ID: 1820982675228792931
  • Last Price Increase: 2024-08-07
  • Automatic offers:
    • eh2077 | Reviewer | 103427314
    • abzokhattab | Contributor | 103427316
Issue OwnerCurrent Issue Owner: @eh2077
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 3, 2024
Copy link

melvin-bot bot commented Aug 3, 2024

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

@lanitochka17
Copy link
Author

@mallenexpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

@lanitochka17
Copy link
Author

We think that this bug might be related to #vip-vsp

@hayes102
Copy link

hayes102 commented Aug 3, 2024

I can not reproduce the issue

Copy link

melvin-bot bot commented Aug 3, 2024

📣 @hayes102! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@abzokhattab
Copy link
Contributor

abzokhattab commented Aug 3, 2024

Edited by proposal-police: This proposal was edited at 2024-08-03 16:48:03 UTC.

Proposal

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

The date picker on the confirmation page does not display the last selected date.

What is the root cause of that problem?

  1. When a request's date is updated, the modifiedCreated value is set to a specific date.

  2. Later, when the date is changed on the confirmation page during expense categorization, the created property of the transaction is modified.

  3. When retrieving the created date here , if modifiedCreated is present, the code returns the modifiedCreated date instead of the created date.

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

When creating the draft transaction while categorizing the request, we should set modifiedCreated to undefined, similar to how we clear the modifiedMerchant:

App/src/libs/ReportUtils.ts

Lines 7206 to 7218 in 6a22933

IOU.createDraftTransaction({
...transaction,
actionableWhisperReportActionID: reportActionID,
linkedTrackedExpenseReportAction,
linkedTrackedExpenseReportID: reportID,
created,
amount,
currency,
comment,
merchant,
modifiedMerchant: '',
mccGroup,
} as Transaction);

Alternatively

We should update the logic so that when setting the created date, it also checks and updates the modifiedCreated property if it exists.

function setMoneyRequestCreated(transactionID: string, created: string, isDraft: boolean, modifiedCreated?: string) {
    Onyx.merge(
        `${isDraft ? ONYXKEYS.COLLECTION.TRANSACTION_DRAFT : ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`,
        modifiedCreated
            ? {
                  modifiedCreated: created,
                  created
              }
            : {created},
    );
}

Finally, modify the setMoneyRequestCreated call here .

OR

we can just set the modifiedCreated as undefined:

function setMoneyRequestCreated(transactionID: string, created: string, isDraft: boolean) {
    Onyx.merge(`${isDraft ? ONYXKEYS.COLLECTION.TRANSACTION_DRAFT : ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`, {
        created,
        modifiedCreated: null,
    });
}

POC

Screen.Recording.2024-08-03.at.6.45.05.PM.mov

@melvin-bot melvin-bot bot added the Overdue label Aug 5, 2024
Copy link

melvin-bot bot commented Aug 6, 2024

@mallenexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@mallenexpensify mallenexpensify added the External Added to denote the issue can be worked on by a contributor label Aug 7, 2024
@melvin-bot melvin-bot bot changed the title Categorizing - Date picker on confirmation page does not show the selected date [$250] Categorizing - Date picker on confirmation page does not show the selected date Aug 7, 2024
Copy link

melvin-bot bot commented Aug 7, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 7, 2024
Copy link

melvin-bot bot commented Aug 7, 2024

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

@mallenexpensify
Copy link
Contributor

@eh2077 plz review @abzokhattab 's proposal above

Was able to reproduce, I initially added to #wave-collect cuz it was an expense but, since it's via a self-DM. I wasn't able to reproduce when submitting an expense on a collect plan. @abzokhattab and @eh2077 , let me know if either of you are.

@eh2077
Copy link
Contributor

eh2077 commented Aug 7, 2024

I wasn't able to reproduce when submitting an expense on a collect plan. @abzokhattab and @eh2077 , let me know if either of you are.

@mallenexpensify Submitting an expense on a collect plan also works as expected for me. I think this edge case only happens via self-DM where we can categorise an expense.

@eh2077
Copy link
Contributor

eh2077 commented Aug 7, 2024

@abzokhattab 's proposal looks good to me. We can discuss coding details in PR.

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Aug 7, 2024

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

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 7, 2024
Copy link

melvin-bot bot commented Aug 7, 2024

📣 @eh2077 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

Copy link

melvin-bot bot commented Aug 7, 2024

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

@eh2077
Copy link
Contributor

eh2077 commented Aug 9, 2024

@abzokhattab Please let us know when we can expect a PR to be ready for review, thanks

@abzokhattab
Copy link
Contributor

PR is ready

@mallenexpensify mallenexpensify added Daily KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Reviewing Has a PR in review Weekly KSv2 labels Aug 31, 2024
@mallenexpensify
Copy link
Contributor

Contributor: @abzokhattab paid $250 via Upwork
Contributor+: @eh2077 paid $250 via Upwork

Hit production a bit ago

@eh2077 plz complete the BZ checklist.

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:

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

Copy link

melvin-bot bot commented Aug 31, 2024

@mallenexpensify @srikarparsi @abzokhattab @eh2077 this issue is now 4 weeks old, please consider:

  • Finding a contributor to fix the bug
  • Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
  • If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

Copy link

melvin-bot bot commented Sep 3, 2024

@mallenexpensify, @srikarparsi, @abzokhattab, @eh2077 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot melvin-bot bot added the Overdue label Sep 3, 2024
@eh2077
Copy link
Contributor

eh2077 commented Sep 4, 2024

Checklist

  • [@eh2077] The PR that introduced the bug has been identified. Link to the PR: I think this is a missing case when self DM track expense feature was introduced. But I can't figure out the exact PR that is directly responsible for this issue.
  • [@eh2077] 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: N/A
  • [@eh2077] 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: N/A
  • [@eh2077] Determine if we should create a regression test for this bug. Yes

Regression test

  1. Go to staging.new.expensify.com
  2. Go to self DM
  3. Track a manual expense
  4. Go to transaction thread
  5. Click Date and change to another date
  6. Return to main chat
  7. Click Categorize it > Select any category
  8. Click Date on confirmation page
  9. Change to a different date and save it
  10. Click Date again
  11. Ensure that the new date is shown and not the date from step 5.

@melvin-bot melvin-bot bot removed the Overdue label Sep 4, 2024
@srikarparsi
Copy link
Contributor

Nice, if this is paid is this good to close out @mallenexpensify?

@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Daily KSv2 labels Sep 6, 2024
@mallenexpensify
Copy link
Contributor

Test case created, good to close now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

6 participants