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.

Sign up for GitHub

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

ignore periods on search #8375

Merged
merged 5 commits into from
Apr 13, 2022
Merged

ignore periods on search #8375

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b6610ba
ignore periods on search
mateusbra Mar 29, 2022
bd09df6
removed unnecessary push to searchTerms and solved edge case
mateusbra Mar 31, 2022
1b2e519
added test unit in order to cover the search with results containing …
mateusbra Apr 7, 2022
2d5e311
added tests for userToInvite
mateusbra Apr 11, 2022
cd963aa
refactor noOptions, noOptionsMatchExactly
mateusbra Apr 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/libs/OptionsListUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ function getSearchText(report, personalDetailList, isChatRoomOrPolicyExpenseChat
if (!isChatRoomOrPolicyExpenseChat) {
_.each(personalDetailList, (personalDetail) => {
searchTerms.push(personalDetail.displayName);
searchTerms.push(personalDetail.login);
searchTerms.push(personalDetail.login.replace(/\./g, ''));
});
}
if (report) {
Expand Down Expand Up @@ -333,6 +333,7 @@ function createOption(personalDetailList, report, {
function isSearchStringMatch(searchValue, searchText, participantNames = new Set(), isChatRoom = false) {
const searchWords = _.map(
searchValue
.replace(/\./g, '')
.replace(/,/g, ' ')
.split(' '),
word => word.trim(),
Expand Down Expand Up @@ -587,8 +588,8 @@ function getOptions(reports, personalDetails, activeReportID, {

let userToInvite = null;
if (searchValue
&& recentReportOptions.length === 0
&& personalDetailsOptions.length === 0
&& ((recentReportOptions.length + personalDetailsOptions.length) === 0
|| (!_.find(loginOptionsToExclude, loginOptionToExclude => loginOptionToExclude.login === searchValue.toLowerCase())))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: It might help readability if we just extract this whole if block to a method like canAddUserToInvite()? It looks pretty crazy and we are duplicating some logic for the login options.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have this feeling too.

What do you think of changing the if block to:

if (canAddUserToInvite(searchValue,recentReportOptions,personalDetailsOptions,loginOptionsToExclude,selectedOptions,loginOptionsToExclude))

adding the method:

function canAddUserToInvite(searchValue,recentReportOptions,personalDetailsOptions,loginOptionsToExclude,selectedOptions){
    const canAddUserToInvite = searchValue && ((recentReportOptions.length + personalDetailsOptions.length) === 0
    || (!_.find(loginOptionsToExclude, loginOptionToExclude => loginOptionToExclude.login === searchValue.toLowerCase())))
    && !isCurrentUser({login: searchValue})
    && _.every(selectedOptions, option => option.login !== searchValue)
    && ((Str.isValidEmail(searchValue) && !Str.isDomainEmail(searchValue)) || Str.isValidPhone(searchValue))
    && (!_.find(loginOptionsToExclude, loginOptionToExclude => loginOptionToExclude.login === searchValue.toLowerCase()))

    return canAddUserToInvite;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are duplicating some logic for the login options.

Yes

thinking A = (!_.find(loginOptionsToExclude, loginOptionToExclude => loginOptionToExclude.login === searchValue.toLowerCase()))
and B = (recentReportOptions.length + personalDetailsOptions.length) === 0
We have A && (A || B), its a redundant logic, we could change to A.
A && (A || B) = A , so we could discard the B logic keeping the same behavior:

function canAddUserToInvite(searchValue,loginOptionsToExclude,selectedOptions){
    const canAddUserToInvite = searchValue
    && !isCurrentUser({login: searchValue})
    && _.every(selectedOptions, option => option.login !== searchValue)
    && ((Str.isValidEmail(searchValue) && !Str.isDomainEmail(searchValue)) || Str.isValidPhone(searchValue))
    && (!_.find(loginOptionsToExclude, loginOptionToExclude => loginOptionToExclude.login === searchValue.toLowerCase()))

    return canAddUserToInvite;
}

let me know what you guys think before I push some changes to it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's a lot of variables for a method, but cleaning it up would be an improvement. We could also maybe making it less of a giant ternary that is hard to make sense out of and use extra variables and if statements (but let's pause that for a second).

As for the redundancy, I agree with the assessment that B would be unneeded - but is the proposal to remove the code to check for available options? Why was the change added in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change was added in order to prevent this edge case @Santhosh-Sellavel found on #8007 (comment) , so we needed to show user to invite even when it doesn't found a result from search(when lenghts == 0)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcaaron nice one! may I commit all thoses changes?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, @marcaaron for the confusion.

You are right,

looking at loginOptionsToExclude doesn’t make sense.

The confusion originated from my suggestions here. #8375 (comment) those were two quick suggestions.

But we would really avoided this conversation if @mateusbra went with first suggestion, because this occurs only when search has dot.

&& ((recentReportOptions.length + personalDetailsOptions.length) === 0 || _.contains(searchValue, '.'))

But this is the better & simpler solution, unfortunately we both missed to foresee

!_.find(personalDetailsOptions.concat(recentReportOptions), option => option.login === searchValue.toLowerCase());

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @Santhosh-Sellavel I thought the second sugestion you proposed was better 😅, but now I think the misunderstoods were solved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I commited the changes, if you think its better to use the first suggestion from #8375 (comment) please tell me.

&& !isCurrentUser({login: searchValue})
&& _.every(selectedOptions, option => option.login !== searchValue)
&& ((Str.isValidEmail(searchValue) && !Str.isDomainEmail(searchValue)) || Str.isValidPhone(searchValue))
Expand Down