search

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.56k stars 2.9k forks source link

[HOLD for payment 2023-08-24] [HOLD for payment 2023-08-21] [$1000] Split bill - 'No results found' appears after selecting the user in split bill member search #22959

Closed lanitochka17 closed 1 year ago

lanitochka17 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:

  1. Go to staging.new.expensify.com
  2. Click + icon > Split bill > Enter amount > Next.
  3. Type an email address.
  4. Select the member in search result

    Expected Result:

    There is no error message after selecting the member in the search result

Actual Result:

No results found' appears after selecting the member in the search result

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.41.0

Reproducible in staging?: Yes

Reproducible in production?: Yes

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/78819774/7f2d84a4-f686-4f5f-b712-57e573250eda

Expensify/Expensify Issue URL:

Issue reported by: Applause - Interanl Team

Slack conversation:

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0199d5a89c5f1a937a
  • Upwork Job ID: 1681294844527841280
  • Last Price Increase: 2023-08-01
  • Automatic offers:
    • fedirjh | Reviewer | 25952719
melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

allroundexperts commented 1 year ago

Proposal

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

No results found appears after selecting the user in split bill number search

What is the root cause of that problem?

The root cause of the issue is that when selecting a participant which does not exist and needs to be invited, after getting selected, we're emptying our userToInvite array. This is incorrect since the user still needs to be invited. Since the invited user array is empty, when getting the header message here, we get a no result found error to show up.

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

Similar to how this is being done in the new chat page, we can change this to:

const chatOptions = OptionsListUtils.getNewChatOptions(reports, personalDetails, betas, searchTerm, [], CONST.EXPENSIFY_EMAILS);

Then, we'll need to change this to:

const chatOptions = OptionsListUtils.getNewChatOptions(reports, personalDetails, betas, searchTerm, [], CONST.EXPENSIFY_EMAILS);

We'll finally also need to replace this with:

  const filterText = _.reduce(participants, (str, {login}) => `${str} ${login}`, '');
  const hasUnselectedUserToInvite = newChatOptions.userToInvite && !filterText.includes(newChatOptions.userToInvite.login);

  if (hasUnselectedUserToInvite) {

What alternative solutions did you explore? (Optional)

We need to replace this with:

        newChatOptions.personalDetails.length + 
        newChatOptions.recentReports.length + 
        _.filter(participants, ({login}) => login.includes(searchTerm)).length !== 0,
melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~0199d5a89c5f1a937a

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

mallenexpensify commented 1 year ago

The root cause of the issue is that when selecting a participant which does not exist and needs to be invited,

@allroundexperts , this happens with existing accounts too, was able to reproduce

image
spcheema commented 1 year ago

The root cause of the issue is that when selecting a participant which does not exist and needs to be invited,

@allroundexperts , this happens with existing accounts too, was able to reproduce

Regardless of new or existing participants, getHeaderMessage func should not preform any further validation when the given search term is valid phone number or email.

allroundexperts commented 1 year ago

Regardless of new or existing participants, getHeaderMessage func should preform any further validation when the given search term is valid phone number or email.

I'm not fully sure about this to be honest. If there's any place where we're searching for existing users only, then I think this will not show No members found error. I don't know if such a place exists now.

spcheema commented 1 year ago

Regardless of new or existing participants, getHeaderMessage func should not preform any further validation when the given search term is valid phone number or email.

I'm not fully sure about this to be honest. If there's any place where we're searching for existing users only, then I think this will not show No members found error. I don't know if such a place exists now.

@allroundexperts Since you are an expert contributor so you have more responsibility 😉

Below condition in the getHeaderMessage does not make sense if the given search term is valid. Adding users to the group is just band-aid way to fix the problem. I would suggest to test it and give your valuable opinion. Honestly I am new to this app but flow control isn't relevant.

if (searchValue && !hasSelectableOptions && !hasUserToInvite) { 
     if (/^\d+$/.test(searchValue) && !isValidPhone) { 
         return Localize.translate(preferredLocale, 'messages.errorMessageInvalidPhone'); 
     } 
     if (/@/.test(searchValue) && !isValidEmail) { 
         return Localize.translate(preferredLocale, 'messages.errorMessageInvalidEmail'); 
     } 

     return Localize.translate(preferredLocale, 'common.noResultsFound'); 
 }
allroundexperts commented 1 year ago

Adding users to the group is just band-aid way to fix the problem

I don't think that this statement is entirely correct. A user which does not exist, should be added to userToInvite array. This is what this array is meant for. In current case, we're not adding that once we select that user. This is in-consistent with how we're doing this elsewhere in our App. That being said, I could be wrong as well.

Also, since I've proposed a solution here, I would really refrain from testing your solution and giving my opinion since this would be a case of conflict of interest.

As such, I'll wait for @fedirjh to weigh in here!

spcheema commented 1 year ago

Adding users to the group is just band-aid way to fix the problem

I don't think that this statement is entirely correct. A user which does not exist, should be added to userToInvite array. This is what this array is meant for. In current case, we're not adding that once we select that user. This is in-consistent with how we're doing this elsewhere in our App. That being said, I could be wrong as well.

Also, since I've also proposed a solution here as well, I would really refrain from testing your solution and giving my opinion since this would be a case of conflict of interest.

As such, I'll wait for @fedirjh to weigh in here!

I think I miss understood the requirements. You are right. We need that list in the case of existing participants.

mallenexpensify commented 1 year ago

Still waiting on proposals

luckyman0305 commented 1 year ago

Proposal

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

On the split bill page, after search Name or email and select user, there is an error message like "No results found" Should be no error message.

What is the root cause of that problem?

const headerMessage = OptionsListUtils.getHeaderMessage(
        newChatOptions.personalDetails.length + newChatOptions.recentReports.length !== 0,
        Boolean(newChatOptions.userToInvite),
        searchTerm,
        maxParticipantsReached,
    );

This function has no considered the selected members in the search result. Should consider it.

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

Should consider selected members that is included in search text. From https://github.com/Expensify/App/blob/75e737b91867e01f83b04e4c36e0d02328e5727a/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSplitSelector.js#L129-L133 To

            if (isOptionInList) {
                newSelectedOptions = _.reject(participants, (selectedOption) => selectedOption.accountID === option.accountID);
            } else {
                newSelectedOptions = [...participants, {accountID: option.accountID, login: option.login, selected: true, searchText: option.searchText}];
            }

From https://github.com/Expensify/App/blob/75e737b91867e01f83b04e4c36e0d02328e5727a/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSplitSelector.js#L148-L153 To

    const headerMessage = OptionsListUtils.getHeaderMessage(
        newChatOptions.personalDetails.length + newChatOptions.recentReports.length !== 0,
        Boolean(newChatOptions.userToInvite),
        searchTerm,
        maxParticipantsReached,
        participants.filter(participant => participant.searchText.includes(searchTerm)).length > 0
    );

From https://github.com/Expensify/App/blob/75e737b91867e01f83b04e4c36e0d02328e5727a/src/libs/OptionsListUtils.js#L1002-L1026

To


function getHeaderMessage(hasSelectableOptions, hasUserToInvite, searchValue, maxParticipantsReached = false, hasMatchedParticipant=false) { 
     if (maxParticipantsReached) { 
         return Localize.translate(preferredLocale, 'common.maxParticipantsReached', {count: CONST.REPORT.MAXIMUM_PARTICIPANTS}); 
     } 

     const isValidPhone = parsePhoneNumber(LoginUtils.appendCountryCode(searchValue)).possible; 

     const isValidEmail = Str.isValidEmail(searchValue); 

     if (searchValue && CONST.REGEX.DIGITS_AND_PLUS.test(searchValue) && !isValidPhone) { 
         return Localize.translate(preferredLocale, 'messages.errorMessageInvalidPhone'); 
     } 

     // Without a search value, it would be very confusing to see a search validation message. 
     // Therefore, this skips the validation when there is no search value. 
     if (searchValue && !hasSelectableOptions && !hasUserToInvite) { 
         if (/^\d+$/.test(searchValue) && !isValidPhone) { 
             return Localize.translate(preferredLocale, 'messages.errorMessageInvalidPhone'); 
         } 
         if (/@/.test(searchValue) && !isValidEmail) { 
             return Localize.translate(preferredLocale, 'messages.errorMessageInvalidEmail'); 
         } 
         if (!hasMatchedParticipant)
             return Localize.translate(preferredLocale, 'common.noResultsFound'); 
     }

What alternative solutions did you explore? (Optional)

https://github.com/Expensify/App/assets/139998585/baf9c6ca-d9a0-4b56-9307-8e0f71f84ba5

mallenexpensify commented 1 year ago

@fedirjh can you review @StanislavLavrenchuk 's proposal above please?

fedirjh commented 1 year ago

I think @allroundexperts's proposal make sense to me. We just make it consistent with new group page.

@StanislavLavrenchuk OptionsListUtils.getHeaderMessage is used in different pages , and each page have a different behaviour , have you considered that ?

luckyman0305 commented 1 year ago

@fedirjh thanks your review. we should change only the first param for this feature. no need change OptionsListUtils.getHeaderMessage function. from https://github.com/Expensify/App/blob/cd851d0fbd901c70a32220472104ffc887639f2a/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSplitSelector.js#L148-L153 to

   const headerMessage = OptionsListUtils.getHeaderMessage(
        newChatOptions.personalDetails.length + newChatOptions.recentReports.length + participants.length!== 0,
fedirjh commented 1 year ago

@StanislavLavrenchuk I think your solution will break other error handling , for example if we type a wrong email address it will not display any error

Actual Proposal result
Screenshot 2023-07-25 at 1 58 26 PM Screenshot 2023-07-25 at 1 57 55 PM
melvin-bot[bot] commented 1 year ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

luckyman0305 commented 1 year ago

@fedirjh Thanks for your reply. I will check again, thanks

luckyman0305 commented 1 year ago

From https://github.com/Expensify/App/blob/0fec944de6f8014f6a4586c7d4b988ffdf8ea495/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSplitSelector.js#L148-L153 To

    const headerMessage = OptionsListUtils.getHeaderMessage(
            newChatOptions.personalDetails.length + newChatOptions.recentReports.length !== 0,
            Boolean(newChatOptions.userToInvite),
            searchTerm,
            maxParticipantsReached,
            participants.length > 0
        );

From https://github.com/Expensify/App/blob/0fec944de6f8014f6a4586c7d4b988ffdf8ea495/src/libs/OptionsListUtils.js#L993-L1029

To

/**
 * Helper method that returns the text to be used for the header's message and title (if any)
 *
 * @param {Boolean} hasSelectableOptions
 * @param {Boolean} hasUserToInvite
 * @param {String} searchValue
 * @param {Boolean} [maxParticipantsReached]
 * @param {Boolean} [isSelectedOption]
 * @return {String}
 */
function getHeaderMessage(hasSelectableOptions, hasUserToInvite, searchValue, maxParticipantsReached = false, isSelectedOption = false) {
    if (maxParticipantsReached) {
        return Localize.translate(preferredLocale, 'common.maxParticipantsReached', {count: CONST.REPORT.MAXIMUM_PARTICIPANTS});
    }

    const isValidPhone = parsePhoneNumber(LoginUtils.appendCountryCode(searchValue)).possible;

    const isValidEmail = Str.isValidEmail(searchValue);

    if (searchValue && CONST.REGEX.DIGITS_AND_PLUS.test(searchValue) && !isValidPhone) {
        return Localize.translate(preferredLocale, 'messages.errorMessageInvalidPhone');
    }

    // Without a search value, it would be very confusing to see a search validation message.
    // Therefore, this skips the validation when there is no search value.
    if (searchValue && !hasSelectableOptions && !hasUserToInvite) {
        if (/^\d+$/.test(searchValue) && !isValidPhone) {
            return Localize.translate(preferredLocale, 'messages.errorMessageInvalidPhone');
        }
        if (/@/.test(searchValue) && !isValidEmail) {
            return Localize.translate(preferredLocale, 'messages.errorMessageInvalidEmail');
        }
        if(!isSelectedOption)
            return Localize.translate(preferredLocale, 'common.noResultsFound');
    }

    return '';
}
melvin-bot[bot] commented 1 year ago

📣 @Luckyman0305! 📣 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. 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.
  2. 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.
  3. 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>
fedirjh commented 1 year ago

@StanislavLavrenchuk I think your approach to fixing the getHeaderMessage makes sense, but we should consider all instances where it was used. Could you please check when common.noResultsFound is displayed ?

luckyman0305 commented 1 year ago

@fedirjh thanks for your reply. We no need check when common.noResultsFound is displayed. because we will add isSelectedOption = false param in getHeaderMessage function and use only for this feature.

image

image

image

fedirjh commented 1 year ago

We no need check when common.noResultsFound is displayed.

@StanislavLavrenchuk Yes, I see your point. My intention is that common.noResultsFound is needed only in a few specific places under certain conditions. We can update the logic and display not found when necessary, and for that, we should list all possible cases. Lastly, the condition participants.length > 0 seems misleading to me, as it doesn't strictly imply that the user is selected. In other scenarios, the user can be excluded as well. For example, if you select a few users and then search for your own email, it should display common.noResultsFound. However, with your current approach, it will not display any result as the participants' list is not empty.

luckyman0305 commented 1 year ago

@fedirjh I am clear what you mean, You are right. need to find other solution.

allroundexperts commented 1 year ago

@fedirjh can you also check my proposal please?

luckyman0305 commented 1 year ago

@fedirjh Please check this, I think this is working well From https://github.com/Expensify/App/blob/75e737b91867e01f83b04e4c36e0d02328e5727a/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSplitSelector.js#L129-L133 To

            if (isOptionInList) {
                newSelectedOptions = _.reject(participants, (selectedOption) => selectedOption.accountID === option.accountID);
            } else {
                newSelectedOptions = [...participants, {accountID: option.accountID, login: option.login, selected: true, searchText: option.searchText}];
            }

From https://github.com/Expensify/App/blob/75e737b91867e01f83b04e4c36e0d02328e5727a/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSplitSelector.js#L148-L153 To

    const headerMessage = OptionsListUtils.getHeaderMessage(
        newChatOptions.personalDetails.length + newChatOptions.recentReports.length !== 0,
        Boolean(newChatOptions.userToInvite),
        searchTerm,
        maxParticipantsReached,
        participants.filter(participant => participant.searchText.includes(searchTerm)).length > 0
    );

From https://github.com/Expensify/App/blob/75e737b91867e01f83b04e4c36e0d02328e5727a/src/libs/OptionsListUtils.js#L1024-L1026

To


        if (!hasMatchedParticipant)
            return Localize.translate(preferredLocale, 'common.noResultsFound');
fedirjh commented 1 year ago

can you also check my proposal please?

@allroundexperts Your proposal looks valid; however, I believe it was suggested as a solution on top of a bad implementation. When examining this line, we can see that getNewChatOptions expects an array of selectedOptions. Thus, passing an empty array when we already have selected options does not make sense and appears to be a mere patch for the bug rather than addressing its root cause similar to what has been done inside NewChatPage.js.

https://github.com/Expensify/App/blob/e845e87b055cf2aebc2d2789a5edede6b84284ab/src/libs/OptionsListUtils.js#L950

allroundexperts commented 1 year ago

That's fair. Thanks for the feedback @fedirjh!

luckyman0305 commented 1 year ago

hi @fedirjh let me know If my proposal is unacceptable.

spcheema commented 1 year ago

@fedirjh @allroundexperts In my opinion getHeaderMessage should be refactored to consider following:

In order to avoid touching other code, new function can be introduce and gracefully applier to other areas.

If you guys this is right then I can work proposal.

fedirjh commented 1 year ago

hi @fedirjh let me know If my proposal is unacceptable.

@StanislavLavrenchuk Please update your main proposal to include the necessary steps.

In my opinion getHeaderMessage should be refactored to consider following

@spcheema I think that's the right path and I believe that @StanislavLavrenchuk has proposed that. If you have something different please share it on your proposal.

spcheema commented 1 year ago

Thanks @fedirjh. I'll pass then. No point proposing the same thing.

melvin-bot[bot] commented 1 year ago

@mallenexpensify @fedirjh 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!

luckyman0305 commented 1 year ago

Proposal

Updated

melvin-bot[bot] commented 1 year ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 1 year ago

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

fedirjh commented 1 year ago

Reviewing shortly. The proposal looks good, Will give a final test before moving forward.

fedirjh commented 1 year ago

I think we should proceed with @StanislavLavrenchuk's proposal

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

📣 @fedirjh 🎉 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

melvin-bot[bot] commented 1 year ago

❌ There was an error making the offer to @StanislavLavrenchuk1 for the Contributor role. The BZ member will need to manually hire the contributor. cc @thienlnam

madmax330 commented 1 year ago

Issue is assigned, not overdue

melvin-bot[bot] commented 1 year ago

@madmax330 @mallenexpensify @fedirjh @StanislavLavrenchuk1 this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

luckyman0305 commented 1 year ago

Waiting internal review @madmax330 @mallenexpensify

melvin-bot[bot] commented 1 year ago

📣 @luckyman0305! 📣 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. 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.
  2. 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.
  3. 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>
melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

If no regressions arise, payment will be issued on 2023-08-21. :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.

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

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

melvin-bot[bot] commented 1 year 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: