Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe changes add API limit restriction evaluation for free-plan organisations and use the result in the grace-period task. The organisation serializer exposes restriction status, grace-period usage and overage billing eligibility as read-only fields. The overage eligibility check uses subscription, billing-period and feature-flag conditions. The frontend organisation type and OpenAPI schemas include the response fields. Unit tests cover the evaluator, task behaviour and serializer output. Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The API adds read-only organization status fields. No concrete behavior violating the repository’s response or billing contract is established, so no material merge-blocking risk remains. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The new fields remain read-only and organisation-scoped, without granting billing or enforcement authority. The reviewed enforcement lifecycle appears preserved. Disabled-alerting responses do not fully match the stated all-false behavior, and incomplete evidence prevents a minimal-risk assessment. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8641 +/- ##
========================================
Coverage 98.84% 98.85%
========================================
Files 1658 1666 +8
Lines 68447 68788 +341
========================================
+ Hits 67657 67999 +342
+ Misses 790 789 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@themis-blindfold review |
⚖️ Themis review: 🟠 Fix before mergeThis adds the requested organisation state, but Targeted tests could not run locally because the sandbox cannot fetch the project dependencies.
🟠 Majors
📝 Walkthrough
🧪 How to verify
Product take: The new response fields would make API-limit state visible to the usage experience, a solid product improvement. 🧭 Assumptions & unverified claimsNo unverified assumptions or claims. One grace-period row cannot do two jobs without a name tag · reviewed at 023fcf4 |
023fcf4 to
5ceb643
Compare
|
@themis-blindfold review |
⚖️ Themis review: 🟠 Fix before mergeThe API now exposes the required state, but the dashboard never reads it: it still infers possible charges from the plan name and billing period. As a result, the usage page cannot tell an over-limit customer whether grace covers them or a charge applies, nor surface the new restriction state. All completed CI checks passed.
🟠 Majors
⚖️ Acknowledged
📝 Walkthrough
🧪 How to verify
Product take: The server now supplies the facts the usage experience needs, but customers still receive the old generic guidance. Until the dashboard consumes them, this is a partial implementation of a customer-facing billing feature. 🧭 Assumptions & unverified claimsNo unverified assumptions or claims. The API has brought the facts; the dashboard still needs to read the briefing. · reviewed at cc07346 |
Docker builds report
|
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: e27ea962-cd51-40b1-9e17-b7c02abe9cf2
📒 Files selected for processing (12)
api/organisations/constants.pyapi/organisations/dataclasses.pyapi/organisations/serializers.pyapi/organisations/services.pyapi/organisations/tasks.pyapi/organisations/views.pyapi/tests/unit/organisations/test_unit_organisations_serializers.pyapi/tests/unit/organisations/test_unit_organisations_services.pyapi/tests/unit/organisations/test_unit_organisations_tasks.pyfrontend/common/types/responses.tsmcp/src/flagsmith_mcp/openapi.jsonopenapi.yaml
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review.
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #21279 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-16 — run #21279 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #21279 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-16 — run #21279 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
Skipped testsfirefox › tests/onboarding-tests.pw.ts › Onboarding › New user connects via the single-page onboarding flow @oss ✅ private-cloud · depot-ubuntu-latest-arm-16 — run #21278 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
✅ private-cloud · depot-ubuntu-latest-16 — run #21278 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #21278 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-16 — run #21278 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #21251 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
✅ private-cloud · depot-ubuntu-latest-16 — run #21251 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-16 — run #21251 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #21251 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
|
Visual Regression20 screenshots compared. See report for details. |
There was a problem hiding this comment.
such simply named functions, such a lot of code 😄
| openfeature_client = get_openfeature_client() | ||
| evaluation_context = organisation.openfeature_evaluation_context | ||
| return APILimitRestrictions( | ||
| stop_serving_flags=openfeature_client.get_boolean_value( | ||
| "api_limiting_stop_serving_flags", | ||
| default_value=False, | ||
| evaluation_context=evaluation_context, | ||
| ), | ||
| block_access_to_admin=openfeature_client.get_boolean_value( | ||
| "api_limiting_block_access_to_admin", | ||
| default_value=False, | ||
| evaluation_context=evaluation_context, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
I'm not sure I understand why we're using Flagsmith for these versus the stop_serving_flags and block_access_to_admin attributes on the Organisation model?
There was a problem hiding this comment.
Yes, the naming was confusing. The model fields say if the org is blocked right now, and these fields were already in the response. The flags decide what it'll do once the grace period is over, so this is more "would we cut them off if they go over".
FE needs that to warn free orgs before it happens. I renamed it to APILimitEnforcement so it doesn't confict with the model fields anymore wdyt ?
Cf the comment above 😅
There was a problem hiding this comment.
Should we not just have the FE access the flags directly in that case? Since we merged the projects, I think that's feasible, right?
There was a problem hiding this comment.
It's feasible but do we really want to? This way it's living close to the task where it decides whether the org should be cut-off, so it avoids drifting and duplicating the (small, I agree) logic that combines the free plan check.
This way the frontend just blindly applies what the BE is sending.
But re-looking at it, i realized ENABLE_API_USAGE_ALERTING is only set in the task processor as we speak. I'll add it now
… and rename API limit enforcement
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Contributes to #8256. Supersedes #8584 and #8492.
Four read-only fields on the organisation response:
stop_serving_flagsapi_limit_restriction_enabledapi_limiting_*flag onapi_limit_grace_period_usedoverage_billing_eligibleapi_usage_overage_chargesonThe two computed fields are
falsewhenENABLE_API_USAGE_ALERTINGis off, since the tasks aren't registered then.The restriction task shares
get_api_limit_restrictions().is_overage_billing_eligible()mirrors the billing task's eligibility, with a parity test running the real task against it. Neither task's behaviour changes.The viewset
select_relatedsubscription,subscription_information_cacheandbreached_grace_period, so the fields add no per-organisation queries.How did you test this code?