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-01-23] [$4000] Workspace - The welcome message does not appear immediately after deleting the workspace #13615

Closed
kbecciv opened this issue Dec 15, 2022 · 76 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 Dec 15, 2022

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. Go to URL https://staging.new.expensify.com/
  2. Login with any account
  3. Go to Settings -> Workspace (you should NOT have any workspaces - delete all workspaces if you do)
  4. Click "New Workspace" for creating the workspace
  5. In the newly created workspace click on 3 dots
  6. Click on "Delete workspace" -> "Delete"

Expected Result:

The welcome message appears immediately

Actual Result:

The welcome message does not appear immediately - it blinks

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number: 1.2.39.0

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug5864228_Recording__3280.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~010de9dc97b9a28579
  • Upwork Job ID: 1603567405855707136
  • Last Price Increase: 2022-12-30
@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Dec 15, 2022
@melvin-bot melvin-bot bot locked and limited conversation to collaborators Dec 15, 2022
@Christinadobrzyn
Copy link
Contributor

  • I am able to replicate this by following the steps above.
  • No other GHs matching this issue exist
  • I think this can be external

@Christinadobrzyn Christinadobrzyn added the External Added to denote the issue can be worked on by a contributor label Dec 16, 2022
@melvin-bot melvin-bot bot unlocked this conversation Dec 16, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 16, 2022

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

@melvin-bot melvin-bot bot changed the title Workspace - The welcome message does not appear immediately after deleting the workspace [$1000] Workspace - The welcome message does not appear immediately after deleting the workspace Dec 16, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 16, 2022

Job added to Upwork: https://www.upwork.com/jobs/~010de9dc97b9a28579

@melvin-bot
Copy link

melvin-bot bot commented Dec 16, 2022

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 16, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 16, 2022

Triggered auto assignment to @srikarparsi (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@bernhardoj
Copy link
Contributor

bernhardoj commented Dec 16, 2022

Proposal

When we delete a workspace, we set the optimistic value with pendingAction of delete. When we navigated back to the workspace list page, the list still has one workspace left with delete pendingAction. If we look at the implementation of OfflineWithFeedback, if the pendingAction is delete, we will hide it.

const OfflineWithFeedback = (props) => {
const hasErrors = !_.isEmpty(props.errors);
const isOfflinePendingAction = props.network.isOffline && props.pendingAction;
const isUpdateOrDeleteError = hasErrors && (props.pendingAction === 'delete' || props.pendingAction === 'update');
const isAddError = hasErrors && props.pendingAction === 'add';
const needsOpacity = (isOfflinePendingAction && !isUpdateOrDeleteError) || isAddError;
const needsStrikeThrough = props.network.isOffline && props.pendingAction === 'delete';
const hideChildren = !props.network.isOffline && props.pendingAction === 'delete' && !hasErrors;
let children = props.children;
// Apply strikethrough to children if needed, but skip it if we are not going to render them
if (needsStrikeThrough && !hideChildren) {
children = applyStrikeThrough(children);
}
return (
<View style={props.style}>
{!hideChildren && (
<View style={[needsOpacity ? styles.offlineFeedback.pending : {}, props.contentContainerStyle]}>
{children}
</View>
)}

And after a while, we will receive a Pusher API update that will set the workspace item value to null.
[{"key": "policy_142AC62FE7444C5B", "onyxMethod": "set", "value": null}]

What we can do here is to change the condition to show the empty workspace view. Filter out all workspace with pendingAction of delete.

render() {
    const workspaces = this.getWorkspaces();
+   const isWorkspacesEmpty = _.isEmpty(_.filter(workspaces, workspace => workspace.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE));
    return (
        <ScreenWrapper>
            <HeaderWithCloseButton
                title={this.props.translate('common.workspaces')}
                shouldShowBackButton
                onBackButtonPress={() => Navigation.navigate(ROUTES.SETTINGS)}
                onCloseButtonPress={() => Navigation.dismissModal(true)}
            />
+           {isWorkspacesEmpty ? (
-           {_.isEmpty(workspaces) ? (
                <BlockingView
                    icon={Expensicons.Building}
                    title={this.props.translate('workspace.emptyWorkspace.title')}
                    subtitle={this.props.translate('workspace.emptyWorkspace.subtitle')}
                />
            ) : (
                <ScrollView style={styles.flex1}>
                    {_.map(workspaces, (item, index) => this.getMenuItem(item, index))}
                </ScrollView>
            )}
            <FixedFooter style={[styles.flexGrow0]}>
                <Button
                    success
                    text={this.props.translate('workspace.new.newWorkspace')}
                    onPress={() => Policy.createWorkspace()}
                />
            </FixedFooter>
        </ScreenWrapper>
    );
}

Result

328546.t.mp4

@melvin-bot
Copy link

melvin-bot bot commented Dec 16, 2022

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

@tienifr
Copy link
Contributor

tienifr commented Dec 16, 2022

Proposal

Problem

When we delete the workspace we just update the pendingAction to delete and remove this workspace when API call is successful.
In https://github.com/Expensify/App/blob/main/src/pages/workspace/WorkspacesListPage.js#L118

we get all the policies and filter by policy && policy.type === CONST.POLICY.TYPE.FREE && policy.role === CONST.POLICY.ROLE.ADMIN => the deleted workspace still be there until the API call is successful.

Solution

we shouldn't filter out the workspace that has pendingAction === 'delete' when we're in offline

I'll filter out the workspace that has pendingAction === 'delete' when we're in online.

In https://github.com/Expensify/App/blob/main/src/pages/workspace/WorkspacesListPage.js

...
    getWorkspaces() {
        return _.chain(this.props.policies)
            .filter(policy => policy && policy.type === CONST.POLICY.TYPE.FREE && policy.role === CONST.POLICY.ROLE.ADMIN)
+           .filter(policy => this.props.network.isOffline || policy.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE)

...

export default compose(
    withLocalize,
+   withNetwork(),
...
Screen.Recording.2022-12-16.at.10.23.23.mp4

@s77rt
Copy link
Contributor

s77rt commented Dec 16, 2022

Proposal

diff --git a/src/pages/workspace/WorkspacesListPage.js b/src/pages/workspace/WorkspacesListPage.js
index 41c6b3babe..2c037c685d 100755
--- a/src/pages/workspace/WorkspacesListPage.js
+++ b/src/pages/workspace/WorkspacesListPage.js
@@ -115,7 +115,10 @@ class WorkspacesListPage extends Component {
      */
     getWorkspaces() {
         return _.chain(this.props.policies)
-            .filter(policy => policy && policy.type === CONST.POLICY.TYPE.FREE && policy.role === CONST.POLICY.ROLE.ADMIN)
+            .filter(policy => policy
+                && policy.type === CONST.POLICY.TYPE.FREE
+                && policy.role === CONST.POLICY.ROLE.ADMIN
+                && policy.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE)
             .map(policy => ({
                 title: policy.name,
                 icon: policy.avatar ? policy.avatar : Expensicons.Building,
@@ -128,7 +131,6 @@ class WorkspacesListPage extends Component {
                 pendingAction: policy.pendingAction,
                 errors: policy.errors,
                 dismissError: () => dismissWorkspaceError(policy.id, policy.pendingAction),
-                disabled: policy.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE,
             }))
             .sortBy(policy => policy.title)
             .value();
@@ -164,7 +166,6 @@ class WorkspacesListPage extends Component {
                     badgeText={this.getWalletBalance(isPaymentItem)}
                     fallbackIcon={item.fallbackIcon}
                     brickRoadIndicator={item.brickRoadIndicator}
-                    disabled={item.disabled}
                 />
             </OfflineWithFeedback>
         );

RCA

I agree with the above @bernhardoj @tienifr analysis

Solution

Filter out workspaces that have pendingAction equal to CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE
Remove the unused disabled prop

Alternative solutions

  1. Add a new prop OfflineWithFeedback to decide whether children should be hidden

@melvin-bot melvin-bot bot added the Overdue label Dec 19, 2022
@parasharrajat
Copy link
Member

Reviewing.

@melvin-bot melvin-bot bot removed the Overdue label Dec 19, 2022
@Christinadobrzyn
Copy link
Contributor

I'll be ooo till Jan 9th, assigning this issue to @mallenexpensify. Matt, I'm happy to take it back when I return!

@syedsaroshfarrukhdot
Copy link
Contributor

syedsaroshfarrukhdot commented Dec 21, 2022

Proposal :-

With all above proposals when we filter list on policy.pendingAction it causes a regression as shown in below video. The pendingAction changes from delete to undefined which causes deleted workspace to reappear in the list for a split second.

Regression Video :-

Screen.Recording.2022-12-21.at.8.49.38.AM.mov

As pendingAction : undefined is set before removing it from the list. We can fix it in Policy.js deleteWorkspace function by passing a new value lastAction in optimisticData which isn't modified and filter list on it.

diff --git a/src/libs/actions/Policy.js b/src/libs/actions/Policy.js
index 0f6784e7d..2bb5117eb 100644
--- a/src/libs/actions/Policy.js
+++ b/src/libs/actions/Policy.js
@@ -68,6 +68,7 @@ function deleteWorkspace(policyID, reports) {
             value: {
                 pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE,
                 errors: null,
+                lastAction : CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE,
             },
         },
         ..._.map(reports, ({reportID}) => ({
@@ -82,6 +83,7 @@ function deleteWorkspace(policyID, reports) {
         })),
     ];
 
+
     // Restore the old report stateNum and statusNum
     const failureData = [
         ..._.map(reports, ({


diff --git a/src/pages/workspace/WorkspacesListPage.js b/src/pages/workspace/WorkspacesListPage.js
index 41c6b3bab..a79fce73d 100755
--- a/src/pages/workspace/WorkspacesListPage.js
+++ b/src/pages/workspace/WorkspacesListPage.js
@@ -114,8 +114,9 @@ class WorkspacesListPage extends Component {
      * @returns {Array} the menu item list
      */
     getWorkspaces() {
+        
         return _.chain(this.props.policies)
-            .filter(policy => policy && policy.type === CONST.POLICY.TYPE.FREE && policy.role === CONST.POLICY.ROLE.ADMIN
)
+            .filter(policy => policy && policy.type === CONST.POLICY.TYPE.FREE && policy.role === CONST.POLICY.ROLE.ADMIN && policy.lastAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE)
             .map(policy => ({
                 title: policy.name,
                 icon: policy.avatar ? policy.avatar : Expensicons.Building,
@@ -172,6 +173,7 @@ class WorkspacesListPage extends Component {
 
     render() {
         const workspaces = this.getWorkspaces();

         return (
             <ScreenWrapper>
                 <HeaderWithCloseButton

After Fix :-

Screen.Recording.2022-12-21.at.8.52.24.AM.mov

We can also not filter list when we are offline if we want to

...
    getWorkspaces() {
        return _.chain(this.props.policies)
            .filter(policy => policy && policy.type === CONST.POLICY.TYPE.FREE && policy.role === CONST.POLICY.ROLE.ADMIN)
+           .filter(policy => this.props.network.isOffline || policy.lastAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE)

...

export default compose(
    withLocalize,
+   withNetwork(),
...

After Offline Filtering Fix :-

Screen.Recording.2022-12-21.at.9.05.25.AM.mov

@s77rt
Copy link
Contributor

s77rt commented Dec 21, 2022

@syedsaroshfarrukhdot Nice catch, but this is not a regression from my proposal, this issue existed long before. Your solution does not address the root cause for that either.

The workspace will appear again for a short period only if you create the workspace and delete it immediately (in very short time), here is the order of events:

  1. Create workspace button is clicked
  2. optimisticData sets the pendingAction to add
  3. Create workspace request is still going...
  4. Delete workspace
  5. optimisticData sets the pendingAction to delete <-- Workspace no longer visible
  6. Delete workspace request is still going...
  7. Create workspace request is done
  8. successData sets the pendingAction to null <-- Workspace is visible again (root cause)
  9. Delete workspace request is done
  10. A pusher notification will be received where it sets the workspace to null <-- Workspace no longer exists

As you see this is an edge case and we shouldn't prioritise it, besides the final results is the workspace being deleted and not visible (self fix)


I think we still should go with my proposal, but just curious why you are adding the network.isOffline check?

@syedsaroshfarrukhdot
Copy link
Contributor

@s77rt Yes, I agree with your analysis it is an edge case and it can be handled on filtering Workspaces on a prop which doesn't change just like in my proposal. So I believe once we are taking care of it it should be fixed along with edge case rest is up to team on what they decide how to handle it.

I am using network.isOffline to not filter out deleted workspace when in offline mode just like in the proposal video. When a workspace is deleted while offline it shouldn't be filtered unless user becomes online and actually the workspaces I deleted.

@s77rt
Copy link
Contributor

s77rt commented Dec 22, 2022

@syedsaroshfarrukhdot Thanks for the clarification, however I still think we should go with my proposal (without adding a new prop) as that's how we deal with similar cases https://github.com/Expensify/App/blob/main/src/pages/settings/InitialSettingsPage.js#L130-L137

As for the offline case, your explanation makes sense, I will update my proposal to cover the offline cases in few minutes in both InitialSettingsPage and WorkspacesListPage

Another question: Why you are using two filters calls? it's expensive to call functions in JS and iterate over the objects twice, just wrap them in one filter for future proposals.

@s77rt
Copy link
Contributor

s77rt commented Dec 22, 2022

Proposal (Updated)

diff --git a/src/pages/settings/InitialSettingsPage.js b/src/pages/settings/InitialSettingsPage.js
index c2bf6b384b..18d1067f31 100755
--- a/src/pages/settings/InitialSettingsPage.js
+++ b/src/pages/settings/InitialSettingsPage.js
@@ -131,7 +131,7 @@ class InitialSettingsPage extends React.Component {
             .filter(policy => policy
                 && policy.type === CONST.POLICY.TYPE.FREE
                 && policy.role === CONST.POLICY.ROLE.ADMIN
-                && policy.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE)
+                && (this.props.network.isOffline || policy.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE))
             .sortBy(policy => policy.name)
             .pluck('avatar')
             .value();
diff --git a/src/pages/workspace/WorkspacesListPage.js b/src/pages/workspace/WorkspacesListPage.js
index 41c6b3babe..5fc2446513 100755
--- a/src/pages/workspace/WorkspacesListPage.js
+++ b/src/pages/workspace/WorkspacesListPage.js
@@ -23,6 +23,7 @@ import Permissions from '../../libs/Permissions';
 import Button from '../../components/Button';
 import FixedFooter from '../../components/FixedFooter';
 import BlockingView from '../../components/BlockingViews/BlockingView';
+import {withNetwork} from '../../components/OnyxProvider';
 
 const propTypes = {
     /* Onyx Props */
@@ -115,7 +116,10 @@ class WorkspacesListPage extends Component {
      */
     getWorkspaces() {
         return _.chain(this.props.policies)
-            .filter(policy => policy && policy.type === CONST.POLICY.TYPE.FREE && policy.role === CONST.POLICY.ROLE.ADMIN)
+            .filter(policy => policy
+                && policy.type === CONST.POLICY.TYPE.FREE
+                && policy.role === CONST.POLICY.ROLE.ADMIN
+                && (this.props.network.isOffline || policy.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE))
             .map(policy => ({
                 title: policy.name,
                 icon: policy.avatar ? policy.avatar : Expensicons.Building,
@@ -222,4 +226,5 @@ export default compose(
             key: ONYXKEYS.BETAS,
         },
     }),
+    withNetwork(),
 )(WorkspacesListPage);

Update

  • Cover offline cases in InitialSettingsPage and WorkspacesListPage
Kooha-2022-12-22-13-39-20.mp4

@syedsaroshfarrukhdot
Copy link
Contributor

Another question: Why you are using two filters calls? it's expensive to call functions in JS and iterate over the objects twice, just wrap them in one filter for future proposals.

Agree with you will also post an updated proposal and will use one filter call to avoid it.

@mallenexpensify mallenexpensify added Daily KSv2 and removed Weekly KSv2 Reviewing Has a PR in review labels Jan 17, 2023
@NikkiWines NikkiWines mentioned this issue Jan 18, 2023
91 tasks
@NikkiWines
Copy link
Contributor

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:

For this, I don't really think there was a regression or we could've caught this earlier. This behavior was introduced back when we originally added workspaces into newDot. We have solid documentation on ensuring actions are done optimistically, so I think that should be sufficient in this case.

@mallenexpensify
Copy link
Contributor

I'd just check the box then @NikkiWines , I'm asking about the steps/process for updating TestRail here
https://expensify.slack.com/archives/C9YU7BX5M/p1674091251918159

@jatinsonijs
Copy link
Contributor

@mallenexpensify can you plz look into this, regression period is over.

@mallenexpensify
Copy link
Contributor

Thanks for the ping @jatinsonijs , sorry for the delay (I'm looking into why this was labeled Overdue)
You're paid $6000, inc. the 50% timeliness bonus.
@parasharrajat can you please accept the job and reply here once you have?
#13615 (comment)

@mallenexpensify
Copy link
Contributor

@jatinsonijs I'm working on updating the regression steps and I see the final test step listed in your PR as

Verify welcome message (empty list message) appear immediately.

Yet, when I delete a workspace I:

  • If the main chat window isn't #announce for the workspace, nothing happens
  • If the main chat window is the #announce room it shows as archived.

So I'm not seeing a 'welcome message'. Am I missing something or are the two steps above the expected behavior?

@jatinsonijs
Copy link
Contributor

jatinsonijs commented Jan 25, 2023

@mallenexpensify it is about this message

Screenshot 2023-01-26 at 12 16 34 AM

@jatinsonijs
Copy link
Contributor

jatinsonijs commented Jan 25, 2023

I refer it as welcome message because in the issue title it is called as welcome message.

@mallenexpensify
Copy link
Contributor

Thanks @jatinsonijs, I was testing on Desktop. Was able to reproduce to get the screen above.

@mallenexpensify
Copy link
Contributor

Checking with QA about test step updates https://expensify.slack.com/archives/C9YU7BX5M/p1674676068594109

@mallenexpensify
Copy link
Contributor

@melvin-bot melvin-bot bot added the Overdue label Jan 30, 2023
@parasharrajat
Copy link
Member

@mallenexpensify I have already applied for the job. Waiting for the offer, I guess.

@melvin-bot melvin-bot bot removed the Overdue label Jan 30, 2023
@mallenexpensify
Copy link
Contributor

Hired @parasharrajat , can you please accept the job and reply here once you have?
Total is $6K inc. timeliness bonus

@parasharrajat
Copy link
Member

Done @mallenexpensify

@mallenexpensify
Copy link
Contributor

Paid $6K to @parasharrajat for +C, includes timeliness budget.

Triple checking in #qa about steps

@mallenexpensify
Copy link
Contributor

Gonna propose these.
https://expensify.testrail.io/index.php?/cases/view/1971214
Update step 8 and add step 9
8. Desktop - Verify the main chat window is the #announce room it shows as archived.
9 Mobile - Verify you're taken to the "Create a new workspace" window.

https://expensify.slack.com/archives/C01SKUP7QR0/p1675268410262409

@mallenexpensify
Copy link
Contributor

https://github.com/Expensify/Expensify/issues/259653
TestRail steps proposed, assigned myself to ensure they're updated. Checklist updated 👋

@mallenexpensify
Copy link
Contributor

@parasharrajat , reviewing this issue. It appears your reviews took a while and the priced jumped from $1k to $4k before you reviewed the accepted proposal from @jatinsonijs. I think part (at least) of the reason was due to IP-related issues around this time. Please try to review proposals as promptly as possible to reduce the chance of prices unnecessarily doubling.

12/23 - @jatinsonijs proposal
12/29 - no reviews, comment from you - "Checking the issue now."
Jan 1 - issues raised to $4000 before proposal review.

@parasharrajat
Copy link
Member

parasharrajat commented Feb 10, 2023

Sure, I am fully active at this time. I was partly available at that time. Also, we were facing a few blockers.

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