App: [HOLD for payment 2023-08-08] [$1000] Web - An appropriate message is not shown when inviting a member that is already invited.

@eh2077 made a proposal on this dupe as well.

I think mapping an array of objects to get only emails/logins are light and are widely done through out the App.

@tienifr I understand that mapping an array of objects to extract only emails/logins is a common practice in the app. However, it seems like we are duplicating the task unnecessarily. We are transforming a simple array of strings into an array of objects and then transforming it back to strings. This approach raises questions about its extensibility and maintainability. Is it truly necessary? Can we explore alternative solutions that are simpler and more efficient in the long run?

Use the correct exclusionReason if the userToInvite is empty and the email is in the exclusion list, here

I’m concerned about the additional complexity it may introduce. If we follow this approach, it seems we would need another loop to retrieve the exclusionReason. This could potentially make the code harder to understand and maintain.

Moreover, considering that we will only be using one email with an exclusionReason, it raises the question of whether it’s necessary to hold all the other emails along with their respective exclusionReason. This approach may introduce unnecessary overhead and could potentially be simplified.

In conclusion, I believe it would be beneficial to explore alternative approaches that address these concerns and provide a more efficient and maintainable solution.

I think designing the codebase to be easily extensible and maintainable should not be discouraged.

Here is example of extensible and optimized approach compared to the suggested one, It would be efficient to :

• Update getExcludedUsers to getExcludedUsersWithMessage and accepts a new param searchUser
• Return both message and list of excluded users
• Avoid extra calls to OptionsListUtils.getMemberInviteOptions when searchUser is excluded.

But I still think we should not go with this approach, given these parts will be refactored in the next months, I think we should go with simplest solution, and if anything is needed to be done in the future then the refactor is the right place to do it.

    getExcludedUsersWithMessage(searchUser) {
        
        let message;
        
        // get list of invited users , which are the policy members
        const invitedUsers = _.compact(
            _.map(this.props.policyMembers, (policyMember, accountID) => {
                if (policyMember.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE || !_.isEmpty(policyMember.errors)) {
                    return;
                }
                return lodashGet(this.props.personalDetails, `[${accountID}].login`, null);
            })
        );
        
        if (invitedUsers.includes(searchUser)) {
            message = this.props.translate('workspace.invitePage.alreadyInvited');
        } else if (CONST.EXPENSIFY_EMAILS.includes(searchUser)) {
            message = this.props.translate('workspace.invitePage.expensifyDomain');
        }

        return {
            message,
            excludedUsers: [...CONST.EXPENSIFY_EMAILS, invitedUsers]
        };
    }

@spcheema , can you please accept the job and reply here once you have? https://www.upwork.com/jobs/~01f5b4bf8537c32990

Note to self… Issue reporter: @daveSeife paid $250 via Upwork (paid early by mistake

Sorry for the delay…

1. The original job closed, @daveSeife is hired for reporting and so is @eh2077 (who will be compensated 75%) . @fedirjh , can you accept the offer please? https://www.upwork.com/jobs/~0129bb1fb0fcd4cb51
2. @spcheema , can you apply to this new job please and reply here once you have? https://www.upwork.com/jobs/~01f5b4bf8537c32990 Pay will be $250 for you

Do I need to apply on the Job?

Thank you for your understanding and cooperation, @spcheema! Yes, please go ahead and apply for the job.

Thanks @pecanoro @fedirjh

Do I need to apply on the Job?

@spcheema The decision is already made so let’s just keep moving with the issue forward.

We discussed it and decided that the fair thing to do is to go with @eh2077 since @fedirjh suggestion was based on this proposal. However, since we recognize that all approaches were similar and inspired by each other’s, we will do 25% for @spcheema and 75% for @eh2077 with @eh2077 being the one raising the PR. Does it sound good?

I will get back to you all later today after we made a decision since it’s a tricky case.

Honestly, after all these discussions, @fedirjh alternative solution is the best one. We don’t need a _.difference, an extra alreadyInvitedUser or a new structure. The if else should be enough:

if (CONST.EXPENSIFY_EMAILS.includes(this.state.searchTerm.trim())) {
    headerMessage = '...'
} else if (excludedUsers.includes(this.state.searchTerm.trim())) {
    headerMessage = '...'
} else {
    headerMessage = OptionsListUtils.getHeaderMessage...;
}

Since it’s an if-else, getting on the first condition means it won’t try to evaluate the second one, so that should be everything that we need. The question is “who will fix it?” 😄

should display another message if we try to add one of the excluded Expensify emails, or should we simply display “not found” ?

Good question @fedirjh , Let’s go with “Invalid email address” since that’s technically what it is and I can’t think of better wording

Thanks @tienifr for the update , the last proposal looks far better than the initially posted one.

_.difference is going to be another loop under the hood. Isn’t it? Then why do use existing loop to maintain a list of excluded members and avoid reverse engineering which @tienifr highlighted already.

@spcheema It could be just simplified, since CONST.EXPENSIFY_EMAILS is included inside excludedUsers we can just reverse the order :

const excludedUsers = this.getExcludedUsers();
let headerMessage;

if (CONST.EXPENSIFY_EMAILS.includes(this.state.searchTerm.trim())) {
    headerMessage = '...'
} else if (excludedUsers.includes(this.state.searchTerm.trim())) {
    headerMessage = '...'
} else {
    headerMessage = OptionsListUtils.getHeaderMessage...;
}

@fedirjh Thanks for reviewing my solution but issue was about to already invited members.

If Expensify email are also considered as members (which means already invited) it can be simply done checking already built list my solution can adapt this requirement.

I can modify the solution accordingly but root cause analysis is the same. Agree?

@eh2077 could you please copy your proposal in this issue for better visibility ?

Let’s just go ahead and fix it. It’s still a bit unclear where the refactor will begin. Thank you so much for confirming!