-
Notifications
You must be signed in to change notification settings - Fork 46.1k
feat(platform): Backend completion of Onboarding tasks #11375
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
base: dev
Are you sure you want to change the base?
Conversation
…into kpczerwinski/secrt-1727-onboarding-notifications
…into kpczerwinski/secrt-1727-onboarding-notifications
|
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
|
Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly. |
|
Thanks for implementing the backend support for onboarding tasks! This looks like a solid improvement that will help track and reward user progress automatically. However, before this PR can be merged, you need to address these issues: Missing PR Description
Technical ImplementationThe implementation looks good overall! Here are some observations:
Please update the PR description with details about:
Once you've addressed these documentation issues, this PR should be ready for another review. |
|
Thanks for this PR that improves the security of the onboarding process by moving task completion to the backend! What looks good
What needs attentionThe PR needs a completed checklist as per our requirements. Please fill out the test plan section and check off the boxes in the PR description. Since this is a significant change touching both backend and frontend, having a clear test plan is important. Suggested test plan could include:
Once you've added and checked off the test plan items, this PR should be good to go! |
|
Thanks for your PR on making onboarding task completion backend-authoritative! The changes look well thought out and should effectively prevent users from cheating by marking all tasks as completed instantly. I noticed a few items to address before this can be merged:
Once you've completed testing the full onboarding flow and updated your checklist accordingly, this PR should be ready for final review and merging. The code changes themselves look well-structured and maintain proper security patterns with user_id checks. |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
| @field_serializer("payload") | ||
| def serialize_payload(self, payload: NotificationPayload): | ||
| """Ensure extra fields survive Redis serialization.""" | ||
| return payload.model_dump() |
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.
This makes all fields of NotificationPayload subclasses serialized (in this case OnboardingNotificationPayload)
| FrontendOnboardingStep = Literal[ | ||
| OnboardingStep.WELCOME, | ||
| OnboardingStep.USAGE_REASON, | ||
| OnboardingStep.INTEGRATIONS, | ||
| OnboardingStep.AGENT_CHOICE, | ||
| OnboardingStep.AGENT_NEW_RUN, | ||
| OnboardingStep.AGENT_INPUT, | ||
| OnboardingStep.CONGRATS, | ||
| OnboardingStep.MARKETPLACE_VISIT, | ||
| OnboardingStep.BUILDER_OPEN, | ||
| ] |
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.
This is a subset of OnboardingStep with only the step that are allowed to be completed frontend-side.
| async def update_user_onboarding(user_id: str, data: UserOnboardingUpdate): | ||
| update: UserOnboardingUpdateInput = {} | ||
| onboarding = await get_user_onboarding(user_id) | ||
| if data.completedSteps is not None: |
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.
Step completion has now dedicated endpoint.
| mock_complete_onboarding = mocker.patch( | ||
| "backend.server.v2.library.routes.agents.complete_onboarding_step", | ||
| new_callable=AsyncMock, | ||
| ) |
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.
This makes sure we don't actually call onboarding function (and try to access db)
| GraphCreationSource = Literal["builder", "upload"] | ||
| GraphExecutionSource = Literal["builder", "library", "onboarding"] |
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.
We need sources to know if an onboarding step should be completed; for example running agent from library shouldn't be completed when running onboarding agent or in the builder.
| CREDENTIALS_FIELDS: dict[str, str] = get_credentials_blocks() | ||
|
|
||
|
|
||
| def _normalize_datetime(value: datetime | None) -> datetime | None: |
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.
The following functions are to calculate run streak for the user for running agent, i.e. keep increasing by one every day and if one day is missed reset. The time is calculated in the user timezone.
|
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
|
Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly. |
| return get_user_timezone_or_utc(user.timezone if user else None) | ||
|
|
||
|
|
||
| async def increment_runs(user_id: str): |
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 know how far are we trying to be careful with this,
It's safer to just increment the integer field than do this see Updating Atomic Fields in https://prisma-client-py.readthedocs.io/en/v0.1.0/reference/operations/#integer-fields_1
Make onboarding task completion backend-authoritative which prevents cheating (previously users could mark all tasks as completed instantly and get rewards) and makes task completion more reliable. Completion of tasks is moved backend with exception of introductory onboarding tasks and visit-page type tasks.
Changes 🏗️
source, so appropriate tasks can be completedclient.tsapi calls with orval generated and remove no longer used functions fromclient.tsresolveResponsehelper function that unwraps orval generated call result to 2xx responseSmall changes&bug fixes:
Checklist 📋
For code changes: