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 on July 16] App crashes when trying to assign/unassign all in new workspace #3923

Closed
rdjuric opened this issue Jul 8, 2021 · 10 comments
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@rdjuric
Copy link
Contributor

rdjuric commented Jul 8, 2021

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. Open the app
  2. Click on the LHN and then on New Workspace
  3. Choose a name and create the Workspace
  4. Click on People on the left side
  5. Notice that no one shows up on the list
  6. Click on the checkbox close to ASSIGNEE

Expected Result:

After a workspace is created, the user should show up on the People list and clicking on the checkbox shouldn't crash.

Actual Result:

The user who created the workspace didn't show up on the list and after clicking the checkbox the app crashed.

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?
Refreshing the page makes the user show up and the checkbox no longer crashes.

Platform:

Where is this issue occurring?

Web ✔️
iOS ✔️
Android ✔️
Desktop App ✔️
Mobile Web ✔️

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

web.mov

Expensify/Expensify Issue URL:
View all open jobs on Upwork

@rdjuric rdjuric added the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Jul 8, 2021
@MelvinBot
Copy link

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

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Jul 8, 2021
@Christinadobrzyn Christinadobrzyn added Daily KSv2 External Added to denote the issue can be worked on by a contributor Engineering and removed External Added to denote the issue can be worked on by a contributor labels Jul 8, 2021
@MelvinBot
Copy link

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

@rdjuric
Copy link
Contributor Author

rdjuric commented Jul 8, 2021

This happens because we're not merging our employeeList after creating a new workspace.
https://github.com/Expensify/Expensify.cash/blob/bdbae0c998aa513fa5754d978f4bd8ed8976b595/src/libs/actions/Policy.js#L195-L200
So when we visit our WorkspacePeoplePage just after creating it, we won't show up on it and the toggleUser will crash since it expects an array but receives an undefined.
https://github.com/Expensify/Expensify.cash/blob/bdbae0c998aa513fa5754d978f4bd8ed8976b595/src/pages/workspace/WorkspacePeoplePage.js#L112

Refreshing makes the problem go away because that calls our getPolicySummaries which will update the local policy with the correct employeeList.


Proposal

When we create a new policy our api response contains a employeeList, we can use our getSimplifiedEmployeeListObject to merge that list locally in the right format. We need to:

  1. Modify our getSimplifiedEmployeeListObject so that it receives a employeeList as a parameter, not a fullPolicy
  2. Adapt our getPolicyList to the above change in this line
    https://github.com/Expensify/Expensify.cash/blob/bdbae0c998aa513fa5754d978f4bd8ed8976b595/src/libs/actions/Policy.js#L87
  3. Merge the employeeList locally when creating a new policy by adding
employeeList: getSimplifiedEmployeeListObject(response.policy.employeeList)

to our create action.

@luacmartins
Copy link
Contributor

This can be external. Proposal LGTM, @marcaaron would love your eyes here since I'm less familiar with the code.

@luacmartins luacmartins added the External Added to denote the issue can be worked on by a contributor label Jul 8, 2021
@MelvinBot
Copy link

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

@marcaaron
Copy link
Contributor

Nice catch @rdjuric. Normally, this would be the responsibility of the PR authors as the feature has not yet gone to production and is also under a beta. In this case, that would be myself and @mountiny. But I have no issues with someone else fixing it :)

@marcaaron marcaaron assigned rdjuric and unassigned luacmartins Jul 8, 2021
@marcaaron
Copy link
Contributor

Solution LGTM and I've assigned you @rdjuric

@stephanieelliott
Copy link
Contributor

Job has been posted on Upwork, @rdjuric please post a quick proposal to the job on Upwork so we can hire you there!

Job on Upwork: https://www.upwork.com/jobs/~0102a46787404b3af3

@rdjuric
Copy link
Contributor Author

rdjuric commented Jul 9, 2021

Submitted a proposal there @stephanieelliott. Thanks!

@stephanieelliott stephanieelliott changed the title App crashes when trying to assign/unassign all in new workspace [HOLD for payment on July 16] App crashes when trying to assign/unassign all in new workspace Jul 12, 2021
@stephanieelliott
Copy link
Contributor

PR was merged, holding for 7 days to ensure no regressions and will pay on Upwork July 16.

@stephanieelliott stephanieelliott added Weekly KSv2 and removed Daily KSv2 labels Jul 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests

6 participants