-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Implement upgrade to collect when booking travel #53753
base: main
Are you sure you want to change the base?
Conversation
src/languages/es.ts
Outdated
travel: { | ||
title: 'Viajes', | ||
description: | ||
'Expensify Travel es una nueva plataforma corporativa de reserva y gestión de viajes que permite a los miembros reservar alojamientos, vuelos, transporte y mucho más.', | ||
onlyAvailableOnPlan: 'Travel solo está disponible en el plan Controlar, a partir de ', | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cristipaval Can you please get the translation confirmation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll confirm this comment first.
Screen.Recording.2024-12-09.at.6.50.15.PM.mov@Expensify/design Is this flow correct? |
@ryanschaffer @stitesExpensify is this correct? I don't think Travel is only available on Control, but rather, it just requires at least a workspace. So if anything, that modal should say "Only available on Collect and Control" |
That's my understanding too. It should be a prompt to create a workspace for those with none. Rather than a prompt to upgrade a collect workspace to control. |
@shawnborton What should be the button text? |
Rather than use this upgrade screen, what if we use the workspace confirmation step that we just merged? Maybe we can update the text to include something about travel? cc @jamesdeanexpensify we current say this:
Maybe we could say this:
|
I like that too. |
I thought we had decided to use the upgrade modal similar to categories in this situation? It's not an upgrade from Collect to Control, but it's still an upgrade from no workspace to a Collect workspace. Right? Or am I missing something? |
Yeah, that is what we'd landed on. I think Shawn just meant that we also now have this new page for confirming to create a workspace that might apply too. Not sure of the context of the new page though or the pattern of where we use one vs the other. |
What about for the copy (it felt a bit too long as suggested, so removing the reference to
|
Ah yeah that's a good point Danny. So here are my thoughts then:
Does that sound like a plan? |
Yeah that sounds good! |
That is correct. I think we settled on something like "Only available on paid workspaces" |
Aren't all workspaces paid? Or they will be soon with Ryan's project. Workspace creation will kick off the free trial. |
I haven't looked at that project yet, but the idea was that we need to prevent users who only have a personal policy on OldDot |
Ah, got it. Thank you! |
@shawnborton Can you please let me know the correct text here? |
cc @jamesdeanexpensify for thoughts but maybe we can do:
|
Quick clarifying question - have they (1) upgraded their existing workspace to the Collect plan or (2) upgraded to a (new) workspace on the Collect plan? That might determine my copy feedback (and then maybe we should revisit the categories copy based on this, too). |
I think we show this because the user didn't have any workspace period, so we ask them to at least make a workspace first (much like we do when someone tries to categorize something without a workspace). I think we just start them off on a Collect workspace. |
Ok, cool! Would you be down with:
|
Love it, let's do it |
@shubham1206agra Is this ready for review? |
Hold on #53845 as we require workspace confirmation screen. |
@shubham1206agra, the PR is merged and it's been in staging for more than 1 day. I hope it will survive and won't get reverted and will end up in production. I think you can continue on this one, given that the deadline for travel is at the end of next week when we want this deployed. |
Screen.Recording.2025-01-29.at.12.22.05.AM.mov@Expensify/design Is this flow fine? |
I think that looks pretty good. If they click the I was thinking for a second that it might be nice to take them to the travel flow immediately after the upgrade confirmation, but that doesn't really make any sense since travel opens in a new window. So I think what you have there looks good 👍 |
Yeah, agree that it probably still make sense to show the Travel panel after upgrade |
@mananjadhav Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
@shawnborton that would work for me. As long as it's clear that when they click the button something is going to happen, I'm good. I do think that in a similar issue* Joe is proposing that the button on the confirmation screen continues you along the flow you started, so I guess it would make sense to do that here as well. I could go either way. *Quoted from linked issue:
Maybe if we decide to do that here as well, we can update the button text in that linked issue to also communicate "Continue doing what you were doing" instead of "Got it, thanks". |
Totally, makes sense to align them if we're cool with doing something like this. |
Yep, happy to change the copy on the button if we decide on that in both places. I also agree that it makes sense to continue along the path, as you got prompted to upgrade while trying to take an action. There is also precedent in the app for this now. If you enable an accounting integration and upgrade to access it, we continue you in the setup flow: 2025-01-29_10-13-06.mp4 |
Wouldya just look at that, nice! |
Boom, precedent! I say we move forward with that same idea in both of these new places then. |
I like where this is landing 👍 |
Ok bad news We can't do that since after policy creation, we don't automatically set And there is a precedent of just going back and not continuing, in the flow when company address is empty (after saving the address, we just go back without continuing). cc @shawnborton @dannymcclain @dubielzyk-expensify @joekaufmanexpensify |
So are you saying we'll need to drop them back on the |
Yes. But I am not against refinement in this flow in the later stage. |
@mananjadhav Can you start the review now? Since this is a bit urgent. |
I can definitely start this but I am not sure if I'll be able to finish it immediately. Let me prioritize and try this. |
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
@mananjadhav Please continue with the review. |
createDraftWorkspace('', false, params.name, params.policyID, params.currency, params.avatarFile as File); | ||
setShouldShowConfirmation(false); | ||
setIsUpgraded(true); | ||
createWorkspace('', false, params.name, params.policyID, '', params.currency, params.avatarFile as File); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mananjadhav What should the onboarding intent be here?
Explanation of Change
Fixed Issues
$ #48305
Tests
Pre-requisite - Have a new account with
spotnanaTravel
beta enabled and no workspace created.Offline tests
Same as Tests
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Screen.Recording.2025-01-29.at.12.13.58.PM.mov
Android: mWeb Chrome
Screen.Recording.2025-01-29.at.11.58.52.AM.mov
iOS: Native
Screen.Recording.2025-01-29.at.12.09.22.PM.mov
iOS: mWeb Safari
Screen.Recording.2025-01-29.at.11.56.30.AM.mov
MacOS: Chrome / Safari
Screen.Recording.2025-01-29.at.12.22.05.AM.mov
MacOS: Desktop
Screen.Recording.2025-01-29.at.12.02.17.PM.mov