Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.32k stars 2.75k forks source link

[$250] Search filters - (you) disappears after selecting own user name #47712

Open lanitochka17 opened 3 weeks ago

lanitochka17 commented 3 weeks 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: 9.0.22-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: N/A Email or phone of affected tester (no customers): applausetester+kh05081@applause.expensifail.com Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to staging.new.expensify.com/search/filters
  2. Click From or To
  3. Select your user name

Expected Result:

(you) will not disappear after selecting own user name

Actual Result:

(you) disappears after selecting own user name

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/0d3a4fab-3206-40fa-937f-3c09d3fc5858

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01bc33a0aabc7f4ab9
  • Upwork Job ID: 1826026505817504872
  • Last Price Increase: 2024-08-27
  • Automatic offers:
    • FitseTLT | Contributor | 103781419
Issue OwnerCurrent Issue Owner: @FitseTLT
melvin-bot[bot] commented 3 weeks ago

Triggered auto assignment to @twisterdotcom (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 commented 3 weeks ago

@twisterdotcom 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 commented 3 weeks ago

We think that this bug might be related to #wave-collect - Release 1

etCoderDysto commented 3 weeks ago

Edited by proposal-police: This proposal was edited at 2024-08-20 13:39:32 UTC.

Proposal

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

(you) disappears after selecting own user name

What is the root cause of that problem?

When the user selects their account, In getParticipantsOption, we assign assign displayName which is the users name without (you) to text https://github.com/Expensify/App/blob/d519981587b065ce96f73ab341d84f9eb17a864c/src/libs/OptionsListUtils.ts#L447-L457 And the text prop will be used to render the users display name in InviteMemberListItem component

getParticipantsOption is called here by formatSectionsFromSearchTerm https://github.com/Expensify/App/blob/d519981587b065ce96f73ab341d84f9eb17a864c/src/libs/OptionsListUtils.ts#L2378

And formatSectionsFromSearchTerm is called here to fetch section https://github.com/Expensify/App/blob/d519981587b065ce96f73ab341d84f9eb17a864c/src/components/Search/SearchFiltersParticipantsSelector.tsx#L98-L99

Then the section will be used by SectionList

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

Since participant.text has the display name that includes (you) we should assign participant.text to text prop and fallback to displayName if participant.text is undefiend

text: participant?.text ?? displayName,

What alternative solutions did you explore? (Optional)

FitseTLT commented 3 weeks ago

Edited by proposal-police: This proposal was edited at 2024-08-20 14:33:24 UTC.

Proposal

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

Search filters - (you) disappears after selecting own user name

What is the root cause of that problem?

We are properly setting (you) to current user here https://github.com/Expensify/App/blob/a32da40e1017d2136389f05735bace7bc307e098/src/components/Search/SearchFiltersParticipantsSelector.tsx#L111-L113 but only doing it when current user is not selected

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

We should do the same update of text we did here for selected current user too So change this to

   const currentUserSelected = formattedResults.section.data.find((option) => option.accountID === chatOptions.currentUserOption?.accountID);
        newSections.push(formattedResults.section);

        if (chatOptions.currentUserOption) {
            const formattedName = ReportUtils.getDisplayNameForParticipant(chatOptions.currentUserOption.accountID, false, true, true, personalDetails);
            if (!currentUserSelected) {
                newSections.push({
                    title: '',
                    data: [{...chatOptions.currentUserOption, text: formattedName}],
                    shouldShow: true,
                });
            } else {
                currentUserSelected.text = formattedName;
            }
        }

What alternative solutions did you explore? (Optional)

bernhardoj commented 3 weeks ago

Proposal

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

The (you) text disappears from the current user option after selecting it.

What is the root cause of that problem?

When the current user is not selected yet, we format the name using getDisplayNameForParticipant and pass shouldAddCurrentUserPostfix as true which adds the (you) suffix. https://github.com/Expensify/App/blob/c6d81c82fc88d52622c41a958844b2fcc9b1bb33/src/components/Search/SearchFiltersParticipantsSelector.tsx#L111-L115

When the user is selected, the selected options is formatted with OptionsListUtils.formatSectionsFromSearchTerm > getParticipantsOption. https://github.com/Expensify/App/blob/c6d81c82fc88d52622c41a958844b2fcc9b1bb33/src/components/Search/SearchFiltersParticipantsSelector.tsx#L98-L100 https://github.com/Expensify/App/blob/c6d81c82fc88d52622c41a958844b2fcc9b1bb33/src/libs/OptionsListUtils.ts#L2378

In getParticipantsOption, the text is from PersonalDetailsUtils.getDisplayNameOrDefault whcih doesn't contains the (you) suffix because we don't pass shouldAddCurrentUserPostfix to it. https://github.com/Expensify/App/blob/c6d81c82fc88d52622c41a958844b2fcc9b1bb33/src/libs/OptionsListUtils.ts#L451

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

Pass as true for shouldAddCurrentUserPostfix to getDisplayNameOrDefault.

const displayName = PersonalDetailsUtils.getDisplayNameOrDefault(detail, LocalePhoneNumber.formatPhoneNumber(login), undefined, shouldAddCurrentUserPostfix);

This will require us to pass down the value as true from SearchFiltersParticipantsSelector > formatSectionsFromSearchTerm > getParticipantsOption.

melvin-bot[bot] commented 3 weeks ago

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

melvin-bot[bot] commented 3 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

@twisterdotcom, @abdulrahuman5196 Eep! 4 days overdue now. Issues have feelings too...

abdulrahuman5196 commented 2 weeks ago

Hi, came back from weekend. Will check in the morning.

melvin-bot[bot] commented 2 weeks ago

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

abdulrahuman5196 commented 1 week ago

@FitseTLT 's proposal here https://github.com/Expensify/App/issues/47712#issuecomment-2299001034 looks good and works well. It aims to fix the issue in the SearchFiltersParticipantsSelector similar to existing approach of showing (you).

πŸŽ€ πŸ‘€ πŸŽ€ C+ Reviewed

melvin-bot[bot] commented 1 week ago

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

bernhardoj commented 1 week ago

@abdulrahuman5196 any opinion with my proposal?

I think the selected proposal doesn't follow a similar approach. The approach is mutating the item returned from the array below. https://github.com/Expensify/App/blob/a32da40e1017d2136389f05735bace7bc307e098/src/components/Search/SearchFiltersParticipantsSelector.tsx#L107

The current approach is, selected options are formatted by OptionsListUtils.formatSectionsFromSearchTerm and then pushed to the list. If the current user isn't selected, we push the current user with (you) suffix to the list. If the user is selected, then it will be handled by OptionsListUtils.formatSectionsFromSearchTerm, just like the other selected items.

Also, the selected proposal will cause the current user to always be at the top even though there are selected items.

image
FitseTLT commented 1 week ago

Also, the selected proposal will cause the current user to always be at the top even though there are selected items.

@abdulrahuman5196 It's because I moved down the newSections.push(formattedResults.section); below the if block I have updated and moved it back. It works fine now. We alrdeay have a code that concatenates (you) https://github.com/Expensify/App/blob/8c69c58dfa7f6a9ab079e560ff026c1aed12be8e/src/components/Search/SearchFiltersParticipantsSelector.tsx#L115 the only problem we forgot to do the same for selected list so my approach is indeed using the same as existing approach.

https://github.com/user-attachments/assets/9db1d9d7-e907-4aad-a96b-d03bb69bec33

abdulrahuman5196 commented 1 week ago

@bernhardoj

I think the selected proposal doesn't follow a similar approach. The approach is mutating the item returned from the array below.

I noticed that during approval, but its more like a PR code fix. for the issue you mentioned, we can fix that during PR since we don't consider the proposal code to be the expect PR code.

any opinion with my proposal? Pass as true for shouldAddCurrentUserPostfix to getDisplayNameOrDefault

I was not aligned in passing shouldAddCurrentUserPostfix as true in the common helper method usecase. Since the default value of shouldAddCurrentUserPostfix in getDisplayNameOrDefault is false and I have also noticed we are not passing it as true in multiple common places.

bernhardoj commented 1 week ago

I was not aligned in passing shouldAddCurrentUserPostfix as true in the common helper method usecase. Since the default value of shouldAddCurrentUserPostfix in getDisplayNameOrDefault is false and I have also noticed we are not passing it as true in multiple common places.

I don't see why this is a problem since we only pass it as true in the filter page just like we did with the non-selected current user section.

I think the selected proposal doesn't follow a similar approach. The approach is mutating the item returned from the array below.

I noticed that during approval, but its more like a PR code fix.

I believe the root issue is that we are not formatting it inside OptionsListUtils.formatSectionsFromSearchTerm (assuming you agree with this), but the selected proposal explanation is revolving around mutating the array and lacks the explanation that we already did some formatting inside OptionsListUtils.formatSectionsFromSearchTerm which is why I posted my proposal because I think the approach is very different.

^ just an explaination why I posted my proposal πŸ˜„, i'll accept any decision

melvin-bot[bot] commented 1 week ago

πŸ“£ @FitseTLT πŸŽ‰ 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 πŸ“–

twisterdotcom commented 1 week ago

We have an assignee @MelvinBot

melvin-bot[bot] commented 1 week ago

@twisterdotcom @Gonals @FitseTLT @abdulrahuman5196 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!

FitseTLT commented 1 week ago

PR will be up tomorrow