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 BugZero Checklist] [$1000] current currency is not highlighted and not marked with ✔️ #17372

Closed
6 tasks done
kavimuru opened this issue Apr 13, 2023 · 52 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 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@kavimuru
Copy link

kavimuru commented Apr 13, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. click on + button
  2. select send money option
  3. click on currency

Expected Result:

current currency should be highlighted and marked with ✔️ (same as Timezone, Pronouns)

Actual Result:

current currency not highlighted

Workaround:

unknown

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: v1.2.99-4
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
Notes/Photos/Videos: Any additional supporting documentation

Recording.218.mp4
Screen.Recording.2023-04-12.at.4.57.17.PM.mov

Expensify/Expensify Issue URL:
Issue reported by: @gadhiyamanan
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1681298924546589

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~015d6db25e1b8518af
  • Upwork Job ID: 1649369664727408640
  • Last Price Increase: 2023-04-21
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Apr 13, 2023
@MelvinBot
Copy link

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

@MelvinBot
Copy link

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

@NicMendonca
Copy link
Contributor

Hmm, I think this is a stylistic preference, not a bug. I think it's is very clear which is the current currency:

image

@PrashantMangukiya
Copy link
Contributor

@NicMendonca please click the currency marked with yellow square in your image, it will open Select currency screen. On that screen CA is not highlighted with bold and marked with green check mark.

So in this issue we are talking about it. You can go to Timezone, Pronouns page to see the exact behaviour what should be expected result for this issue.

Here is quick video I recorded to make it more clear. So this issue should not be closed.

Currency.mov

@Prince-Mendiratta
Copy link
Contributor

@NicMendonca I too think this issue is about the list itself, not the label

@Prince-Mendiratta
Copy link
Contributor

Proposal

Posting proposal early as per new guidelines for the eventual review

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

In this issue, we can notice that on the Select a currency page, the Options selector does not have the selected / default currency marked with a tick, as in other pages like Timezone, Language, Pronouns.

What is the root cause of that problem?

Unlike other pages, we do not have a customIcon and the boldStyle set for the options that are displayed on the OptionsSelector on the IOUCurrencySelection page. This results in the default currency not being checkmarked and neither being bold.

In order for an item to be checkmarked, when we are creating the objects for the options that will be displayed on the OptionsList via the OptionsSelector, the object must have the customIcon and boldStyle key, which is missing on the OptionsSelector.

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

To solve this issue, we need to account for the selected currency that is chosen by the user by default and set the custom icon to a green checkmark and apply a bold style to that currency. In order to do that, we will have to perform the following additions:

  1. To get the default currency that is chosen by the user, we need the Onyx key ONYXKEYS.IOU. We will import that by adding the key for that in the withOnyx call for IOUCurrencySelection.
  2. Next, we will declare the respective propTypes and defaultProps for iou prop.
  3. Next, we need to import the green checkmark. This will be done by using the following code and adding the relevant dependencies:
    const greenCheckmark = {src: Expensicons.Checkmark, color: themeColors.success};
  4. Once this is done, we need to modify the getCurrencyOptions method on the IOUCurrencySelection page. We need to include the customIcon as a key in the object where if the iterated currency code equals the default currency mode, we will set the icon as the green checkmark else as undefined.
  5. We also need to include the boldStyle key in the object such that if the iterated currency code equals the default currency mode, we will set the value of boldStyle to true.
    return _.map(this.props.currencyList, (currencyInfo, currencyCode) => ({
    text: `${currencyCode} - ${CurrencySymbolUtils.getLocalizedCurrencySymbol(this.props.preferredLocale, currencyCode)}`,
    currencyCode,
    keyForList: currencyCode,
    }));
  6. Once this has been done, the green checkmark and bold style will be applied to the selected option. However, in order to improve the experience and be consistent with other pages, we also need to scroll the OptionsList to the default selected currency when the page is initially opened. To do this, we need to add a prop initiallyFocusedOptionKey to the OptionsSelector where it's value will be this.props.iou.selectedCurrencyCode.
    <OptionsSelector

What alternative solutions did you explore? (Optional)

None

@PrashantMangukiya
Copy link
Contributor

Proposal

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

Current currency is not highlighted and not marked with checkmark.

What is the root cause of that problem?

Within IOUCurrencySelection.js we are creating currency options via getCurrencyOptions() function

getCurrencyOptions() {
return _.map(this.props.currencyList, (currencyInfo, currencyCode) => ({
text: `${currencyCode} - ${CurrencySymbolUtils.getLocalizedCurrencySymbol(this.props.preferredLocale, currencyCode)}`,
currencyCode,
keyForList: currencyCode,
}));
}

We can see that here there is not any condition to highlight the selected currency. This is the root cause of the problem.

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

We have to update getCurrencyOptions() function, so it will set the customIcon and boldStyle for selected currency as shown below:

// Add this const  
const greenCheckmark = {src: Expensicons.Checkmark, color: themeColors.success};

getCurrencyOptions() {
    return _.map(this.props.currencyList, (currencyInfo, currencyCode) => {
        const isCurrentCurrency = currencyCode === this.props.iou.selectedCurrencyCode;
        return {
            text: `${currencyCode} - ${CurrencySymbolUtils.getLocalizedCurrencySymbol(this.props.preferredLocale, currencyCode)}`,
            currencyCode,
            keyForList: currencyCode,
            customIcon: isCurrentCurrency ? greenCheckmark : undefined,
            boldStyle: isCurrentCurrency,
        };
    });
}

We have to import iou as props using withOnyx as shown in pseudo code below, we also need to define iou prop type.

export default compose(
    ....
    withOnyx({
       ...
        iou: {key: ONYXKEYS.IOU}, // ADD THIS LINE
    }),
    ....
)(IOUCurrencySelection);

It will solve the issue and works as expected as shown in results video.

What alternative solutions did you explore? (Optional)

None

Results
Web.mov

@gadhiyamanan
Copy link
Contributor

bump @NicMendonca

@NicMendonca
Copy link
Contributor

Hmmm, I still think this is a stylistic preference and not a bug. Timezone and Pronouns are setting you set in your profile, and the currency you use for sending/ requesting money is ad-hoc but seems to be some passion about it here 😊

@MelvinBot
Copy link

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

@NicMendonca NicMendonca added the External Added to denote the issue can be worked on by a contributor label Apr 21, 2023
@melvin-bot melvin-bot bot changed the title current currency is not highlighted and not marked with ✔️ [$1000] current currency is not highlighted and not marked with ✔️ Apr 21, 2023
@MelvinBot
Copy link

Job added to Upwork: https://www.upwork.com/jobs/~015d6db25e1b8518af

@MelvinBot
Copy link

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

@MelvinBot
Copy link

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 21, 2023
@MelvinBot
Copy link

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

@dukenv0307
Copy link
Contributor

dukenv0307 commented Apr 21, 2023

Proposal

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

Curret currency is not highlighted and not marked with checkmark icon

What is the root cause of that problem?

To highlight and mark option that is selected, each option need to has customIcon, boldStyle property. That's why in IOUCurrencySelection current currency is not highlighted and not marked with checkmark icon

getTimezoneOption(text) {
return {
text,
keyForList: this.getKey(text),
// Include the green checkmark icon to indicate the currently selected value
customIcon: text === this.timezone.selected ? greenCheckmark : undefined,
// This property will make the currently selected value have bold text
boldStyle: text === this.timezone.selected,
};
}

getCurrencyOptions() {
return _.map(this.props.currencyList, (currencyInfo, currencyCode) => ({
text: `${currencyCode} - ${CurrencySymbolUtils.getLocalizedCurrencySymbol(this.props.preferredLocale, currencyCode)}`,
currencyCode,
keyForList: currencyCode,
}));
}

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

  1. Subscribe to the Onyx key IOU.
  2. If the current option is the selected currency => add a greenCheckmark as the customIcon, and a bold style to the option.

What alternative solutions did you explore? (Optional)

NA

Result

Screencast.from.14-04-2023.16.53.18.webm

@melvin-bot melvin-bot bot added the Overdue label Apr 24, 2023
@MelvinBot
Copy link

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

@NicMendonca
Copy link
Contributor

@rushatgabhane any feedback on this proposal?

@melvin-bot melvin-bot bot removed the Overdue label Apr 25, 2023
@NicMendonca
Copy link
Contributor

Don't forget to apply to the job: https://www.upwork.com/jobs/~015d6db25e1b8518af

@rushatgabhane @gadhiyamanan @dukenv0307

@gadhiyamanan
Copy link
Contributor

@NicMendonca applied, Thanks!

@rushatgabhane
Copy link
Member

rushatgabhane commented May 16, 2023

applied with account "Satish Gabhane" - https://www.upwork.com/freelancers/~01212e8255b02ae924

all context: https://expensify.slack.com/archives/C02NK2DQWUX/p1683614947522329

@NicMendonca
Copy link
Contributor

@mananjadhav paid!

@gadhiyamanan
Copy link
Contributor

gadhiyamanan commented May 18, 2023

@NicMendonca it's @gadhiyamanan not mananjadhav 😅
can you please close the Upwork contact

@NicMendonca
Copy link
Contributor

@mananjadhav you did not get paid lol

@gadhiyamanan I'm sorry!!

@rushatgabhane
Copy link
Member

accepted offer

@NicMendonca
Copy link
Contributor

@rushatgabhane thanks! Paid!

@dukenv0307 just need you to accept the offer - thanks!

@dukenv0307
Copy link
Contributor

@NicMendonca accepted, thank you!

@NicMendonca
Copy link
Contributor

Everyone has been paid!

bump @rushatgabhane on this checklist

@NicMendonca NicMendonca changed the title [HOLD for payment 2023-05-16] [$1000] current currency is not highlighted and not marked with ✔️ [HOLD for BugZero Checklist] [$1000] current currency is not highlighted and not marked with ✔️ May 19, 2023
@melvin-bot melvin-bot bot added the Overdue label May 22, 2023
@melvin-bot
Copy link

melvin-bot bot commented May 22, 2023

@iwiznia, @NicMendonca, @rushatgabhane, @dukenv0307 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@NicMendonca
Copy link
Contributor

Going OOO until June 5th so assigning a buddy to this GH to watch over the checklist.

Bump @rushatgabhane on the checklist! #17372 (comment)

@melvin-bot melvin-bot bot removed the Overdue label May 23, 2023
@NicMendonca NicMendonca removed the Bug Something is broken. Auto assigns a BugZero manager. label May 23, 2023
@NicMendonca NicMendonca removed their assignment May 23, 2023
@NicMendonca NicMendonca added the Bug Something is broken. Auto assigns a BugZero manager. label May 23, 2023
@melvin-bot
Copy link

melvin-bot bot commented May 23, 2023

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

@melvin-bot

This comment was marked as duplicate.

@melvin-bot melvin-bot bot added the Overdue label May 25, 2023
@sonialiap
Copy link
Contributor

@rushatgabhane checklist please :) #17372 (comment)

@melvin-bot melvin-bot bot removed the Overdue label May 25, 2023
@rushatgabhane
Copy link
Member

rushatgabhane commented May 25, 2023

  1. The PR that introduced the bug has been identified. Link to the PR: This is a consistency bug. We added a checkmark to timezone selection in this PR. But it's out of scope of PR to go and update all pages to add a checkmark.

  2. 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: https://github.com/Expensify/App/pull/12201/files#r1206076584

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

  4. Determine if we should create a regression test for this bug. No

  5. 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 N.A.

@rushatgabhane
Copy link
Member

Hi @sonialiap, I completed the checklist. We can now close this issue.

@melvin-bot melvin-bot bot added the Overdue label May 29, 2023
@sonialiap
Copy link
Contributor

Thanks @rushatgabhane !

@melvin-bot melvin-bot bot removed the Overdue label May 29, 2023
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 Engineering External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

10 participants