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

Correctly import groups when the service account is both a member and an admin #364

Merged
merged 5 commits into from
Jun 9, 2023

Conversation

amstilp
Copy link
Contributor

@amstilp amstilp commented Jun 9, 2023

Bugfix: set is_managed_by_app correctly when importing a group where the app service account is both a member an an admin.

It appears that this bug occurs when the service account running the
app is both a member and an admin of the same group. Update the
ManagedGroup.anvil_import method to handle multipler ecords being
returned from the API for a given group.
Add tests confirming the ManagedGroup.anvil_audit behavior is correct
when the response contains two records for the same group, one as
an admin and one as a member.
If any of the records returned by the app indicates that the service
account is an admin, then set is_managed_by_app=True.

Closes #360
The email was being set by the last group in the json response,
which is not necessarily the group being imported. Fix the email
by setting it in the new for loop instead of at the end, when the
for loop has finished.
@amstilp amstilp changed the title Correctly set is_managed_by_app when importing an auth_domain when the app is both a member and an admin Correctly import groups when the service account is both a member and an admin Jun 9, 2023
@codecov
Copy link

codecov bot commented Jun 9, 2023

Codecov Report

Merging #364 (3621497) into main (1816507) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 3621497 differs from pull request most recent head 00c7056. Consider uploading reports for the commit 00c7056 to get more accurate results

@@           Coverage Diff           @@
##             main     #364   +/-   ##
=======================================
  Coverage   99.81%   99.81%           
=======================================
  Files         117      117           
  Lines       20547    20614   +67     
=======================================
+ Hits        20508    20575   +67     
  Misses         39       39           
Impacted Files Coverage Δ
anvil_consortium_manager/models.py 100.00% <100.00%> (ø)
...ortium_manager/tests/test_models_anvil_api_unit.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@amstilp amstilp merged commit 93ad7d4 into main Jun 9, 2023
@amstilp amstilp deleted the bugfix/issue-360 branch October 25, 2023 23:39
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.

1 participant