[HOLD for payment 2024-10-07] Workspace - Invite message comes back after deleting it

lanitochka17 commented 1 month ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: 9.0.41-2 Reproducible in staging?: Y Reproducible in production?: N If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Email or phone of affected tester (no customers): applausetester+ds@applause.expensifail.com Issue reported by: Applause - Internal Team

Action Performed:

Login to an account
Create a workspace
Go to the created workspace > Members
Click on invite member > Choose a user
On "add message" page try to delete the message

Expected Result:

Message is deleted

Actual Result:

The deleted message comes back

Workaround:

Unknown

Platforms:

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

[ ] Android: Native
[x] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/b04cf507-1cde-4a47-a349-9c0f23c369c2

View all open jobs on GitHub

Issue Owner

Current Issue Owner: @JmillsExpensify

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @dangrous (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

github-actions[bot] commented 1 month ago

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

Identify the pull request that introduced this issue and revert it.
Find someone who can quickly fix the issue.
Fix the issue yourself.

Nodebrute commented 1 month ago

Edited by proposal-police: This proposal was edited at 2024-09-29 18:55:32 UTC.

Proposal

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

Invite message comes back after deleting it

What is the root cause of that problem?

We are setting setWelcomeNote(getDefaultWelcomeNote()) again here https://github.com/Expensify/App/blob/3047c1baa21f47825f5c1ffd652184341dd22256/src/pages/workspace/WorkspaceInviteMessagePage.tsx#L89-L94

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

We can remove this code block https://github.com/Expensify/App/blob/3047c1baa21f47825f5c1ffd652184341dd22256/src/pages/workspace/WorkspaceInviteMessagePage.tsx#L89-L94

We should test the code to make sure everything is working properly

What alternative solutions did you explore? (Optional)

Krishna2323 commented 1 month ago

Edited by proposal-police: This proposal was edited at 2024-09-29 19:17:45 UTC.

Proposal 

Offending PR: https://github.com/Expensify/App/pull/48660

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

Workspace - Invite message comes back after deleting it

What is the root cause of that problem?

We have multiple useEffect hooks to set the welcome note when the component mounts or the members change.

https://github.com/Expensify/App/blob/3047c1baa21f47825f5c1ffd652184341dd22256/src/pages/workspace/WorkspaceInviteMessagePage.tsx#L77-L94

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

We can remove the second useEffect hook here: https://github.com/Expensify/App/blob/3047c1baa21f47825f5c1ffd652184341dd22256/src/pages/workspace/WorkspaceInviteMessagePage.tsx#L89-L94
And in WorkspaceInvitePage.tsx we need to clear the draft invite message when the component unmounts.

https://github.com/Expensify/App/blob/3047c1baa21f47825f5c1ffd652184341dd22256/src/pages/workspace/WorkspaceInvitePage.tsx#L78-L83

TO:

    useEffect(() => {
        return () => {
            Member.setWorkspaceInviteMembersDraft(route.params.policyID, {});
            Policy.setWorkspaceInviteMessageDraft(route.params.policyID, null);
        };
        // eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
    }, [route.params.policyID]);

If we want to clear the draft message when invite members list changes, we can clear the message draft here or here.

What alternative solutions did you explore? (Optional)

Result

Shahidullah-Muffakir commented 1 month ago

Proposal

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

when a user deletes the invite message in the workspace invite process, the message reappears unexpectedly.

What is the root cause of that problem?

The root cause of this problem is that the component was automatically resetting the welcome message to its default value, even when the user had intentionally deleted it

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

we should update the useEffect to add a check that skips resetting the invite message if it is already empty. This ensures that when the user deletes the message, it doesn't get reset to the default value.

Add!welcomeNotecheck here: https://github.com/Expensify/App/blob/3047c1baa21f47825f5c1ffd652184341dd22256/src/pages/workspace/WorkspaceInviteMessagePage.tsx#L89-L92

to:

 useEffect(() => {
        if (isEmptyObject(invitedEmailsToAccountIDsDraft) || !welcomeNote) {
            return;
        }

https://github.com/user-attachments/assets/c1bf77cf-634a-4405-944b-a4aefe147855

huult commented 1 month ago

Proposal

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

Invite message comes back after deleting it

What is the root cause of that problem?

Since we set getDefaultWelcomeNote as a dependency of this hook, the hook is triggered every time the input's onChange event occurs

https://github.com/Expensify/App/blob/36346702f2e2e1621bb2b31d6a7d35cbe1c1d105/src/pages/workspace/WorkspaceInviteMessagePage.tsx#L94

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

We should remove getDefaultWelcomeNote from the dependency array to avoid re-renders when the input changes.

// .src/pages/workspace/WorkspaceInviteMessagePage.tsx#L94
    useEffect(() => {
        if (isEmptyObject(invitedEmailsToAccountIDsDraft)) {
            return;
        }
        setWelcomeNote(getDefaultWelcomeNote());
-    }, [getDefaultWelcomeNote, invitedEmailsToAccountIDsDraft]);
+    }, [invitedEmailsToAccountIDsDraft]);

and We need to validate this input, if necessary, to avoid empty values ("")

// .src/pages/workspace/WorkspaceInviteMessagePage.tsx#L119
        if (isEmptyObject(invitedEmailsToAccountIDsDraft)) {
            errorFields.welcomeMessage = translate('workspace.inviteMessage.inviteNoMembersError');
-        }
+        } else if (isEmptyObject(welcomeNote)) {
+            errorFields.welcomeMessage = 'Please enter a welcome note.';
        }

POC https://github.com/user-attachments/assets/60e6acda-8cf1-4687-97e2-462e2df34f8b

Gonals commented 1 month ago

Seems to be a frontend issue, so removing Web-E blocker

dangrous commented 1 month ago

Yeah this was caused by PR https://github.com/Expensify/App/pull/48660 and issue https://github.com/Expensify/App/issues/34929 . We can just remove that new useEffect but I'm not sure what the point of it was originally, so pinged the PR authors. Thanks!

rojiphil commented 1 month ago

Yeah this was caused by PR #48660 and issue #34929 .

@dangrous Yeah. Fixing an eslint error (i.e. using useOnyx instead of withOnyx) during some last-minute changes caused this issue. I think we can consider this as a separate issue. Please assign me here as I have the context.

We can just remove that new useEffect but I'm not sure what the point of it was originally, so pinged the PR authors. Thanks!

We need the second useEffect to ensure workspaceInviteMessageDraft is populated from the Onyx. However, an edge case got left out i.e. when the user intentionally deletes the entire welcome note in which case we should not reset. @Shahidullah-Muffakir proposal LGTM

dangrous commented 1 month ago

Yep checking for an empty welcome note makes sense to me. Can you raise that PR quick?

rojiphil commented 1 month ago

Yep checking for an empty welcome note makes sense to me. Can you raise that PR quick?

Sure. Working on the PR now. Will update in about an hour

rojiphil commented 1 month ago

@dangrous PR is ready for review.

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

https://github.com/Expensify/App/pull/49955

If no regressions arise, payment will be issued on 2024-10-07. :confetti_ball:

For reference, here are some details about the assignees on this issue:

@rojiphil requires payment (Needs manual offer from BZ)

Krishna2323 commented 1 month ago

@jasperhuangg @rojiphil, sorry for the delay. I believe we still need to clear the message when the RHP closes. If we edit the message, close the RHP, and select members again, the edited message is shown instead of the default message. I have tested this after applying the changes from this PR.

https://github.com/user-attachments/assets/d0b72ecf-4e8c-44da-8264-a2da5dd2d995

jasperhuangg commented 1 month ago

@rojiphil Can you please address that in a followup?

rojiphil commented 1 month ago

I believe we still need to clear the message when the RHP closes. If we edit the message, close the RHP, and select members again, the edited message is shown instead of the default message.

@Krishna2323 The way it was working before was that if the welcome message text was cleared and we navigated away and return to the same page, the default welcome message was displayed. I tested this against a recent ad-hoc build generated before the changes were introduced. And the test video is demonstrated below.

So, I think this is expected and there is no regression here. Setting empty text seems to be the way to get back to the default welcome message.

cc @jasperhuangg

https://github.com/user-attachments/assets/78760cb5-27db-4809-80fb-bf626200e506

dangrous commented 1 month ago

I think we might want to discuss this in Slack? I'm not sure if we have an expected behavior called out for this scenario. Personally, I would probably say:

As long as the RHP stays open, the message should not change on its own (stay edited as edited, or empty as empty, or default as default). This would be true even if you're changing who you're inviting.
Once the RHP is closed, it should reset to the default value.\

Again, this is just my opinion, but is probably what I'd expect? We can ask for some other opinions though.

rojiphil commented 1 month ago

@dangrous Yeah. We can take some other’s opinion too. Meanwhile, I am just a little confused about the following:

• Once the RHP is closed, it should reset to the default value.\

Did you mean removing the draft feature so that it resets to default value always? The reason why I ask is that the draft feature involves saving the draft message for any further member invites done locally. This was implemented here.

Or did you mean resetting to default value only when it is empty? The current behavior though is like this.

melvin-bot[bot] commented 1 month ago

Issue is ready for payment but no BZ is assigned. @JmillsExpensify you are the lucky winner! Please verify the payment summary looks correct and complete the checklist. Thanks!

melvin-bot[bot] commented 1 month ago

Payment Summary

[Upwork Job]()

ROLE: @rojiphil paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@JmillsExpensify)

[ ] I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
[ ] I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
[ ] I have paid out the Upwork contracts or cancelled the ones that are incorrect
[ ] I have verified the payment summary above is correct

JmillsExpensify commented 1 month ago

Payment summary: $250 to @rojiphil for PR resolving this issue.

Shahidullah-Muffakir commented 1 month ago

@JmillsExpensify I wanted to kindly ask if I might also be eligible for compensation, as the solution I proposed in My proposal was implemented in the PR. Even @rojiphil acknowledged and approved my proposal in here. Thank you.

dangrous commented 1 month ago

Oh got it! I wasn't aware of that specific behavior - so I take back what I said haha. I guess then I think it should just always stay as edited, no matter what.

But that does bring up the question of how to return to the default message, since that seems like a feature we might want.

Maybe:

As long as the RHP stays open, the message should not change on its own (stay edited as edited, or empty as empty, or default as default). This would be true even if you're changing who you're inviting.
Once the RHP is closed and the field is empty, it should reset to the default value.

What do we think? Alternate (more involved) solution would be adding a Reset to default or similar button

melvin-bot[bot] commented 1 month ago

@JmillsExpensify, @rojiphil, @dangrous Whoops! This issue is 2 days overdue. Let's get this updated quick!

dangrous commented 1 month ago

bump on this - do we think that's the right way forward?

rojiphil commented 1 month ago

I wanted to kindly ask if I might also be eligible for compensation, as the solution I proposed in My proposal was implemented in the PR. Even @rojiphil acknowledged and approved my proposal in here. Thank you.

It’s correct that the mentioned proposal was implemented here to fix the issue. I would be happy to split the compensation and share a part with @Shahidullah-Muffakir.

rojiphil commented 1 month ago

Maybe:

As long as the RHP stays open, the message should not change on its own (stay edited as edited, or empty as empty, or default as default). This would be true even if you're changing who you're inviting.

Once the RHP is closed and the field is empty, it should reset to the default value.

What do we think? Alternate (more involved) solution would be adding a Reset to default or similar button

So, that brings before us the following options:

As long as the RHP stays open, keep the message as empty when it is empty. Reset to default value only once the RHP is closed and the message is empty.
When set to empty, always keep the message as empty. And use a Reset to default or similar button to reset to default value.
Do nothing. i.e. Reset to default value whenever the invite message page is shown and the message is empty. This is irrespective of whether the RHP stays open or closed.

I personally like Option 2 but let’s see what’s the consensus would look like here cc @JmillsExpensify @Expensify/design

dangrous commented 1 month ago

bumped design in Slack to see if we can figure something out!

shawnborton commented 1 month ago

Can someone share a video of the problem at hand or maybe a more detailed description of the full problem that needs to be resolved?

dannymcclain commented 1 month ago

Yeah a video would be super helpful. It also might be worth looking into how Status works—feels like it might be kinda similar.

rojiphil commented 1 month ago

Can someone share a video of the problem at hand or maybe a more detailed description of the full problem that needs to be resolved?

Let me try to explain with a video and see if it helps.

The test video attached here demonstrates the current behaviour i.e. option 3. Here, after clearing the default message, when we navigate back to Invite new members view and again navigate to Add message view, it again displays the default message.

Now, the thought is that since the user has set the message as empty, it should remain empty. The approach in option 1 would be to keep the message empty itself until the RHP is closed (i.e. user closes the Invite new members page). If RHP is closed and we, again, navigate to Add message, we can show the default message.

Option 2 proposes to keep the message edited as empty always irrespective of whether the RHP is open or closed. And then rely on Reset to default or similar button to reset to default value.

https://github.com/user-attachments/assets/a847c2c9-d034-48ef-b59f-6fac2bf299fb

shawnborton commented 1 month ago

Hmm I don't feel too strongly here. I can see where it makes sense that if you get to the "Add message" screen and make edits, those edits should remain regardless if you navigate back to the invite page or not. But I would say we should only save a new default message once the actual invite gets sent.

dannymcclain commented 1 month ago

But I would say we should only save a new default message once the actual invite gets sent.

I agree with this. The behavior you're showing in the video doesn't seem out of place to me. I tested it in the product and if you do actually write something in the field, we persist that edit if you navigate back to the participant selector and then move forward again. So generally that feels pretty right to me.

The only thing that feels weird is if I customize the message and then completely abandon the flow (close the RHP without sending an invite) the custom message persists if I go through the invite flow again from scratch. That feels a little weird to me that we're saving a custom message that never got sent.

shawnborton commented 1 month ago

That feels a little weird to me that we're saving a custom message that never got sent.

Yeah, I definitely agree with that part.

dangrous commented 1 month ago

Okay so it sounds like we might be going for an option 1.5:

1.5. As long as the RHP stays open, keep the message as empty when it is empty. Reset to default value only once the RHP is closed without an invite being sent ~and the message is empty~.

Am I interpreting that correctly?

dannymcclain commented 1 month ago

Yes I think that makes the most sense to me.

dubielzyk-expensify commented 1 month ago

Catching up on it all and yeah that feels right to me too

dangrous commented 1 month ago

Awesome, thank you! @rojiphil, are you up for tackling that adjustment, or should we open it up to contributors again?

rojiphil commented 1 month ago

But I would say we should only save a new default message once the actual invite gets sent.

1.5. As long as the RHP stays open, keep the message as empty when it is empty. Reset to default value only once the RHP is closed without an invite being sent ~and the message is empty~.

Hmm.. I am slightly confused here. Are we saying the following? Or am I missing something here?

As long as the RHP stays open, keep the messages as edited (even if it is set to empty). And save the edited message as invite message draft only on an actual invite. Once the RHP closes without an actual invitation, discard the edited message. And when the RHP is shown again, show either the previously saved invite message draft or show the default message if no custom message is set.

dannymcclain commented 1 month ago

I think you've outlined it exactly how I was understanding it. 👍

rojiphil commented 1 month ago

are you up for tackling that adjustment, or should we open it up to contributors again?

@dangrous I think it’s best to open it up to contributors again to get the best proposal as this does not seem like a minor adjustment.

dangrous commented 1 month ago

Okay cool! I may actually just make a new issue for it, so we can close this one out / pay. It's more of a clarification on expected behavior than a regression or anything that should be handled here.

I'm also going to hold it on https://github.com/Expensify/App/issues/49996 which is related and should make things easier.

dangrous commented 4 weeks ago

Made https://github.com/Expensify/App/issues/51096!

rojiphil commented 4 weeks ago

Okay cool! I may actually just make a new issue for it, so we can close this one out / pay.

Yeah. Since another issue is created for the enhancement, I think we can pay as per this comment and close this one out. cc @JmillsExpensify

JmillsExpensify commented 3 weeks ago

Ok, so based on that comment, we'd have:

Contributor: $125 @rojiphil via Upwork
Contributor: $125 @Shahidullah-Muffakir via Upwork

JmillsExpensify commented 3 weeks ago

Offers sent to both via Upwork.

Shahidullah-Muffakir commented 3 weeks ago

Offers sent to both via Upwork.

Accepted, Thank you.

rojiphil commented 3 weeks ago

Offers sent to both via Upwork.

Accepted the offer. Thanks

JmillsExpensify commented 3 weeks ago

Both paid out. Thank you!

Expensify / App