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

#3319: On UserPortfolioPermission save, add user to org_model_users when applicable - [EL] #3377

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions src/registrar/models/user_portfolio_permission.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
)
from .utility.time_stamped_model import TimeStampedModel
from django.contrib.postgres.fields import ArrayField
from django.contrib.auth.models import Group


class UserPortfolioPermission(TimeStampedModel):
Expand Down Expand Up @@ -186,3 +187,30 @@ def clean(self):
"""Extends clean method to perform additional validation, which can raise errors in django admin."""
super().clean()
validate_user_portfolio_permission(self)

def save(self, *args, **kwargs):
"""
Save override
Ensures that the user associated with this instance is added to the
'org_model_users' group under specific conditions.
"""
is_new_instance = self.pk is None # Check if this is a new instance being created

super().save(*args, **kwargs) # Call the original save method

if is_new_instance:
try:
# Fetch the 'org_model_users' group
org_model_users_group = Group.objects.get(name="org_model_users")
except Group.DoesNotExist:
org_model_users_group = None

if (
org_model_users_group # Ensure the group exists
and not self.user.groups.filter(
name="org_model_users"
).exists() # Check if the user is already in the group
Comment on lines +211 to +213
Copy link
Contributor

Choose a reason for hiding this comment

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

(optional) Might be nice to have a test for this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're gonna remove the org_model_users group logic per @abroddrick. I'll simplify this PR and will ping you back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, this makes this whole PR obsolete

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I think this whole PR is now obsolete

):
# Add the user to the group
self.user.groups.add(org_model_users_group)
60 changes: 60 additions & 0 deletions src/registrar/tests/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
from registrar.models.portfolio import Portfolio
from registrar.models.portfolio_invitation import PortfolioInvitation
from registrar.models.transition_domain import TransitionDomain
from registrar.models.user_group import UserGroup
from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices
from registrar.models.verified_by_staff import VerifiedByStaff # type: ignore

Expand Down Expand Up @@ -750,6 +751,65 @@ def test_get_forbidden_permissions_with_multiple_roles(self):
# Should return the forbidden permissions for member role
self.assertEqual(member_only_permissions, set(member_forbidden))

@less_console_noise_decorator
def test_save_new_instance_adds_perm_user_to_og_model_users_group(self):
"""
Saving a new instance of UserPortfolioPermission adds the UserPortfolioPermission.user
to the group org_model_users.

Saving an existing instance does not add the user to the group.
"""
# Create the org_model_users group
org_model_users, _ = UserGroup.objects.get_or_create(name="org_model_users")

# Ensure the test user does not belong to org_model_users
self.assertFalse(self.user.groups.filter(name="org_model_users").exists())

# Save a new UserPortfolioPermission instance
portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(
portfolio=self.portfolio,
user=self.user,
roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN],
)

# Assert that the user is now in org_model_users
self.assertTrue(self.user.groups.filter(name="org_model_users").exists())

# Remove the user from org_model_users
self.user.groups.remove(org_model_users)
self.assertFalse(self.user.groups.filter(name="org_model_users").exists())

# Modify the permissions and save again
portfolio_permission.roles = [UserPortfolioRoleChoices.ORGANIZATION_MEMBER]
portfolio_permission.save()

# Assert that the user has not been added back to org_model_users
self.assertFalse(self.user.groups.filter(name="org_model_users").exists())

@less_console_noise_decorator
def test_save_new_instance_when_org_model_users_group_does_not_exist(self):
"""
This mocks the lower environments.

In this instance, nothing should happen.
"""
# Ensure the org_model_users group does not exist
UserGroup.objects.filter(name="org_model_users").delete()
self.assertFalse(UserGroup.objects.filter(name="org_model_users").exists())

# Save a new UserPortfolioPermission instance
portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(
portfolio=self.portfolio,
user=self.user,
roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN],
)

# Assert that the portfolio_permission exists
self.assertIsNotNone(portfolio_permission)

# Assert that the org_model_users group still does not exist
self.assertFalse(UserGroup.objects.filter(name="org_model_users").exists())


class TestUser(TestCase):
"""Test actions that occur on user login,
Expand Down
Loading