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.

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

[payment DUE] [$250] Don't show GBR to AM/guides for managed customer employee invitations #39659

Closed
2 of 6 tasks
joekaufmanexpensify opened this issue Apr 4, 2024 · 43 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review

Comments

@joekaufmanexpensify
Copy link
Contributor

joekaufmanexpensify commented Apr 4, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Issue reported by: @joekaufmanexpensify / @greg-schroeder
Slack conversation: https://expensify.slack.com/archives/C03U7DCU4/p1709891758477419

Action Performed:

  1. Expensify account manager/guide is added to customer's workspace to assist them.
  2. Employee requests to join workspace.

Expected Result:

Account manager/guide should not have the workspace join request pinned to their LHN, since they are not a part of the company, and should not action it on their behalf.

Actual Result:

Account manager/guide has the workspace join request pinned to their LHN as GBR.

image

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01b0c3b670623a55c2
  • Upwork Job ID: 1775996355860926464
  • Last Price Increase: 2024-04-11
  • Automatic offers:
    • Pujan92 | Reviewer | 0
@joekaufmanexpensify joekaufmanexpensify added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Apr 4, 2024
@melvin-bot melvin-bot bot changed the title Don't show GBR to AM/guides for managed customer employee invitations [$250] Don't show GBR to AM/guides for managed customer employee invitations Apr 4, 2024
Copy link

melvin-bot bot commented Apr 4, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 4, 2024
Copy link

melvin-bot bot commented Apr 4, 2024

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

Copy link

melvin-bot bot commented Apr 4, 2024

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

@joekaufmanexpensify
Copy link
Contributor Author

Another proposal here is to make an explicit AM/guide an explicit workspace role. Which could be the better long term solution

@twisterdotcom
Copy link
Contributor

I am a huge fan of doing whatever needs to be done to fix this quickly and for now. This is just causing super high annoyance and noise for me and every single person on our Sales and Account Management team.

@ShridharGoel
Copy link
Contributor

ShridharGoel commented Apr 5, 2024

Proposal

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

Account manager gets report shown as having a new join request in LHN.

What is the root cause of that problem?

The current logic doesn't check if the user is an account manager before pinning and marking as unread + showing the green dot.

if (ReportActionsUtils.isActionableJoinRequestPending(report.reportID)) {
    result.isPinned = true;
    result.isUnread = true;
    result.brickRoadIndicator = CONST.BRICK_ROAD_INDICATOR_STATUS.INFO;
}

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

We can add a check to not show as pinned, unread and green dot if the user is from Expensify team.

if (ReportActionsUtils.isActionableJoinRequestPending(report.reportID) && !PolicyUtils.isExpensifyTeam(personalDetail?.login)) {
    result.isPinned = true;
    result.isUnread = true;
    result.brickRoadIndicator = CONST.BRICK_ROAD_INDICATOR_STATUS.INFO;
}

We might also need to add another condition: if the owner is from Expensify team, then we can show as pinned, unread and with green dot for Expensify accounts also because that might mean that the workspace belongs to Expensify itself.

The above is something that needs discussion.

Code is this situation can be:

if (ReportActionsUtils.isActionableJoinRequestPending(report.reportID)) {
    if (PolicyUtils.isExpensifyTeam(personalDetail?.login) && !PolicyUtils.isExpensifyTeam(result.ownerEmail)) {
        return;
    }
    result.isPinned = true;
    result.isUnread = true;
    result.brickRoadIndicator = CONST.BRICK_ROAD_INDICATOR_STATUS.INFO;
}

@nkdengineer
Copy link
Contributor

nkdengineer commented Apr 5, 2024

Proposal

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

Account manager/guide has the workspace join request pinned to their LHN as GBR.

What is the root cause of that problem?

We already have a method isJoinRequestInAdminRoom to check whether to show GBR here, but currently don't prevent Expensify AM/guides from seeing the GBR from customer workspace's employee invitations.

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

In isJoinRequestInAdminRoom here, if we check that the currentUserPersonalDetails belongs to an Expensify team member, and the policy is not created by an Expensify team member (meaning the policy belongs to the customer). Then we can early return false to avoid distractions to Expensify team member.

if (report.policyID) {
    const policy = getPolicy(report.policyID);
    if (!PolicyUtils.isExpensifyTeam(policy.owner) && PolicyUtils.isExpensifyTeam(currentUserPersonalDetails?.login ?? currentUserPersonalDetails?.displayName)) {
        return false;
    }
}

In here, we can also change to use the isJoinRequestInAdminRoom for consistency

if (ReportUtils.isJoinRequestInAdminRoom(report)) {

The above is similar logic to what we already do here to decide whether to show the Expensify team member as workspace member.

To make this more robust, we can pass the necessary data like policy down to the isJoinRequestInAdminRoom from the caller instead of getting it from cached value.

The earlier suggested proposal won't work because the LHN GBR logic is not there.

What alternative solutions did you explore? (Optional)

NA

@Pujan92
Copy link
Contributor

Pujan92 commented Apr 5, 2024

@joekaufmanexpensify @greg-schroeder I think this should be internal as we can't reproduce/test this because it requires AM/guide or expensify account.

@ShridharGoel
Copy link
Contributor

@Pujan92 It can be tested by hardcoding the email ID.

@ShridharGoel
Copy link
Contributor

But yeah, we'll need to login to test. Is there any test ID with expensify domain?

@Pujan92
Copy link
Contributor

Pujan92 commented Apr 5, 2024

At the moment I am facing an issue with inviting a user with the workspace join link and seems to be a regression from here.

@Pujan92
Copy link
Contributor

Pujan92 commented Apr 5, 2024

But yeah, we'll need to login to test. Is there any test ID with expensify domain?

I don't think so but let @joekaufmanexpensify confirm.

@melvin-bot melvin-bot bot added the Overdue label Apr 8, 2024
@joekaufmanexpensify
Copy link
Contributor Author

I am a huge fan of doing whatever needs to be done to fix this quickly and for now. This is just causing super high annoyance and noise for me and every single person on our Sales and Account Management team.

@twisterdotcom Totally fair. I think the better long term solution is to create a new user role on workspaces for guides/AMs. Not sure how much additional work it is relative to just changing the behavior of these messages (though I assume it is some). If it's a lot more work, perhaps we could just change this now, and then open a follow up issue to explore the new user-role more holistically.

@joekaufmanexpensify
Copy link
Contributor Author

But yeah, we'll need to login to test. Is there any test ID with expensify domain?

@Pujan92 @ShridharGoel could you clarify what test ID is referring to here?

@ShridharGoel
Copy link
Contributor

@joekaufmanexpensify I meant a test account with Expensify domain which we can use to login.

Copy link

melvin-bot bot commented Apr 8, 2024

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

@joekaufmanexpensify
Copy link
Contributor Author

Got it. Hmm, yeah I don't think we have a test expensify.com account that you could test with at this time.

@twisterdotcom
Copy link
Contributor

Can we just push it and have somebody test on staging?

@Pujan92
Copy link
Contributor

Pujan92 commented Apr 9, 2024

Can we just push it and have somebody test on staging?

Yes, that is doable.

@melvin-bot melvin-bot bot removed the Overdue label Apr 9, 2024
@melvin-bot melvin-bot bot added the Overdue label Apr 11, 2024
@zanyrenney
Copy link
Contributor

how is the PR review going @nkdengineer @Pujan92

@melvin-bot melvin-bot bot removed the Overdue label Apr 11, 2024
@Pujan92
Copy link
Contributor

Pujan92 commented Apr 11, 2024

Waiting for @marcochavezf to review the comment #39659 (comment)

@marcochavezf
Copy link
Contributor

Thanks for the review @Pujan92, assigning @nkdengineer

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 11, 2024
Copy link

melvin-bot bot commented Apr 11, 2024

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

Copy link

melvin-bot bot commented Apr 11, 2024

📣 @nkdengineer You have been assigned to this job!
Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs!
Keep in mind: Code of Conduct | Contributing 📖

@nkdengineer
Copy link
Contributor

@Pujan92 The PR is here.

@Pujan92
Copy link
Contributor

Pujan92 commented Apr 30, 2024

@zanyrenney Seems automation is broken here, payment is due now.

@Pujan92
Copy link
Contributor

Pujan92 commented May 10, 2024

Bump @zanyrenney :)

@zanyrenney
Copy link
Contributor

Seems liks there might be an issue here - When I click the offer link in the GH issue, it seems like it doesn't exist.
2024-05-14_12-24-09

@zanyrenney zanyrenney changed the title [$250] Don't show GBR to AM/guides for managed customer employee invitations [payment DUE] [$250] Don't show GBR to AM/guides for managed customer employee invitations May 14, 2024
@zanyrenney zanyrenney added Daily KSv2 and removed Weekly KSv2 labels May 14, 2024
@Pujan92
Copy link
Contributor

Pujan92 commented May 14, 2024

@zanyrenney The Offer has already been accepted, Plz check this Upwork job link

@zanyrenney
Copy link
Contributor

zanyrenney commented May 16, 2024

I know - It's paid for you @Pujan92
2024-05-16_14-17-04

I am waiting for @nkdengineer to accept.

@zanyrenney
Copy link
Contributor

bump @nkdengineer

@nkdengineer
Copy link
Contributor

@zanyrenney Invitation accepted! Sorry for the delay 🙏

@zanyrenney
Copy link
Contributor

Dm @nkdengineer need him to accept offer.

@zanyrenney
Copy link
Contributor

@nkdengineer
Copy link
Contributor

@zanyrenney Offer accepted!

@zanyrenney
Copy link
Contributor

payment summary

paid @nkdengineer $250 via upwork
paid @Pujan92 $250 via upwork

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review
Projects
No open projects
Archived in project
Development

No branches or pull requests

7 participants