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

[Hold for payment 2024-08-27][$250] Missing checkmark when reselecting date of month in Submission frequency modal #47514

Closed
1 of 6 tasks
m-natarajan opened this issue Aug 15, 2024 · 18 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@m-natarajan
Copy link

m-natarajan commented Aug 15, 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: v9.0.20-6
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: @dukenv0307
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1723741251007689

Action Performed:

  1. Open app
  2. Open a workspace with enable workflow
  3. Turn on Delay submissions and open Submission frequency
  4. Choose monthly
  5. Choose Date of month
  6. Select any date
  7. Choose Date of month again

Expected Result:

The selected date is still focused and has a checkmark.

Actual Result:

The selected date is missing a checkmark on the right.

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

Screen.Recording.2024-08-16.at.00.01.29.mov
Recording.447.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0101f6414948e6716e
  • Upwork Job ID: 1824158740168423243
  • Last Price Increase: 2024-08-15
  • Automatic offers:
    • dukenv0307 | Reviewer | 103555137
    • Krishna2323 | Contributor | 103555139
Issue OwnerCurrent Issue Owner: @
@m-natarajan m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 15, 2024
Copy link

melvin-bot bot commented Aug 15, 2024

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

@daledah
Copy link
Contributor

daledah commented Aug 15, 2024

Proposal

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

The selected date is missing a checkmark on the right.

What is the root cause of that problem?

Currently, we are saving autoReportingOffset as string in Onyx and we are calculator isSelected by comparing day which is a number with offsetwhich is a string here

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

We should convert offset to number here

            text: toLocaleOrdinal(day),
            keyForList: day.toString(), // we have to cast it as string for <ListItem> to work
+           isSelected: day === Number(offset),
            isNumber: true,

What alternative solutions did you explore? (Optional)

@dukenv0307
Copy link
Contributor

@lschurr I'm reporter so I can take this bug as C+

@Krishna2323
Copy link
Contributor

Krishna2323 commented Aug 15, 2024

Edited by proposal-police: This proposal was edited at 2024-08-15 18:53:25 UTC.

Proposal

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

Missing checkmark when reselecting date of month in Submission frequency modal

What is the root cause of that problem?

We are using unnecessarily using toString() when passing the autoReportingOffset value to the backend.

const value = JSON.stringify({autoReportingOffset: autoReportingOffset.toString()});

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

We should remove the the toString() method. I have checked the backend response and it does support both number and string. Also OpenPolicyWorkflowsPage api call also returns autoReportingOffset as number.

We should also update value type in SetWorkspaceAutoReportingMonthlyOffsetParams.

type SetWorkspaceAutoReportingMonthlyOffsetParams = {
policyID: string;
value: string;
};

What alternative solutions did you explore? (Optional)

We should also pass shouldUpdateFocusedIndex to SelectionList because the focus index in not changed when selecting monthly option and the RHP is not closed. We have an open issue for this, so I believe its better to handle both bugs separately.

@Krishna2323
Copy link
Contributor

Proposal Updated

  • Added alternative to fix another issue

@lschurr lschurr added the External Added to denote the issue can be worked on by a contributor label Aug 15, 2024
@melvin-bot melvin-bot bot changed the title Missing checkmark when reselecting date of month in Submission frequency modal [$250] Missing checkmark when reselecting date of month in Submission frequency modal Aug 15, 2024
Copy link

melvin-bot bot commented Aug 15, 2024

Job added to Upwork: https://www.upwork.com/jobs/~0101f6414948e6716e

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

melvin-bot bot commented Aug 15, 2024

Current assignee @dukenv0307 is eligible for the External assigner, not assigning anyone new.

@wildan-m
Copy link
Contributor

Proposal

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

Checkmark missing when reselecting date in Submission Frequency modal.

What is the root cause of that problem?

This comparison has might have different type

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

If we check the AutoReportingOffset type it can contain number or string

type AutoReportingOffset = number | ValueOf<typeof CONST.POLICY.AUTO_REPORTING_OFFSET>;

We can use == instead of === to compare.

Using == instead of === will perform type coercion, which means it will convert the operands to the same type before making the comparison.

What alternative solutions did you explore? (Optional)

convert both to string for isSelected check.

src/pages/workspace/workflows/WorkspaceAutoReportingMonthlyOffsetPage.tsx

....
     const dayString = day.toString();
     const offsetString = offset.toString();

        return {
            text: toLocaleOrdinal(day),
            keyForList: dayString, // we have to cast it as string for <ListItem> to work
            isSelected: dayString === offsetString,
            isNumber: true,
        };

@dukenv0307
Copy link
Contributor

@Krishna2323 If we remove toString method, then BE will returns the correct value (number)?

@Krishna2323
Copy link
Contributor

@dukenv0307 yes.

@dukenv0307
Copy link
Contributor

Thank you @Krishna2323. It looks good and tests well on my side.

Let's go with @Krishna2323's proposal

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Aug 16, 2024

Triggered auto assignment to @carlosmiceli, 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 16, 2024
Copy link

melvin-bot bot commented Aug 16, 2024

📣 @dukenv0307 🎉 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 16, 2024

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

@Krishna2323
Copy link
Contributor

@dukenv0307, PR ready for review ^

@Krishna2323
Copy link
Contributor

@lschurr, this was deployed to production 2 weeks ago. Please process the payments when you have a chance. Thanks

@lschurr
Copy link
Contributor

lschurr commented Sep 3, 2024

Thanks for the bump @Krishna2323 - Looks like the automation failed so this should have been paid on August 27th.

@lschurr lschurr changed the title [$250] Missing checkmark when reselecting date of month in Submission frequency modal [Hold for payment 2024-08-27][$250] Missing checkmark when reselecting date of month in Submission frequency modal Sep 3, 2024
@lschurr
Copy link
Contributor

lschurr commented Sep 3, 2024

Payment summary:

@lschurr lschurr closed this as completed Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

7 participants