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

Add end of trial notification #319

Merged
merged 15 commits into from
Jan 31, 2024
Merged

Conversation

Sa1to
Copy link
Contributor

@Sa1to Sa1to commented Jan 19, 2024

Fixes #244 and #245.

Test plan

  • use testing accounts
  • change scheduler e.g. to 30 seconds

@mkondratek mkondratek force-pushed the sa1to/end-of-trial-notification branch from f65f8d6 to 6c5f52c Compare January 24, 2024 22:01
@mkondratek mkondratek force-pushed the sa1to/end-of-trial-notification branch from 6c5f52c to 0e620b4 Compare January 25, 2024 00:09
@mkondratek mkondratek changed the title WIP: Add end of trial notifcation Add end of trial notification Jan 25, 2024
@mkondratek mkondratek force-pushed the sa1to/end-of-trial-notification branch 4 times, most recently from 74d1875 to 2da2322 Compare January 25, 2024 12:09
@mkondratek
Copy link
Contributor

mkondratek commented Jan 26, 2024

ENDING SOON

image

ENDED

image

@mkondratek mkondratek force-pushed the sa1to/end-of-trial-notification branch from 8a3759a to e5b7e2f Compare January 27, 2024 20:15
@mkondratek mkondratek marked this pull request as ready for review January 28, 2024 00:12
@mkondratek mkondratek linked an issue Jan 29, 2024 that may be closed by this pull request
@mkondratek mkondratek force-pushed the sa1to/end-of-trial-notification branch 4 times, most recently from 2dbb7d0 to dbf5e8d Compare January 29, 2024 20:20
@kalanchan
Copy link
Contributor

kalanchan commented Jan 29, 2024

thanks @mkondratek!

in both notifications can you add a bit of bottom padding to space out the title and the body of the notification?

in the first notification, are you able to add a line break after Setup your payment information... to make it easier to read?

@mkondratek
Copy link
Contributor

in both notifications can you add a bit of bottom padding to space out the title and the body of the notification?

That would not be idiomatic - our notification will look very strange compared to the other notifications. I suggest keeping the present padding.

CodyAgentService.applyAgentOnBackgroundThread(project) { agent ->
agent.server
.getCurrentUserCodySubscription()
.thenApply { currentUserCodySubscription ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DO NOT run thenApply on server responses.
Or, run it if you want, but first answer me this:
on which thread/threadpool this will be running, and how big that thread pool is? :)
I do not consider it to be safe, maybe we should so some automagical wrapping just to be able to use it without thinking about it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do completeOnTimeout in the same block in line 63. I think it will be running on the very same thread (started by applyAgentOnBackgroundThread in line 39).

I do not understand your concerns in this case but likely I miss something. Let's discuss it via f2f.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How it can be running in the same thread, if the current thread can even finish in the meantime? :) This is not a blocking operation. It's tricky, I agree, let's discuss on the standup tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refactored the code so it's easier to read and understand.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, it's much more readable now, thank you!


agent.server
.evaluateFeatureFlag(GetFeatureFlag.CodyProTrialEnded)
.completeOnTimeout(false, 4, TimeUnit.SECONDS)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be safe, cannot we wait longer if that is a method run once every 2h?
Or is it simply always expected to return in xx ms?

Copy link
Contributor

@mkondratek mkondratek Jan 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can but 4 seconds seems to be much more than needed for a feature flag check + it is consistent with other timeouts

codyProTrialEnded: Boolean,
useSscForCodySubscription: Boolean
) {
if (currentUserCodySubscription.plan == Plan.PRO &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I verified logic with the specification and to my eyes it look good.

@mkondratek mkondratek force-pushed the sa1to/end-of-trial-notification branch from 30f6620 to f20090b Compare January 30, 2024 15:32
Copy link
Contributor

@pkukielka pkukielka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mkondratek mkondratek merged commit 09229c5 into main Jan 31, 2024
1 of 2 checks passed
@mkondratek mkondratek deleted the sa1to/end-of-trial-notification branch January 31, 2024 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants