izarutskaya commented 1 year 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!

Action Performed:

Click FAB -> Split Bill
Enter some amount
In email or phone enter "+919999676352"
Select/Unselect the user suggested bewlow

Expected Result:

The user should be shown either +919999676352 or 09999676352

Actual Result:

It keep switching initials on click from +919999676352 to 09999676352 and vice versa

Workaround:

Unknown

Platforms:

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

[ ] Android / native
[ ] Android / Chrome
[ ] iOS / native
[ ] iOS / Safari
[x] MacOS / Chrome / Safari
[ ] MacOS / Desktop

Version Number: v1.3.70-5.

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

https://github.com/Expensify/App/assets/115492554/22dba0eb-f4c4-4845-8c9b-7571ae32d602

https://github.com/Expensify/App/assets/115492554/39411ddb-dbb3-46ee-8184-a25328fe3341

Expensify/Expensify Issue URL:

Issue reported by: @DinalJivani

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1692652454644699

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01c4451db1b16853c4
Upwork Job ID: 1702760550821027840
Last Price Increase: 2023-09-15
Automatic offers:

ZhenjaHorbach | Contributor | 26877939
DinalJivani | Reporter | 26877941

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01c4451db1b16853c4

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

[x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[x] 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
[x] 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.
[x] 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

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @bfitzexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

melvin-bot[bot] commented 1 year ago

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

ZhenjaHorbach commented 1 year ago

Proposal

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

After selecting an element on the cash screen, the appearance of the name changes

What is the root cause of that problem?

The main problem is that when we select, the number formatter is triggered

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

https://github.com/Expensify/App/blob/191bda19b9acd42e83c5f87a870a483c30da7cc5/src/libs/OptionsListUtils.js#L214

In this line we can remove the LocalePhoneNumber.formatPhoneNumber, which will not formatted number

https://github.com/Expensify/App/assets/68128028/9fd3b69f-c24e-471c-b9cb-7455ade9e546

What alternative solutions did you explore? (Optional)

NA

robertKozik commented 1 year ago

Hi @ZhenjaHorbach! I appreciate your proposals; they correctly identify the root cause. I'm wondering if we could retain the formatter instead of disabling it entirely? Disabling it creates a discrepancy between the phone number in the title and the one below it in the subtitle. Ideally, they should match in my opinion.

ZhenjaHorbach commented 1 year ago

@robertKozik Hello) As for me, a formatter for the title is not needed Since it only applies in this case which is described in the bug In other cases, we have titles without formatting

But I think what to do according to you is not a problem We can make textand alternateText them the same in this util https://github.com/Expensify/App/blob/191bda19b9acd42e83c5f87a870a483c30da7cc5/src/libs/OptionsListUtils.js#L214-L223

But if you do not want to delete this formatter, we can add a new parameter to this utility that will control text formatting And set only for one screen

robertKozik commented 1 year ago

Thanks for clarifying and checking that @ZhenjaHorbach. Let's stick to your original proposal and disable the formatter. Selected Proposal: https://github.com/Expensify/App/issues/27557#issuecomment-1721810883 Contributor: @ZhenjaHorbach

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 1 year ago

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

neil-marcellini commented 1 year ago

Hi guys. I'm not understanding the root cause explanation. Specifically, why does the formatting change when the option is selected vs not selected. How should the numbers be formatted? There must be a reason we have the phone number formatter so I don't want to disable it without a clear explanation why.

@ZhenjaHorbach please let me know when you have updated your proposal.

ZhenjaHorbach commented 1 year ago

@neil-marcellini

Hello )

Previously it made sense to disable this since by default we had phones without formatting (since screenshots that I left or videos) And adding formatting looked weird when we found the user and selected him

But now I just checked this screen and noticed that the implementation of split screen has changed a lot

Unlike the videos and my screenshots

And now text and subtext look the same (Formatted)

But there's still a problem Since when searching for a user by phone, phone is not formatted

https://github.com/Expensify/App/assets/68128028/1cdeff4c-5d0f-42a0-afd7-72daa954055f

Proposal

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

After selecting an element on the cash screen, the appearance of the name changes

What is the root cause of that problem?

For userToInvite we don't use formatter

https://github.com/Expensify/App/blob/284c170c55f6c06e3191f7f6cba73513e5b31065/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSelector.js#L143-L150

when in turn the main list elements are formatted

https://github.com/Expensify/App/blob/284c170c55f6c06e3191f7f6cba73513e5b31065/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSelector.js#L112-L117

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

we can update this string and make it the same as for the main list items

                data: _.map([newChatOptions.userToInvite], (participant) => {
                    const isPolicyExpenseChat = lodashGet(participant, 'isPolicyExpenseChat', false);
                    return isPolicyExpenseChat ? OptionsListUtils.getPolicyExpenseReportOption(participant) : OptionsListUtils.getParticipantsOption(participant, personalDetails);
                }),

https://github.com/Expensify/App/blob/284c170c55f6c06e3191f7f6cba73513e5b31065/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSelector.js#L146

https://github.com/Expensify/App/assets/68128028/23a27aea-4da8-4274-a663-6dae114279e9

What alternative solutions did you explore? (Optional)

NA

СС @robertKozik

neil-marcellini commented 1 year ago

I'll review again soon

robertKozik commented 1 year ago

@ZhenjaHorbach I think your root cause is lacking why the number keeps changing (in other words why the formatter is triggered only when user is selected)

ZhenjaHorbach commented 1 year ago

@robertKozik

https://github.com/Expensify/App/issues/27557#issuecomment-1731150813

Comment was updated )

neil-marcellini commented 1 year ago

Ok I'm good with @ZhenjaHorbach's proposal. In the future please only update your original proposal.

melvin-bot[bot] commented 1 year ago

📣 @ZhenjaHorbach 🎉 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[bot] commented 1 year ago

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

Offer link Upwork job

ZhenjaHorbach commented 1 year ago

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

PR will be ready today

DinalJivani commented 1 year ago

@johncschuster I had received Upwork offer of $50 This is logged after 30th Aug. So it should be $250.

johncschuster commented 1 year ago

Sorry about that, @DinalJivani! You're right! Let me adjust that.

melvin-bot[bot] commented 1 year ago

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

when @ZhenjaHorbach got assigned: 2023-09-26 06:58:40 Z
when the PR got merged: 2023-09-29 07:44:30 UTC
days elapsed: 3

On to the next one 🚀

melvin-bot[bot] commented 12 months ago

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

melvin-bot[bot] commented 12 months ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.3.76-6 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

https://github.com/Expensify/App/pull/28274

If no regressions arise, payment will be issued on 2023-10-10. :confetti_ball:

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

[x] External issue reporter
[x] Contributor that fixed the issue
[ ] Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

@robertKozik does not require payment (Contractor)
@ZhenjaHorbach requires payment offer (Contributor)
@DinalJivani requires payment offer (Reporter)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

Merged PR within 3 business days of assignment - 50% bonus
Merged PR more than 9 business days after assignment - 50% penalty

melvin-bot[bot] commented 12 months ago

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:

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

neil-marcellini commented 11 months ago

Gentle bump @johncschuster @robertKozik

johncschuster commented 11 months ago

Payment has been issued! We're just waiting on the BZ Checklist to be completed.

robertKozik commented 11 months ago

[X] [@robertKozik] The PR that introduced the bug has been identified. Link to the PR: I was unable to pinpoint exact PR, propably it was missed since implementation

[X] [@robertKozik] 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

[X] [@robertKozik] 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

[X] [@robertKozik] Determine if we should create a regression test for this bug. I don't think it's suitable case for creating the regression test - this bug don't interfere with any of user flows

[X] [@robertKozik] 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

melvin-bot[bot] commented 11 months ago

@johncschuster, @neil-marcellini, @robertKozik, @ZhenjaHorbach Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

neil-marcellini commented 11 months ago

I'm bumping @johncschuster

johncschuster commented 11 months ago

The BZ Checklist is done, which is the only thing we were waiting on. Closing!

Expensify / App

[HOLD for payment 2023-10-10] [$500] Split Bill - Number keeps on changing its initials #27557

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Bug0 Triage Checklist (Main S/O)

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)