update has_seats_left on plan service to work properly with self-hosted and DEC #748
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. 📢 Thoughts on this report? Let us know! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #748 +/- ##
================================================
+ Coverage 96.05000 96.06000 +0.01000
================================================
Files 814 814
Lines 18493 18509 +16
================================================
+ Hits 17764 17780 +16
Misses 729 729
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| def can_activate_owner(owner: Owner) -> bool: | ||
| """ | ||
| Returns true iff there are available seats left for activation. | ||
| Returns true if there are available seats left for activation. |
There was a problem hiding this comment.
weird how all of these had 2 "f"s lol
Maybe they meant if and only if?
There was a problem hiding this comment.
yeah is that a shorthand I don't know about either?
There was a problem hiding this comment.
microsoft says it's if and only if but iff only works with 2 statements right? so i think typo
| return await sync_to_async(activated_owner_query)() | ||
|
|
||
|
|
||
| def activated_owner_query() -> QuerySet: |
There was a problem hiding this comment.
should we be worried about perf at all with this fn? genuinely curious
There was a problem hiding this comment.
yes 🙃
I gated it behind a license check - so it will only run if the license is valid
| @@ -83,9 +91,15 @@ def license_seats() -> int: | |||
| return license.number_allowed_users or 0 | |||
|
|
|||
|
|
|||
| async def enterprise_has_seats_left() -> bool: | |||
| owners = await activated_owners_async() | |||
There was a problem hiding this comment.
Are we able to do the owners.count() here directly on the query set?
There was a problem hiding this comment.
it throws error if I don't make the count() sync_to_async as well, the tradeoff is that I get to use the same activated_owner_query that is used elsewhere, but i have to do this additional async step
|
Test Failures Detected: Due to failing tests, we cannot provide coverage reports at this time. ❌ Failed Test Results:Completed 2274 tests with View the full list of failed testspytest
|
1 similar comment
|
Test Failures Detected: Due to failing tests, we cannot provide coverage reports at this time. ❌ Failed Test Results:Completed 2274 tests with View the full list of failed testspytest
|
Purpose/Motivation
We calculate seats differently for self-hosted and DEC, this updates the
has_seats_leftonPlanServiceto work correctly for self-hosted and DEC.Links to relevant tickets
https://github.com/codecov/internal-issues/issues/679
What does this PR do?
The function was comparing the total seat count to the number of active users for the Org. With self-hosted and DEC, it will now compare total seat count to the number of unique users across all Orgs.