[Hold for payment 2021-08-17] Handle redirects to Invalid PolicyIds in WorkspaceCard and WorkspacePeople screens

[mananjadhav]

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, enter the workspace URL without any policyID
2. It updates the URL as workspace/undefined/card
3. Blank page with the card is shown
4. Same gets reproduced by putting any random workspace policyId in the URL workspace/<randomId>/card, workspace/<randomId>/people

Expected Result:

1. Should've redirected to the Home page
2. Or should've opened the Settings Page with a Not Found design

redirect-to-home.mp4

Actual Result:

Empty Sidebar + List with WorkspacePeople Page
https://user-images.githubusercontent.com/3069065/127743007-cb235857-daaa-402f-bc1a-28f76e36b7fc.mp4

Empty Sidebar with WorkspaceCard Page
https://user-images.githubusercontent.com/3069065/127743012-c6cc58ae-50cf-4b0e-bfa1-ebaad0f9a5d3.mp4

Workaround:

Yes. No blocker impact.

Platform:

Where is this issue occurring?

• Web
• Mobile Web

Version Number:
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:

View all open jobs on Upwork

[MelvinBot]

Triggered auto assignment to @bfitzexpensify (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[mananjadhav]

Proposal

There are two approaches that we have:

1. Redirect to the Home Chat screen
2. Create a default error page for Workspace and load the Settings menu to let the user choose workspace from

Currently, WorkspaceCardPage doesn't take policy as one of the props. WorkspacePeoplePage already has policy page.

Common Code in both approaches

WorkspaceCardPage

Add props:

const propTypes = {
    ..., // existing values
    policy: PropTypes.shape({
        /** The policy name */
        name: PropTypes.string,
    }),
}

const defaultProps = {
    ..., // existing values
    policy: {
      name: '',
    }
};

function isValidPolicy(policy) {  // will be added to WorkspacePeoplePage or moved to some common utils.
  if(!policy || !policy.name || !policy.name.length) {
    return false;
  }
  return true;
}

Approach 1

const WorkspaceCardPage = ({ // and the same block for WorkspacePeoplePage
    betas,
    user,
    translate,
    isSmallScreenWidth,
    reimbursementAccount,
    policy
}) => {

  useEffect(() => {
    if(!isValidPolicy(policy)) {
      Navigation.navigate(ROUTES.REPORT);
      return;
    }
  }, []);

  .... //Rest of the block

Approach 2

Create a new page WorkspaceNotFound page.// Will need help with the design

const WorkspaceNotFound = ({translations: {translate}}) => {

    useEffect(() => {
       if(!isValidPolicy(policy)) {   
          Navigation.navigate(ROUTES.SETTINGS);
          return;
       }
     }, []);

     return (
        <View>
           // Design comes here.
        </View>
     )
}

[aman-atg]

Proposal

Explaination

• The problem lies with the WorkspaceSidebar component which is returning null when the policy object is empty
• We can simply use Navigation.dismissModal(); to avoid this behaviour or redirect the user somewhere we want.
  

  App/src/pages/workspace/WorkspaceSidebar.js

  Lines 66 to 68 in 87cbc24

  	if (_.isEmpty(policy)) {
  	return null;
  	}

  
  will be changed to

useEffect(()=>{
    if (_.isEmpty(policy)) {
        Navigation.dismissModal();
        return null;
    }
},[policy])

@mananjadhav I think your approach is checking for something that's already being checked inside WorkspaceSidebar. Don't you think? I think the root cause of the problem is Sidebar's returning null.

[mananjadhav]

@aman-atg That's right. I missed the check-in WorkspaceSidebar. A small change in your approach. Otherwise, it will throw an exception.

useEffect(() => {
      if (_.isEmpty(policy)) {
      Navigation.dismissModal();
      return null;
    }

    }, []);

[aman-atg]

@mananjadhav Shouldn't we include policy inside the dependency array?

Otherwise, it will throw an exception.

It was already setup like this but can you please elaborate on this.

[mananjadhav]

I am not aware if policy will get changed anywhere. If it does, then yeah we'll have to add it as a dependency. Empty brackets [], will act as a componentDidMount for class components.

It was already set up like this but can you please elaborate on this.

Earlier it was just returning null. Now we're calling Navigation.dismissModal. In the console tab of devtools you'll find the following error/warning.

Warning: Cannot update a component (`ForwardRef(BaseNavigationContainer)`) while rendering a different component (`WorkspaceSidebar`). To locate the bad setState() call inside `WorkspaceSidebar`, follow the stack trace as describ

[aman-atg]

@mananjadhav Thanks for the clarification.