[HOLD] Excluded Concierge, Chronos and receipts from IOU Requests search #4504

Santhosh-Sellavel · 2021-08-07T23:35:12Z

Details

Just excluded bot accounts from IOU Participants Search Results

Fixed Issues

Tests & QA Steps

Search any of the following emails on the IOU request/split participants page.

 CHRONOS - 'chronos@expensify.com',
 CONCIERGE - 'concierge@expensify.com',
 RECEIPTS - 'receipts@expensify.com',

Fix - In search results, the above emails will not appear now.

Note: Chronos/Receipts may appear in recent items - it will be handled in another issue check issue description for more.

Tested On

Screenshots

Screenshots for all platforms tested. Pull requests won't be merged unless the screenshots show the app was tested on all platforms.-->

Desktop

Web

Mobile Web

iOS

Android

Santhosh-Sellavel · 2021-08-20T18:18:50Z

@marcaaron I see a conflict in this PR due to some recent changes so will move it to draft and mark it ready after resolving conflicts.

Santhosh-Sellavel · 2021-08-20T21:28:53Z

I Have tested it once again & all checks are passed marking it ready for review.

marcaaron

Looks good. Just left a few comments.

marcaaron · 2021-08-23T17:37:48Z

src/libs/OptionsListUtils.js

@@ -307,6 +307,25 @@ function isCurrentUser(userDetails) {
    return result;
 }

+/**
+ * Returns whether the given userDetails is excluded from IOU request
+ * @param {Objecy} userDetails


typo Object

marcaaron · 2021-08-23T17:37:55Z

src/libs/OptionsListUtils.js

+/**
+ * Returns whether the given userDetails is excluded from IOU request
+ * @param {Objecy} userDetails
+ *  @returns {Bool}


marcaaron · 2021-08-23T17:46:22Z

src/libs/OptionsListUtils.js

+
+    if (userDetails && userDetails.login) {
+        const login = userDetails.login.toLowerCase();
+        return login === CONST.EMAIL.CHRONOS || login === CONST.EMAIL.CONCIERGE || login === CONST.EMAIL.RECEIPTS;


Something like this is a bit more readable to me. And we could also make the array a constant like EXPENSIFY_EMAILS.

_.contains([CONST.EMAIL.CHRONOS, CONST.EMAIL.CONCIERGE, CONST.EMAIL.RECEIPTS], login);

For adding EXPENSIFY_EMAILS in CONST,
with underscore (we need to import underscore)

get EXPENSIFY_EMAILS() { return _.map(_.allKeys(this.EMAIL), key => this.EMAIL[key]); },

Without _ underscore library

get EXPENSIFY_EMAILS() { return Object.keys(this.EMAIL).map(key => this.EMAIL[key]); },

which way do you prefer @marcaaron ?

_.keys() is what we use instead of Object.keys()

but _.map() also works on object values

I'm not sure I'd do what you are suggesting though because if someone adds a new CONST.EMAIL.SOMETHING it will automatically get excluded from the IOUs (which might not be intended). Better to be specific I think.

So with the underscore library then, I'll update a PR Soon!

Yeah, but I'm also saying we can skip underscore entirely and just do something explicit like this

get EXPENSIFY_EMAILS() { return [ this.EMAIL.CHRONOS, this.EMAIL.CONCIERGE, this.EMAIL.RECEIPTS, ]; }

I'm not sure I'd do what you are suggesting though because if someone adds a new CONST.EMAIL.SOMETHING will automatically get excluded from the IOUs (which might not be intended). Better to be specific I think.

Yes, it might be unintended, so like you said we can keep it specific. Then this should work.

get EXPENSIFY_EMAILS() { return [ this.EMAIL.CHRONOS, this.EMAIL.CONCIERGE, this.EMAIL.RECEIPTS, ]; },

Santhosh-Sellavel · 2021-08-23T20:27:35Z

Working on the request changes

Santhosh-Sellavel · 2021-08-23T23:34:44Z

@marcaaron Requested changes have been updated.

marcaaron

This is testing well for me. Just leaving a thought here on how to improve this. I feel the isExcludedIOUUsers method we are exporting is unlikely to be used in many places and should be part of the internal logic of getOptions()

marcaaron · 2021-08-24T03:03:38Z

src/pages/iou/steps/IOUParticipantsPage/IOUParticipantsRequest.js

@@ -94,7 +94,7 @@ class IOUParticipantsRequest extends Component {
            indexOffset: 0,
        });

-        if (this.state.userToInvite && !isCurrentUser(this.state.userToInvite)) {
+        if (this.state.userToInvite && !isExcludedIOUUsers(this.state.userToInvite)) {


This looked good to me initially, but I am now second guessing it. It feels like it would be more appropriate to just not return a userToInvite from getOptions().

Then we would not need to import this method into these two places and would probably not need the method at all. Just don't allow setting userToInvite if they are one of the emails:

App/src/libs/OptionsListUtils.js

Lines 505 to 525 in 95bf36d

let userToInvite = null;

if (searchValue

&& recentReportOptions.length === 0

&& personalDetailsOptions.length === 0

&& !isCurrentUser({login: searchValue})

&& _.every(selectedOptions, option => option.login !== searchValue)

&& ((Str.isValidEmail(searchValue) && !Str.isDomainEmail(searchValue)) || Str.isValidPhone(searchValue))

&& (!_.find(loginOptionsToExclude, loginOptionToExclude => loginOptionToExclude.login === searchValue))

&& (searchValue !== CONST.EMAIL.CHRONOS || Permissions.canUseChronos(betas))

) {

// If the phone number doesn't have an international code then let's prefix it with the

// current user's international code based on their IP address.

const login = (Str.isValidPhone(searchValue) && !searchValue.includes('+'))

? `+${countryCodeByIP}${searchValue}`

: searchValue;

const userInvitePersonalDetails = getPersonalDetailsForLogins([login], personalDetails);

userToInvite = createOption(userInvitePersonalDetails, null, draftComments, {

showChatPreviewLine,

});

userToInvite.icons = [defaultAvatarForUserToInvite];

}

We'd still need to do some kind of boolean flag like shouldInviteExpensifyEmails passed to getOptions() as there are some cases where we do want to be able to invite these users.

There are already separate flags, we could reuse them or add a separate flag?

App/src/libs/OptionsListUtils.js

Lines 325 to 327 in 95bf36d

excludeConcierge = false,

excludeChronos = false,

excludeReceipts = false,

If we are going to reuse, then modify the line

App/src/libs/OptionsListUtils.js

Line 512 in 95bf36d

&& (!_.find(loginOptionsToExclude, loginOptionToExclude => loginOptionToExclude.login === searchValue))

as follows,

&& (!_.find(loginOptionsToExclude, loginOptionToExclude => loginOptionToExclude.login.toLowerCase() === searchValue.toLowerCase()))

If we are going to add a separate boolean shouldInviteExpensifyEmails with default value as false (if we want to go with the default true we change the below line accordingly)

&& (!shouldInviteExpensifyEmails && !_.contains(CONST.EXPENSIFY_EMAILS, searchValue.toLowerCase()))

Either way, we can just remove the isExcludedIOUUsers method.

What can we do here?
@marcaaron post your thoughts, I'll update PR, once you are okay with the proposed changes!

Oh hmm... well if they are already excluded from the list of options then it seems reasonable to also not allow them to be returned as the userToInvite.

If we are going to reuse, then modify the line

I'm confused now. So there is already logic to exclude them but why are they are not getting excluded? Why do they show in the list then? 😦

Ok to make this a bit more complicated (lol) it looks like there is already an array of "automated" expensify emails that was recently added in this other PR...

https://github.com/Expensify/App/pull/4755/files#diff-9d6b0cc89c7cab66ac4042ac0549bfc66ddc9e7a182b6b5dbbf7f687328f10b5R264-R399

I think we should maybe discuss back in the issue how to handle this. Given that there are also only emails that we may want to exclude I think we should put this back into the proposal state and maybe pause this PR review to get some more feedback from others.

@marcaaron

I'm confused now. So there is already logic to exclude them but why are they are not getting excluded? Why do they show in the list then? 😦
Because we do only === not check without about case sensitivity

Check with the current staging app, search receipts@expensify.com it won't show upresults. But search Receipts@expensify.com it will show up

with existing options, this should work the below solution will work

&& (!_.find(loginOptionsToExclude, loginOptionToExclude => loginOptionToExclude.login.toLowerCase() === searchValue.toLowerCase()))

Hmm I checked on main branch. The bug we are "fixing" seems already fixed. Putting this PR on hold.

We can fix the casing issue, but that's not what we set out to do (and is a minor detail of the larger issue presented). So let's discuss how to proceed on the issue? Thanks!

Santhosh-Sellavel · 2021-08-27T03:52:46Z

Closing, Since this PR is no longer required!

Excluded Concierge, Chronos and receipts from IOU Requests search

f3b74b7

Santhosh-Sellavel closed this Aug 11, 2021

Santhosh-Sellavel reopened this Aug 17, 2021

Merge branch 'main' into IOU_Exclude_Concierge_Chronus

f9be260

Santhosh-Sellavel marked this pull request as ready for review August 17, 2021 20:08

Santhosh-Sellavel requested a review from a team as a code owner August 17, 2021 20:08

MelvinBot requested review from marcaaron and removed request for a team August 17, 2021 20:08

Christinadobrzyn mentioned this pull request Aug 20, 2021

Block chronos@, receipts@ and concierge@ from Split Bill and Request Money #4413

Closed

5 tasks

Santhosh-Sellavel marked this pull request as draft August 20, 2021 18:24

Conflicts Resolved

1d63ad1

Santhosh-Sellavel marked this pull request as ready for review August 20, 2021 21:29

marcaaron requested changes Aug 23, 2021

View reviewed changes

Added EXPENSIFY_EMAILS in CONST, updated some typos

7fe030f

Santhosh-Sellavel requested a review from marcaaron August 23, 2021 23:34

marcaaron assigned Santhosh-Sellavel Aug 23, 2021

marcaaron requested changes Aug 24, 2021

View reviewed changes

marcaaron changed the title ~~Excluded Concierge, Chronos and receipts from IOU Requests search~~ [HOLD] Excluded Concierge, Chronos and receipts from IOU Requests search Aug 24, 2021

Santhosh-Sellavel closed this Aug 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HOLD] Excluded Concierge, Chronos and receipts from IOU Requests search #4504

[HOLD] Excluded Concierge, Chronos and receipts from IOU Requests search #4504

Santhosh-Sellavel commented Aug 7, 2021 •

edited

Loading

Santhosh-Sellavel commented Aug 20, 2021 •

edited

Loading

Santhosh-Sellavel commented Aug 20, 2021

marcaaron left a comment

marcaaron Aug 23, 2021

marcaaron Aug 23, 2021

marcaaron Aug 23, 2021

Santhosh-Sellavel Aug 23, 2021

marcaaron Aug 23, 2021

marcaaron Aug 23, 2021

marcaaron Aug 23, 2021

Santhosh-Sellavel Aug 23, 2021

marcaaron Aug 23, 2021

Santhosh-Sellavel Aug 23, 2021

Santhosh-Sellavel commented Aug 23, 2021

Santhosh-Sellavel commented Aug 23, 2021

marcaaron left a comment

marcaaron Aug 24, 2021

Santhosh-Sellavel Aug 24, 2021 •

edited

Loading

marcaaron Aug 24, 2021

Santhosh-Sellavel Aug 24, 2021 •

edited

Loading

marcaaron Aug 24, 2021

Santhosh-Sellavel commented Aug 27, 2021

	let userToInvite = null;
	if (searchValue
	&& recentReportOptions.length === 0
	&& personalDetailsOptions.length === 0
	&& !isCurrentUser({login: searchValue})
	&& _.every(selectedOptions, option => option.login !== searchValue)
	&& ((Str.isValidEmail(searchValue) && !Str.isDomainEmail(searchValue)) \|\| Str.isValidPhone(searchValue))
	&& (!_.find(loginOptionsToExclude, loginOptionToExclude => loginOptionToExclude.login === searchValue))
	&& (searchValue !== CONST.EMAIL.CHRONOS \|\| Permissions.canUseChronos(betas))
	) {
	// If the phone number doesn't have an international code then let's prefix it with the
	// current user's international code based on their IP address.
	const login = (Str.isValidPhone(searchValue) && !searchValue.includes('+'))
	? `+${countryCodeByIP}${searchValue}`
	: searchValue;
	const userInvitePersonalDetails = getPersonalDetailsForLogins([login], personalDetails);
	userToInvite = createOption(userInvitePersonalDetails, null, draftComments, {
	showChatPreviewLine,
	});
	userToInvite.icons = [defaultAvatarForUserToInvite];
	}

	excludeConcierge = false,
	excludeChronos = false,
	excludeReceipts = false,

[HOLD] Excluded Concierge, Chronos and receipts from IOU Requests search #4504

[HOLD] Excluded Concierge, Chronos and receipts from IOU Requests search #4504

Conversation

Santhosh-Sellavel commented Aug 7, 2021 • edited Loading

Details

Fixed Issues

Tests & QA Steps

Tested On

Screenshots

Desktop

Web

Mobile Web

iOS

Android

Santhosh-Sellavel commented Aug 20, 2021 • edited Loading

Santhosh-Sellavel commented Aug 20, 2021

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Santhosh-Sellavel commented Aug 23, 2021

Santhosh-Sellavel commented Aug 23, 2021

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Santhosh-Sellavel Aug 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Santhosh-Sellavel Aug 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Santhosh-Sellavel commented Aug 27, 2021

Santhosh-Sellavel commented Aug 7, 2021 •

edited

Loading

Santhosh-Sellavel commented Aug 20, 2021 •

edited

Loading

Santhosh-Sellavel Aug 24, 2021 •

edited

Loading

Santhosh-Sellavel Aug 24, 2021 •

edited

Loading