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

Fix plan_activated_users count for an installation #132

Merged
merged 4 commits into from
Oct 10, 2023

Conversation

joseph-sentry
Copy link
Contributor

@joseph-sentry joseph-sentry commented Oct 4, 2023

this PR changes the behaviour of get_installation_plan_activated_users to no longer count one user in multiple orgs as multiple users to match the behaviour in the API

Fixes: #121

@codecov
Copy link

codecov bot commented Oct 4, 2023

Codecov Report

Merging #132 (6c00d0f) into main (41cf406) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #132   +/-   ##
=======================================
  Coverage   98.40%   98.40%           
=======================================
  Files         373      373           
  Lines       27692    27717   +25     
=======================================
+ Hits        27251    27276   +25     
  Misses        441      441           
Flag Coverage Δ
integration 98.44% <100.00%> (+<0.01%) ⬆️
latest-uploader-overall 98.44% <100.00%> (+<0.01%) ⬆️
unit 98.44% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
NonTestCode 96.91% <ø> (ø)
OutsideTasks 98.24% <100.00%> (+<0.01%) ⬆️
Files Coverage Δ
services/license.py 96.82% <ø> (ø)
services/tests/test_activation.py 100.00% <100.00%> (ø)

This change has been scanned for critical changes. Learn more

@codecov-staging
Copy link

codecov-staging bot commented Oct 4, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@dd4d0eb). Click here to learn what that means.
Report is 8 commits behind head on main.
The diff coverage is 100.00%.

❗ Current head 31d7366 differs from pull request most recent head 69aa03b. Consider uploading reports for the commit 69aa03b to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #132   +/-   ##
=======================================
  Coverage        ?   93.25%           
=======================================
  Files           ?      346           
  Lines           ?    26910           
  Branches        ?        0           
=======================================
  Hits            ?    25096           
  Misses          ?     1814           
  Partials        ?        0           
Flag Coverage Δ
integration 93.25% <100.00%> (?)
latest-uploader-overall 93.25% <100.00%> (?)
unit 93.25% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
NonTestCode 93.96% <ø> (?)
OutsideTasks 96.72% <100.00%> (?)
Files Coverage Δ
services/license.py 96.82% <ø> (ø)
services/tests/test_activation.py 100.00% <100.00%> (ø)

@codecov-qa
Copy link

codecov-qa bot commented Oct 4, 2023

Codecov Report

Merging #132 (6c00d0f) into main (41cf406) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #132   +/-   ##
=======================================
  Coverage   98.44%   98.44%           
=======================================
  Files         347      347           
  Lines       27196    27221   +25     
=======================================
+ Hits        26774    26799   +25     
  Misses        422      422           
Flag Coverage Δ
integration 98.44% <100.00%> (+<0.01%) ⬆️
latest-uploader-overall 98.44% <100.00%> (+<0.01%) ⬆️
unit 98.44% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
NonTestCode 97.02% <ø> (ø)
OutsideTasks 98.24% <100.00%> (+<0.01%) ⬆️
Files Coverage Δ
services/license.py 96.82% <ø> (ø)
services/tests/test_activation.py 100.00% <100.00%> (ø)

@joseph-sentry joseph-sentry marked this pull request as ready for review October 4, 2023 16:30
@codecov-public-qa
Copy link

codecov-public-qa bot commented Oct 5, 2023

Codecov Report

Merging #132 (6c00d0f) into main (41cf406) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #132   +/-   ##
=======================================
  Coverage   98.44%   98.44%           
=======================================
  Files         347      347           
  Lines       27196    27221   +25     
=======================================
+ Hits        26774    26799   +25     
  Misses        422      422           
Flag Coverage Δ
integration 98.44% <100.00%> (+<0.01%) ⬆️
latest-uploader-overall 98.44% <100.00%> (+<0.01%) ⬆️
unit 98.44% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
NonTestCode 97.02% <ø> (ø)
OutsideTasks 98.24% <100.00%> (+<0.01%) ⬆️
Files Coverage Δ
services/license.py 96.82% <ø> (ø)
services/tests/test_activation.py 100.00% <100.00%> (ø)

Copy link
Contributor

@giovanni-guidini giovanni-guidini left a comment

Choose a reason for hiding this comment

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

Fix looks fine from the ticket description. I'm having a hard time understanding how the test relates. Asked some clarifying questions (thanks for indulging me 😅)

dbsession.add(org_second)
dbsession.flush()

encrypted_license = "wxWEJyYgIcFpi6nBSyKQZQeaQ9Eqpo3SXyUomAqQOzOFjdYB3A8fFM1rm+kOt2ehy9w95AzrQqrqfxi9HJIb2zLOMOB9tSy52OykVCzFtKPBNsXU/y5pQKOfV7iI3w9CHFh3tDwSwgjg8UsMXwQPOhrpvl2GdHpwEhFdaM2O3vY7iElFgZfk5D9E7qEnp+WysQwHKxDeKLI7jWCnBCBJLDjBJRSz0H7AfU55RQDqtTrnR+rsLDHOzJ80/VxwVYhb"
Copy link
Contributor

Choose a reason for hiding this comment

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

How many seats does this license allows for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a comment to the code

@@ -89,11 +88,57 @@ def test_activate_user_success_for_enterprise_pr_billing(
dbsession.commit()
assert user.ownerid in org.plan_activated_users

def test_activate_user_success_user_org_overlap(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand this test case.

I looks like you are creating 2 orgs with 5 seats taken each. Idk how many total seats this installation (self-hosted) can have. But it seems that all owners of the 2 orgs are distinct (e.g. with different IDs)

So the total count of seats used is 10, and you create a new owner to prove that we can extend that.

Is that the idea?


(assuming I'm correct - which I might very well not be)
What I'd like to see:

  1. Call get_installation_plan_activated_users explicitly to see what users are activated (before and after creating the new user)
  2. Have some overlap on the plan_activated_users across the orgs (cause that was the original problem, right?)

@joseph-sentry joseph-sentry force-pushed the joseph/fix-plan-activated-users branch 2 times, most recently from 69aa03b to 15c90b5 Compare October 9, 2023 09:36
@joseph-sentry joseph-sentry force-pushed the joseph/fix-plan-activated-users branch from 15c90b5 to 6c00d0f Compare October 9, 2023 12:57
@joseph-sentry joseph-sentry merged commit 0ff20ab into main Oct 10, 2023
25 of 26 checks passed
@joseph-sentry joseph-sentry deleted the joseph/fix-plan-activated-users branch October 10, 2023 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mismatched counts of activated users
2 participants