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

test: ad groups update of the user model #103

Merged

Conversation

nikomakela
Copy link
Contributor

Test that the update_ad_groups function of the User-model works as expected:

  1. If the same group is mapped to multiple ad-groups and the ad-groups input argument contains only some, the group is still persisted to user instance since it was mapped to at least one of given ad-groups.

  2. If the ad-groups input argument does not have any link to an existing group of a user, the group is removed from the user instance.

  3. If the ad-groups input argument contains links to new groups that the user is not yet linked to, the group will be added to the user instance.

@nikomakela nikomakela force-pushed the test-user-update_ad_groups branch 2 times, most recently from 485499a to 54417b7 Compare June 26, 2024 16:07
Copy link

codecov bot commented Jun 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.75%. Comparing base (b031c80) to head (f3201ad).
Report is 20 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #103      +/-   ##
==========================================
+ Coverage   56.12%   65.75%   +9.63%     
==========================================
  Files          31       29       -2     
  Lines        1062     1098      +36     
==========================================
+ Hits          596      722     +126     
+ Misses        466      376      -90     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines 84 to 91
# Nothing changes
(
ALL_AD_GROUPS_MAPPING,
ALL_AD_GROUP_NAMES,
ALL_GROUP_NAMES,
ALL_AD_GROUP_NAMES,
ALL_GROUP_NAMES,
),
Copy link
Contributor

Choose a reason for hiding this comment

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

Just FYI, you can do something like this, which would make it more obvious which part of the test fails.

Suggested change
# Nothing changes
(
ALL_AD_GROUPS_MAPPING,
ALL_AD_GROUP_NAMES,
ALL_GROUP_NAMES,
ALL_AD_GROUP_NAMES,
ALL_GROUP_NAMES,
),
pytest.param(
ALL_AD_GROUPS_MAPPING,
ALL_AD_GROUP_NAMES,
ALL_GROUP_NAMES,
ALL_AD_GROUP_NAMES,
ALL_GROUP_NAMES,
id="nothing_changes",
),

Copy link
Contributor Author

@nikomakela nikomakela Jun 27, 2024

Choose a reason for hiding this comment

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

I added the pytest-params with ids'.

helusers/tests/test_models.py::TestUserAdGroups::test_update_ad_groups[nothing_changes] PASSED                                                                                                                                                                                                                                                                                    [ 46%]
helusers/tests/test_models.py::TestUserAdGroups::test_update_ad_groups[not_mapped_not_added] PASSED                                                                                                                                                                                                                                                                               [ 47%]
helusers/tests/test_models.py::TestUserAdGroups::test_update_ad_groups[new_added] PASSED                                                                                                                                                                                                                                                                                          [ 47%]
helusers/tests/test_models.py::TestUserAdGroups::test_update_ad_groups[old_removed] PASSED                                                                                                                                                                                                                                                                                        [ 47%]
helusers/tests/test_models.py::TestUserAdGroups::test_update_ad_groups[mapped_twice_given_once] PASSED                                                                                                                                                                                                                                                                            [ 48%]
helusers/tests/test_models.py::TestUserAdGroups::test_update_ad_groups[mapped_twice_given_twice] PASSED                                                                                                                                                                                                                                                                           [ 48%]
helusers/tests/test_models.py::TestUserAdGroups::test_update_ad_groups[all_removed] PASSED

Copy link
Contributor

@charn charn left a comment

Choose a reason for hiding this comment

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

Added one minor comment, but LGTM! 👍 :shipit:

Test that the `update_ad_groups` function of the User-model works as
expected:

1. If the same group is mapped to multiple ad-groups and the ad-groups
input argument contains only some, the group is still persisted to user
instance since it was mapped to at least one of given ad-groups.

2. If the ad-groups input argument does not have any link to an existing
group of a user, the group is removed from the user instance.

3. If the ad-groups input argument contains links to new groups that the
user is not yet linked to, the group will be added to the user instance.
@nikomakela nikomakela force-pushed the test-user-update_ad_groups branch from 54417b7 to f3201ad Compare June 27, 2024 08:59
@nikomakela
Copy link
Contributor Author

Added one minor comment, but LGTM! 👍 :shipit:

@charn just a note that I don't have privileges to merge. The pr is from my fork because I didn't have privileges to add it to the real project.

@charn charn merged commit 19ffdc6 into City-of-Helsinki:master Jun 27, 2024
12 checks passed
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.

2 participants