-
Notifications
You must be signed in to change notification settings - Fork 1.3k
When starting a workspace but usage attribution is unclear, prompt for explicit user choice #11777
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
Conversation
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.
PR description doesn't seem to match the code 😄
AIUI this PR just removes the automatic attribution when a team signs up for Stripe.
Whoops, this was meant to be a Draft. 😅 Thanks for the quick review though @andrew-farries! 🙏 |
d916294
to
e02252a
Compare
e02252a
to
c335b08
Compare
started the job as gitpod-build-jx-attribution-prompt.3 because the annotations in the pull request description changed |
c335b08
to
da75c99
Compare
270cfbb
to
76098d7
Compare
76098d7
to
546e9da
Compare
@gtsiolis If you have some time, could you please provide some advice on the UX? 🙂 I went with a very basic, non-dismissible Modal, but it's still quite rough:
|
Looking at this now! 👀 UPDATE: Posted some thoughts in a relevant discussion (internal). |
export function BillingAccountSelector(props: { onSelected?: () => void }) { | ||
const { user, setUser } = useContext(UserContext); | ||
const { teams } = useContext(TeamsContext); | ||
const [teamsWithBillingEnabled, setTeamsWithBillingEnabled] = useState<Team[] | undefined>(); |
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.
@jankeromnes, I bet this is not the single place, where we'd need this kind of info. have you considered promoting this flag to the result of getTeams? team.isBillingEnabled
would be nice and it shave off the additional requests.
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.
@AlexTugarev I think this would be premature, because:
- I can't imagine other places that will need the same information currently (note: this billing account selector component is already reusable, and in fact it's already used in two places)
- I don't think it's wise to make
getTeams
load all the team subscriptions on every call, even when most callers don't need that currently
export const PAYMENT_ERROR = 450; | ||
|
||
// 455 Invalid cost center (custom status code) | ||
export const INVALID_COST_CENTER = 455; |
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.
can we agree on a common prefix for everything payment related, please?
and feel free to comment on that BTW!
alternative way to approach this, we could use just PAYMENT_ERROR
(in similar cases) and encode specific conditions into the payload. it feels like we'll have the needs to extends for more error modes. wdyt?
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 don't see much value in aligning every payment-related error around a single prefix or error code range (apart from readability of the error codes list -- which we arguably don't read that often).
Personally, I'd prefer to leave this as is (unless you have a strong opinion towards a different error name or code), and you can use whichever convention seems best in your PR. We can always align later if you think it's valuable, but I suspect that aligning these is not important in the grand scheme of things. 😇
546e9da
to
eaffa7a
Compare
eaffa7a
to
cb12e4f
Compare
Thanks for the reviews! I guess this is now ready to be merged, unless you have any final requests. 🙂 |
} | ||
} | ||
|
||
protected async findSingleTeamWithUsageBasedBilling(user: User): Promise<Team | undefined> { |
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.
FYI: This can also come out of BillingMode
: we already gather the data there, and only need to loop it through.
But that's improvement after both got merged.
@@ -231,12 +294,18 @@ export class UserService { | |||
async getWorkspaceUsageAttributionId(user: User, projectId?: string): Promise<string | undefined> { |
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.
nit, and out-of-scope here: It feels like this method (and it's descendants) do not necessarily belong into UserService, but more into usage/Attribution
or so. But that's sth for later. 🙃
@jankeromnes testing this now... |
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.
Code LGTM, tested and works 🛹
Error message is not nice, but does not harm prod, and can be a follow-up. 💯
…s unclear, prompt for explicit user choice
cb12e4f
to
8e11724
Compare
Description
When starting a workspace but usage attribution is unclear, prompt for explicit user choice.
Possible unclear situations:
Related Issue(s)
Fixes #11579
How to test
Release Notes
Documentation
Werft options: