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-03] [HOLD for payment 2023-08-02] Workspace - System throws an error when refreshing 'Invite new members' page immediately after creating a workspace #23536

Closed
2 of 6 tasks
kbecciv opened this issue Jul 25, 2023 · 28 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering Weekly KSv2

Comments

@kbecciv
Copy link

kbecciv commented Jul 25, 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. Click on 'Profile' avatar
  2. Click on 'Workspaces' link
  3. Open an existing workspace
  4. Click on 'Members' link
  5. Click on 'Invite' button
  6. Refresh the page

Expected Result:

No error should be thrown & 'Invite new members' page should be displayed

Actual Result:

'Invite new members' page is closed * system throws an error & user can't clear the error by clicking the close icon

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.45-1
Reproducible in staging?: y
Reproducible in production?: n
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

2023-07-25.10.55.08.mp4
Recording.3887.mp4

Expensify/Expensify Issue URL:
Issue reported by: @Natnael-Guchima
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690272138001129

View all open jobs on GitHub

@kbecciv kbecciv added the DeployBlockerCash This issue or pull request should block deployment label Jul 25, 2023
@OSBotify
Copy link
Contributor

👋 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:

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

@melvin-bot
Copy link

melvin-bot bot commented Jul 25, 2023

Triggered auto assignment to @flodnv (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@flodnv
Copy link
Contributor

flodnv commented Jul 25, 2023

@Natnael-Guchima
Copy link

I am able to reproduce it @flodnv. To reproduce it, I think, the following condition must hold.

  1. Create a new workspace
  2. Navigate to the 'Invite new members' page immediately after creating the workspace
  3. Refresh the page

Let me know if it not working with these steps😃

2023-07-25.15.24.44.mp4

@flodnv
Copy link
Contributor

flodnv commented Jul 25, 2023

Ah yes, I was able to reproduce now

@flodnv
Copy link
Contributor

flodnv commented Jul 25, 2023

I'm trying to use git bisect to find the offending commit and it's taking a long while due to npm i problems.

@flodnv
Copy link
Contributor

flodnv commented Jul 25, 2023

git bisect shows this commit caused the bug: 46595c3

Indeed, with this diff on main:

diff --cc src/libs/actions/PersistedRequests.js
index c99ed2ace7,f2bafb0d13..0000000000
--- a/src/libs/actions/PersistedRequests.js
+++ b/src/libs/actions/PersistedRequests.js
@@@ -25,16 -25,14 +25,9 @@@ function save(requestsToPersist) 
   * @param {Object} requestToRemove
   */
  function remove(requestToRemove) {
-     /**
-      * We only remove the first matching request because the order of requests matters.
-      * If we were to remove all matching requests, we can end up with a final state that is different than what the user intended.
-      */
-     const index = _.findIndex(persistedRequests, (persistedRequest) => _.isEqual(persistedRequest, requestToRemove));
 -    const persistedRequestsCopy = _.clone(persistedRequests);
 -    const index = _.findIndex(persistedRequestsCopy, (persistedRequest) => _.isEqual(persistedRequest, requestToRemove));
--    if (index !== -1) {
-         persistedRequests.splice(index, 1);
 -        persistedRequestsCopy.splice(index, 1);
 -        persistedRequests = persistedRequestsCopy;
--    }
 -
 -    Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequestsCopy);
++    persistedRequests = _.reject(persistedRequests, (persistedRequest) => _.isEqual(persistedRequest, requestToRemove));
 +
 +    Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests);
  }
  
  /**

The bug is gone. Should we revert that PR @puneetlath @allroundexperts @namhihi237 ?

@allroundexperts
Copy link
Contributor

I'm not sure how this PR is related. @namhihi237 Can you please verify?

@namhihi237
Copy link
Contributor

Yes, I just check, I think the issue is relative to my PR. Somehow we have 2 same requests creating WS

@allroundexperts
Copy link
Contributor

ove(requestToRemove) {

Hm... Thats weird. Why are we making two requests to create a workspace?

@puneetlath
Copy link
Contributor

Looks like it caused another deploy blocker here: #23542 (comment)

@allroundexperts
Copy link
Contributor

Let's revert this I guess until we find the root cause here.

@puneetlath
Copy link
Contributor

Here's a PR to revert: #23551

@allroundexperts can you do the reviewer checklist?

@allroundexperts
Copy link
Contributor

Here's a PR to revert: #23551

@allroundexperts can you do the reviewer checklist?

Sure.

@melvin-bot
Copy link

melvin-bot bot commented Jul 25, 2023

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Hourly KSv2 labels Jul 25, 2023
@allroundexperts
Copy link
Contributor

PR has been reverted. @namhihi237 I think the direct mutation of the persistedRequests caused this issue. Please confirm and raise a PR keeping this in mind.

@namhihi237
Copy link
Contributor

Yes, that's cause. I will raise PR use deep clone.

@allroundexperts
Copy link
Contributor

Yes, that's cause. I will raise PR use deep clone.

I think shallow clone would work equally well. Something like this:

const requests = [...persistedRequests];
    const index = _.findIndex(requests, (persistedRequest) => _.isEqual(persistedRequest, requestToRemove));
    if (index !== -1) {
        requests.splice(index, 1);
    }

    persistedRequests = requests;

    Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, requests);

@namhihi237
Copy link
Contributor

Yes, that work.

@flodnv
Copy link
Contributor

flodnv commented Jul 25, 2023

Thanks all! The bug is gone from staging so we can close this.

@flodnv flodnv closed this as completed Jul 25, 2023
@Natnael-Guchima
Copy link

@flodnv I reported this issue. And no reporting bonus is issued 🙃.

@puneetlath puneetlath reopened this Jul 25, 2023
@puneetlath
Copy link
Contributor

@Natnael-Guchima
Copy link

Accepted the offer. Thanks @puneetlath

@flodnv
Copy link
Contributor

flodnv commented Jul 25, 2023

My bad @Natnael-Guchima, thanks @puneetlath

@flodnv flodnv removed the DeployBlockerCash This issue or pull request should block deployment label Jul 25, 2023
@puneetlath
Copy link
Contributor

Paid!

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jul 26, 2023
@melvin-bot melvin-bot bot changed the title Workspace - System throws an error when refreshing 'Invite new members' page immediately after creating a workspace [HOLD for payment 2023-08-02] Workspace - System throws an error when refreshing 'Invite new members' page immediately after creating a workspace Jul 26, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 26, 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 Jul 26, 2023
@melvin-bot

This comment was marked as duplicate.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Jul 27, 2023
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2023-08-02] Workspace - System throws an error when refreshing 'Invite new members' page immediately after creating a workspace [HOLD for payment 2023-08-03] [HOLD for payment 2023-08-02] Workspace - System throws an error when refreshing 'Invite new members' page immediately after creating a workspace Jul 27, 2023
@melvin-bot

This comment was marked as off-topic.

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 Engineering Weekly KSv2
Projects
None yet
Development

No branches or pull requests

7 participants