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

[HOLD for payment 2023-08-21] [$1000] Web - User is getting error message "Please select at least one member to invite" when invite members again from deeplink #23311

Closed
1 of 6 tasks
kbecciv opened this issue Jul 20, 2023 · 45 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@kbecciv
Copy link

kbecciv commented Jul 20, 2023

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. Choose a Workspace -> Members.
  2. Invite members -> Next.
  3. Copy URL of "Add message" page.
  4. Open a new tab, go to that URL.
  5. Click Invite.
  6. Try to invite new members again to reach "Add message" page.

Expected Result:

User can invite members again

Actual Result:

User is getting error message "Please select at least one member to invite" when invite members again from deeplink.

Workaround:

Unknown

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.43-2
Reproducible in staging?: y
Reproducible in production?: y
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

Screen.Recording.2023-07-20.at.6.58.50.PM.mov
Recording.3794.mp4

Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1689856757832959

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~018e72fd3799cdf925
  • Upwork Job ID: 1682364647495725056
  • Last Price Increase: 2023-07-28
  • Automatic offers:
    • bernhardoj | Contributor | 25910439
    • dhanashree-sawant | Reporter | 25910440
@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jul 20, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 20, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jul 20, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@ginsuma
Copy link
Contributor

ginsuma commented Jul 20, 2023

Proposal

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

After select members, go to invite message page by deeplink, invite again and get "Please select one member".

What is the root cause of that problem?

After we submit, props.invitedEmailsToAccountIDsDraft becomes empty and the text input is blurred => validate function called two times. The first one is for onSubmit, and the second is for onBlur then error message is visible. Because we set props.invitedEmailsToAccountIDsDraft to empty here when submitting.

We can skip navigating after submitting to see it. (Remove this LOC)

When we start to invite users again after using WorkspaceInviteMessagePage deeplink, WorkspaceInvitePage page is placed above in the stack navigator => reuse the WorkspaceInviteMessagePage is mounted already before => the error message still show.

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

Take advantage of withNavigationFocus already used in WorkspaceInviteMessagePage , we only validate when the page is focused.

validate() {
    const errorFields = {};
    if (this.props.isFocused && _.isEmpty(this.props.invitedEmailsToAccountIDsDraft)) {
        errorFields.welcomeMessage = 'workspace.inviteMessage.inviteNoMembersError';
    }
    return errorFields;
}

As my testing, it's enough to works for both web and native apps, but if we want to make sure we can change order for navigating before updating props.invitedEmailsToAccountIDsDraft to empty.

Navigation.navigate(ROUTES.getWorkspaceMembersRoute(this.props.route.params.policyID));
Policy.setWorkspaceInviteMembersDraft(this.props.route.params.policyID, {});

What alternative solutions did you explore? (Optional)

We can use a ref as a flag when submitting to skip validate too. Then we can reset that flag here

componentDidUpdate(prevProps) {
if (!prevProps.isFocused && this.props.isFocused) {
this.focusWelcomeMessageInput();
}

@garrettmknight garrettmknight added the External Added to denote the issue can be worked on by a contributor label Jul 21, 2023
@melvin-bot melvin-bot bot changed the title Web - User is getting error message "Please select at least one member to invite" when invite members again from deeplink [$1000] Web - User is getting error message "Please select at least one member to invite" when invite members again from deeplink Jul 21, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 21, 2023

Job added to Upwork: https://www.upwork.com/jobs/~018e72fd3799cdf925

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 21, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 21, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jul 21, 2023

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

@tienifr
Copy link
Contributor

tienifr commented Jul 21, 2023

Proposal

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

Web - User is getting error message "Please select at least one member to invite" when invite members again from deeplink

What is the root cause of that problem?

When users go to InviteMessagePage page through deeplink and click Invite button

we clear invitedEmailsToAccountIDsDraft

Policy.setWorkspaceInviteMembersDraft(this.props.route.params.policyID, {});

then the input is blurred and onValidate is executed, at that time invitedEmailsToAccountIDsDraft is empty so we set the error. Unfortunately, InviteMessagePage page is not unmounted because this path isn't in the history. So when we go to InviteMessagePage again, the error will appear

App/src/components/Form.js

Lines 297 to 305 in 041d640

onBlur: (event) => {
// We delay the validation in order to prevent Checkbox loss of focus when
// the user are focusing a TextInput and proceeds to toggle a CheckBox in
// web and mobile web platforms.
setTimeout(() => {
setTouchedInput(inputID);
onValidate(inputValues);
}, 200);

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

We should create the new function (isRouteInHistory) in Navigation.js to check the route (in this case it is workspace/<<id>>/members) and check if it's in the history or not by using navigationRef.current.getState()

and update this line

Navigation.navigate(ROUTES.getWorkspaceMembersRoute(this.props.route.params.policyID));

const wsMemberRoute = ROUTES.getWorkspaceMembersRoute(this.props.route.params.policyID);
Navigation.navigate(wsMemberRoute, !Navigation.isRouteInHistory(wsMemberRoute) ? 'UP' : undefined);

When we use type = 'UP' we can make sure the inviteMessage page is unmounted.

Note

If we don't use navigate(route, 'UP') in this case, when users click back button in InviteMember page, they will go to 'InviteMessage' page (it should be Workspace page)

What alternative solutions did you explore? (Optional)

We can add the new parameter to navigate function like shouldNavigateUp

if shouldNavigateUp is true, we'll check if the route isn't in history we will execute linkTo(navigationRef.current, route, 'UP');

Result

Screen.Recording.2023-07-21.at.20.39.39.mov

@bernhardoj
Copy link
Contributor

Proposal

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

Getting an error when going to the invite message page after finishing an invite from a deep link.

What is the root cause of that problem?

To go to the workspace invite message page, the user needs to open Settings > Workspace List > Workspace Detail > Workspace Members > Invite page > Invite message page. Navigating through the page normally will result in the following navigation stack: [Settings, WS List, WS Detail, WS Members, WS Invite, WS Invite message]

In the WS Invite message page, pressing the Invite button will:

  1. Add the member to WS
  2. Clear up the member list draft from Onyx
  3. NAVIGATE to WS Members

sendInvitation() {
Keyboard.dismiss();
Policy.addMembersToWorkspace(this.props.invitedEmailsToAccountIDsDraft, this.state.welcomeNote, this.props.route.params.policyID, this.props.betas);
Policy.setWorkspaceInviteMembersDraft(this.props.route.params.policyID, {});
Navigation.navigate(ROUTES.getWorkspaceMembersRoute(this.props.route.params.policyID));
}

On the web, pressing the Invite button also blurs the text input, which triggers the Form validation. The validation will give an error if the member list draft is empty. As we clear the draft when pressing the Invite button, an error is returned from the validation.

validate() {
const errorFields = {};
if (_.isEmpty(this.props.invitedEmailsToAccountIDsDraft)) {
errorFields.welcomeMessage = 'workspace.inviteMessage.inviteNoMembersError';
}
return errorFields;
}

But this is fine because we already navigated to the WS Members page and the WS Invite message page is popped from the stack, so we won't be able to see the error message. Btw, the navigation stack now looks like this: [Settings, WS List, WS Detail, WS Members]. So, when we navigate again to the WS Invite message page, we will have a fresh page with a fresh Form state.

However, when we directly go to the invite message page (/workspace/{policyID}/invite-message), the navigation stack will be: [WS Invite message], and pressing the Invite button will push WS Members to the stack. On WS Members, we then press the Invite button which will also push it to the stack and now it looks like this: [WS Invite message, WS Members, WS Invite]. Notice that the WS Invite message is still in the stack.

Here is the issue. When we press Next, it will navigate to the existing WS Invite message that is still there on the stack. Because we are just navigating to the existing WS Invite message, the error from the Form validation is still there, thus we can see it.

So, the real issue here is that we should make sure the WS Invite message is popped out after inviting users.

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

Pop the WS invite message page before navigating to WS Members page.

sendInvitation() {
    ...
    Navigation.goBack();
    Navigation.navigate(ROUTES.getWorkspaceMembersRoute(this.props.route.params.policyID));
}

@dhanashree-sawant
Copy link

Hi @garrettmknight, @rushatgabhane I had raised a similar issue 4 days back on slack, link to the issue:
https://expensify.slack.com/archives/C049HHMV9SM/p1689575732247029
If it is similar, can you mention me for reporting as it was earlier then this report?

@ginsuma
Copy link
Contributor

ginsuma commented Jul 22, 2023

Hi @garrettmknight, @rushatgabhane I had raised a similar issue 4 days back on slack, link to the issue: https://expensify.slack.com/archives/C049HHMV9SM/p1689575732247029 If it is similar, can you mention me for reporting as it was earlier then this report?

I have the same thought as giving @dhanashree-sawant reporter role.

@garrettmknight
Copy link
Contributor

@ginsuma @dhanashree-sawant Cool, I'll update the role.

@rushatgabhane either of those proposals look good to you?

@melvin-bot melvin-bot bot removed the Overdue label Jul 24, 2023
@rushatgabhane
Copy link
Member

rushatgabhane commented Jul 25, 2023

Alright, I think @bernhardoj has best explained the root cause. But I'm not sure about the solution.
Calling goBack() in sendInvitation() sounds like a workaround to me.

@rushatgabhane
Copy link
Member

rushatgabhane commented Jul 25, 2023

This might be a simple question. But can someone please help me understand why this issue is occuring?

  1. We've selected members
  2. Go to Invite message page
  3. Open it in new tab
  4. Clicking Invite button will Invite the previously selected members, and clear the draft in Onyx .

Current state of draft => WORKSPACE_INVITE_MEMBERS_DRAFT: {}

sendInvitation() {
Keyboard.dismiss();
Policy.addMembersToWorkspace(this.props.invitedEmailsToAccountIDsDraft, this.state.welcomeNote, this.props.route.params.policyID, this.props.betas);
Policy.setWorkspaceInviteMembersDraft(this.props.route.params.policyID, {});

  1. Select member A & B to invite
  2. Go to Invite message page

Current state of draft => WORKSPACE_INVITE_MEMBERS_DRAFT: {User A, User B}

If the draft isn't empty, why is this bug occuring?

@bernhardoj
Copy link
Contributor

If the draft isn't empty, why is this bug occurring?

That is because, in step 4, the validation runs when we press the Invite button, which throws an error. When we go to the invite message page again (step 6), we simply go back to that page, so we can see the error that is from the prev validation.

Actually, the step can be simpler.

  1. Select members
  2. Go to the Invite message page
  3. Reload the web
  4. Click the Invite button
  5. Click the Back button in the header

Calling goBack() in sendInvitation() sounds like a workaround to me.

Calling goBack simply pop the current screen. We did this in a couple of places where we need to pop the current screen before navigating.

@tienifr
Copy link
Contributor

tienifr commented Jul 26, 2023

Using goBack() seem like a workaround, it can cause the flicker (goBack and then navigate)

The problem here is WS member isn't on the stack when user go to the WS invite message from deeplink. We should check if it's not on the stack then replace the current screen (WS invite message) with WS member screen as what I did in my proposal

@rushatgabhane
Copy link
Member

rushatgabhane commented Jul 26, 2023

When we go to the invite message page again (step 6), we simply go back to that page, so we can see the error that is from the prev validation

Ahh makes sense. So instead of going back to a page, can we Push a new instance of the page and navigate to it?

We use stack navigator. What do you think about this?

| Stack |
| Invite message page | <-- top of stack
| Select members page |

Open the deeplink, and try to invite new members.

| Stack |
| Invite message page | <-- top of stack
| Select members page |
| Invite message page |
| Select members page |

@rushatgabhane
Copy link
Member

rushatgabhane commented Jul 26, 2023

Hmm actually, my suggestion might be flawed because pressing the back button will take you to select members page twice.

@rushatgabhane
Copy link
Member

So another approach could be to replace the old invite message page with a new instance of the page.

@bernhardoj
Copy link
Contributor

When you deep link to the invite message page and follow the suggestion, the stack will be:
| Stack |
| Invite message page | <-- top of stack
| Select members page |
| Invite message page |

Select member is only one, but now we have 2 invite message pages and requires us to introduce a new navigation action to achieve that, which is PUSH.

So another approach could be to replace the old invite message page with a new instance of the page.

This will be too much to achieve. We may need to use RESET action. Also, the stack is still wrong which affects the behavior of the back button.

  1. Deep link to the invite message page
    | Stack |
    | Select members page | <-- top of stack
    | Invite message page |
  2. Press back
  3. The invite message page shows back instead of the workspace page

Instead of thinking to replace it when navigating to it, why not just pop it out when we don't need it anymore, right?

@tienifr
Copy link
Contributor

tienifr commented Jul 28, 2023

@rushatgabhane What do you think about my proposal and the comment?

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Aug 14, 2023
@melvin-bot melvin-bot bot changed the title [$1000] Web - User is getting error message "Please select at least one member to invite" when invite members again from deeplink [HOLD for payment 2023-08-21] [$1000] Web - User is getting error message "Please select at least one member to invite" when invite members again from deeplink Aug 14, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 14, 2023

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

@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Aug 14, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 14, 2023

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

If no regressions arise, payment will be issued on 2023-08-21. 🎊

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.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

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:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@melvin-bot
Copy link

melvin-bot bot commented Aug 14, 2023

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:

  • [@rushatgabhane] The PR that introduced the bug has been identified. Link to the PR:
  • [@rushatgabhane] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@rushatgabhane] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@rushatgabhane] Determine if we should create a regression test for this bug.
  • [@rushatgabhane] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@garrettmknight] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@rushatgabhane
Copy link
Member

rushatgabhane commented Aug 18, 2023

  1. The PR that introduced the bug has been identified. Link to the PR: Implement Invite message page #15672

  2. The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/15672/files#r1315350989

  3. A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: Part of checklist.

  4. Determine if we should create a regression test for this bug. No

  5. If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again - N.A.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 Daily KSv2 labels Aug 21, 2023
@garrettmknight
Copy link
Contributor

Summarizing payments for this issue:

#urgency bonus? No
Reporter: @dhanashree-sawant $250 paid via Upwork
Contributor: @bernhardoj $1000 paid via Upwork
Contributor+: @rushatgabhane $1000

Upwork job is here: https://www.upwork.com/jobs/~018e72fd3799cdf925

@rushatgabhane
Copy link
Member

rushatgabhane commented Aug 22, 2023

i'll complete checklist tomo. I'm just waiting for existing manual requests to be approved so that I can complete all checklists, and request payments in one go

@melvin-bot melvin-bot bot added the Overdue label Aug 25, 2023
@garrettmknight
Copy link
Contributor

@rushatgabhane you get a chance to complete the checklist yet? No rush, just checking.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Aug 25, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 29, 2023

@garrettmknight, @AndrewGable, @rushatgabhane, @bernhardoj Whoops! This issue is 2 days overdue. Let's get this updated quick!

@garrettmknight
Copy link
Contributor

@rushatgabhane bump

@rushatgabhane
Copy link
Member

still waiting on the existing manual requests to be paid 😅

@rushatgabhane
Copy link
Member

Checklist completed - #23311 (comment)

Created a manual request here - https://staging.new.expensify.com/r/796011280744055

@melvin-bot melvin-bot bot removed the Overdue label Sep 5, 2023
@rushatgabhane
Copy link
Member

This issue can be closed if I'm the only one with pending payment because it'll be tracked on new dot
Thanks 🙇

@JmillsExpensify
Copy link

$1,000 payment approved for @rushatgabhane based on BZ summary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

9 participants