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

Global create opening in workspace configuration screen #5392

Closed
MitchExpensify opened this issue Sep 21, 2021 · 11 comments
Closed

Global create opening in workspace configuration screen #5392

MitchExpensify opened this issue Sep 21, 2021 · 11 comments
Assignees

Comments

@MitchExpensify
Copy link
Contributor

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Action Performed:

  1. Sign up to OldDot using a new domain/account (a domain not used before)
  2. Select "Business"
  3. Select "Expensify Card" > Submit
  4. See Global Create open in Workspace configuration:

image

Expected Result:

Global Create should not be open in the workspace configuration screen. This change was intended to fix that - Skip showing create for new users if user already has a workspace

Actual Result:

Global Create is open in the workspace configuration screen

Workaround:

Ignore it and click "Get Started". This is the mainline flow so we can't expect users to follow this workaround. They could easily select a global create option and get diverted from the mainline flow.

Platform:

  • Web

View all open jobs on GitHub

@MitchExpensify MitchExpensify added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Sep 21, 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 Sep 21, 2021
@parasharrajat
Copy link
Member

@MitchExpensify #5244 was reverted here

@Beamanator
Copy link
Contributor

Latest Slack thread here.

Basically it's pretty easy to reproduce in dev, and was determined that currently there shouldn't be any race condition weirdness with one possible solution (@marcaaron 's PR: https://github.com/Expensify/App/pull/5394/files).

However, @TomatoToaster is working on changing some of the flow so we're planning to hold on this until that change is done.

@marcaaron
Copy link
Contributor

Hmm so this is actually really tricky to solve now because of the latest changes.

Basically when the Sidebar loads we need to know if we have a free policy, but we might not have created one yet.

Before the most recent changes we were only redirecting after the policy was created. But now the sidebar loads (inside the MainDrawerNavigator) at the same time as the LoginWithShortLivedTokenPage. So we can no longer guarantee that allPolicies will contain the policy we just created.

I'm struggling to think of a clean way to get around this but we can hack it for now by looking at what route we're on. Unless someone has a better idea... I'm a little stumped since the sidebar doesn't know that we are trying to create a policy via WorkspaceNew and the two things don't really have any relationship to eachother.

@marcaaron
Copy link
Contributor

Probably a better way to fix this would be to stop triggering the modal to open via the SidebarScreen in componentDidMount() entirely and instead do it programmatically (unsure if there's a clear reason why it should be there vs. somewhere like Expensify.js), but that's going to be more complicated than referring to the navigation state.

@TomatoToaster
Copy link
Contributor

Ah wait are you seeing it workspace/new as well? I only saw it with workspace/12312/card when I was transitioning from the existing policy. Is the invalid workspace closing bug also happening for if you hit localhost:8080/workspace/new?

@marcaaron
Copy link
Contributor

you seeing it workspace/new as well?

Yeah global create thing pops open for workspace/new because we are creating it via New Expensify and the sidebar may load before that happens e.g. if it takes longer than 1.5 seconds to create the policy.

@TomatoToaster
Copy link
Contributor

TomatoToaster commented Sep 22, 2021

Ahh wait sorry I'm getting this discussion and the inconsistent InvalidWorkspace thing mixed up. Ignore what I said there.

This is kind of hacky behavior on top of hacky behavior, but one thing we could do is set this NVP which is called from Onyx here for the user while we're doing the transition in LogInWithShortToken or anywhere in its flow.

Anyone who's coming in from OldDot is going to be routed to a Workspace so we don't need to automatically open the global create menu.

@kidroca
Copy link
Contributor

kidroca commented Sep 22, 2021

It makes sense to me to move the isFirstTimeNewExpensifyUser out of the SidebarScreen and pass a prop to trigger the create menu to open when it has to, or save a ref somewhere so that it can be triggered like SidebarScreen.toggleCreateMenu()

@kidroca
Copy link
Contributor

kidroca commented Sep 22, 2021

It just that the logic should be synchronised to have the firstTimeUser check and the policy check as well as trigger the create menu no sooner than 1500ms (to avoid the modal bug)

e.g. something like

Promise.all([
  firstUserCheck,
  policyCheck,
  delay(1500),
])
 .then(results => /* open the create menu if any of the checks passed `true` */)

@marcaaron
Copy link
Contributor

@TomatoToaster Oh hmm so basically set them as "not a first time user"? That could work, but creates a weird condition where we must block navigating until after the NVP has been set + also block on the two API calls @kidroca is calling out.

Either way I think we'll have to reorganize the code after, but looking at the route works for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants