feat(Environments): Track first evaluation with a new Onboarding interface#8063
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds nullable first-evaluation fields to environments, with clone reset behaviour and migration support. Introduces an onboarding status API for reading and recording evaluation state, including validation, idempotent persistence, document writes, logging, and tests. SDK user-agent detection now records the first evaluation during flag requests. Engine and DynamoDB mappings expose Estimated code review effort: 3 (Moderate) | ~25 minutes Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@themis-blindfold review |
⚖️ Themis judgement: 🟠 Fix before mergeNew
🟠 Majors
🧹 Nits
📝 Walkthrough
🧪 How to verify
Automate: a test for GET with an unknown API key asserting 404. Product take: Solid onboarding signal — knowing when and which SDK first evaluates an environment lets the dashboard guide new users through setup. Meaningful for activation tracking; moderate product impact. 🧭 Assumptions & unverified claims
First evaluation is a write-once operation, but that test assertion only gets one chance too — and it picked the wrong keyword. · reviewed at 9c3dfa7 |
Docker builds report
|
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 616f3493-5fbb-4b3b-8c20-3b1649411858
📒 Files selected for processing (17)
api/environments/migrations/0038_add_first_evaluated_fields.pyapi/environments/models.pyapi/environments/onboarding/__init__.pyapi/environments/onboarding/serializers.pyapi/environments/onboarding/tasks.pyapi/environments/onboarding/urls.pyapi/environments/onboarding/views.pyapi/environments/urls.pyapi/tests/unit/environments/onboarding/__init__.pyapi/tests/unit/environments/onboarding/test_unit_environments_views.pyapi/tests/unit/util/mappers/test_unit_mappers_dynamodb.pyapi/tests/unit/util/mappers/test_unit_mappers_engine.pyapi/util/engine_models/environments/models.pyapi/util/mappers/engine.pyapi/util/mappers/sdk.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.mdopenapi.yaml
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #18642 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-16 — run #18642 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #18643 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-16 — run #18643 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #18642 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-16 — run #18642 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ private-cloud · depot-ubuntu-latest-16 — run #18641 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #18641 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #18640 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
✅ private-cloud · depot-ubuntu-latest-16 — run #18640 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-16 — run #18641 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #18640 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/environments/onboarding/views.py (1)
34-48: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftRequire an authenticated principal for onboarding updates.
This endpoint accepts anonymous writes and treats the URL API key as sufficient authority. Client SDK keys can be exposed to end users, allowing anyone with one to falsely complete onboarding and choose the recorded SDK label. Protect PUT with the existing environment-management permission model; retain a separate explicitly authorised skip flow if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f8452941-6950-4da6-871d-94518549d663
📒 Files selected for processing (9)
api/app_analytics/constants.pyapi/app_analytics/mappers.pyapi/environments/models.pyapi/environments/onboarding/tasks.pyapi/environments/onboarding/views.pyapi/features/views.pyapi/tests/unit/environments/onboarding/test_unit_environments_views.pyapi/tests/unit/environments/test_unit_environments_models.pyapi/tests/unit/features/test_unit_features_views.py
khvn26
left a comment
There was a problem hiding this comment.
Looks good on the whole — two questions (which are essentially one question) before I approve, and a nit.
e628afd to
1e15195
Compare
1e15195 to
1fedc19
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b68cfc2b-c1bf-4237-8955-6a0fc72aeb9b
📒 Files selected for processing (11)
api/environments/onboarding/services.pyapi/environments/onboarding/urls.pyapi/environments/onboarding/views.pyapi/features/views.pyapi/tests/conftest.pyapi/tests/test_helpers.pyapi/tests/unit/environments/onboarding/test_unit_environments_views.pyapi/tests/unit/features/test_unit_features_views.pyapi/tests/unit/onboarding/conftest.pyapi/tests/unit/onboarding/test_views.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.md
💤 Files with no reviewable changes (1)
- api/tests/unit/onboarding/conftest.py
khvn26
left a comment
There was a problem hiding this comment.
LGTM — the remaining conversation is non-blocking.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/environments/onboarding/views.py (1)
18-20: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRequire authentication for the PUT endpoint.
The view explicitly disables authentication and permissions, so any caller possessing only the
environment_api_keycan submit a valid SDK label. Because persistence is first-write-wins, this allows onboarding data to be poisoned before the legitimate SDK evaluates the environment. Apply the project’s standard authenticated permission to PUT, or split the public GET and authenticated PUT into separate views.Also applies to: 27-35
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 69abd0de-ef75-4bda-93d3-1c75de70a4c6
📒 Files selected for processing (3)
api/environments/onboarding/urls.pyapi/environments/onboarding/views.pyapi/tests/unit/environments/onboarding/test_unit_environments_views.py
💤 Files with no reviewable changes (1)
- api/tests/unit/environments/onboarding/test_unit_environments_views.py
docs/if required so people know about the feature.Changes
Closes #7623
/api/v1/environments/{api_key}/onboarding-status/API:GETobtains first evaluation data.PUTrecords first evaluation, or skips.onboarding_pending./api/v1/flags/also records first evaluation.Review effort: 3/5
How did you test this code?
Unit tests.