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.

Sign up for GitHub

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

#3528: Fix error handling on new member form - [ZA] #3606

Merged
merged 7 commits into from
Mar 6, 2025
+36 −14
Merged

#3528: Fix error handling on new member form - [ZA] #3606

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d4bcd03
Override error in certain contexts
zandercymatics Mar 5, 2025
88e8b2d
Update portfolio.py
zandercymatics Mar 5, 2025
f4c48b5
lint and fix tests
zandercymatics Mar 5, 2025
330a246
Update portfolio.py
zandercymatics Mar 5, 2025
4d77e29
Update test_views_portfolio.py
zandercymatics Mar 5, 2025
9ded05f
Update src/registrar/forms/portfolio.py
zandercymatics Mar 6, 2025
b694a78
Merge branch 'main' into za/3528-add-member-error-handling
zandercymatics Mar 6, 2025
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
25 changes: 25 additions & 0 deletions src/registrar/forms/portfolio.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,3 +445,28 @@ class PortfolioNewMemberForm(BasePortfolioMemberForm):
class Meta:
model = PortfolioInvitation
fields = ["portfolio", "email", "roles", "additional_permissions"]

def _post_clean(self):
Copy link
Contributor Author

@zandercymatics zandercymatics Mar 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

An alternative to this approach would be to modify the error itself in portfolio_helper. The reason why this is not done is because there is a separate AC which requires a certain message on django admin.

This is the earliest hook I could find to modify this, but if you know of something earlier let me know!

Copy link
Contributor

Choose a reason for hiding this comment

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

The only other place I can think to do this would be in the PortfolioAddMemberView before the form even submits. That said, I honestly don't know if that would be any better and I have no problem with this current approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it's tricky, I spent a long time staring at Django's codebase trying to figure this one out. Thanks for the feedback here

"""
Override _post_clean to customize model validation errors.
This runs after form clean is complete, but before the errors are displayed.
"""
try:
super()._post_clean()
self.instance.clean()
except forms.ValidationError as e:
override_error = False
if hasattr(e, "code"):
field = "email" if "email" in self.fields else None
if e.code == "has_existing_permissions":
self.add_error(field, f"{self.instance.email} is already a member of another .gov organization.")
override_error = True
elif e.code == "has_existing_invitations":
self.add_error(
field, f"{self.instance.email} has already been invited to another .gov organization."
)
override_error = True

# Errors denoted as "__all__" are special error types reserved for the model level clean function
if override_error and "__all__" in self._errors:
del self._errors["__all__"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this error be re-raised at the end here in case none of those conditionals hit?

Copy link
Contributor Author

@zandercymatics zandercymatics Mar 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Nah, not needed. But good thoughts. One con of this approach is that super()._post_clean() does generate the error list on its own. self.instance.clean() is technically code duplication, but it is low-cost. This logic essentially works by generating the error list twice, and hot-swapping the error.

The pro of this approach is that all other error types are "carried through" so even if the underlying clean changes, everything will work normally as it seems like post clean doesn't rely on try/catch. Only one error is raised at a time here, so we can get away with doing this.

The problem I was facing is that _post_clean is not defined normally, its generated on the fly based off of rules I do not fully understand as of now. In other words, its very much an internal function that would require inheriting at a very low level

13 changes: 9 additions & 4 deletions src/registrar/models/utility/portfolio_helper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,8 @@ def validate_user_portfolio_permission(user_portfolio_permission):
if existing_permissions.exists():
raise ValidationError(
"This user is already assigned to a portfolio. "
"Based on current waffle flag settings, users cannot be assigned to multiple portfolios."
"Based on current waffle flag settings, users cannot be assigned to multiple portfolios.",
code="has_existing_permissions",
)

existing_invitations = PortfolioInvitation.objects.filter(email=user_portfolio_permission.user.email).exclude(
Expand All @@ -302,7 +303,8 @@ def validate_user_portfolio_permission(user_portfolio_permission):
if existing_invitations.exists():
raise ValidationError(
"This user is already assigned to a portfolio invitation. "
"Based on current waffle flag settings, users cannot be assigned to multiple portfolios."
"Based on current waffle flag settings, users cannot be assigned to multiple portfolios.",
code="has_existing_invitations",
)


Expand Down Expand Up @@ -350,6 +352,7 @@ def validate_portfolio_invitation(portfolio_invitation):

# == Validate the multiple_porfolios flag. == #
user = User.objects.filter(email=portfolio_invitation.email).first()

# If user returns None, then we check for global assignment of multiple_portfolios.
# Otherwise we just check on the user.
if not flag_is_active_for_user(user, "multiple_portfolios"):
Expand All @@ -362,13 +365,15 @@ def validate_portfolio_invitation(portfolio_invitation):
if existing_permissions.exists():
raise ValidationError(
"This user is already assigned to a portfolio. "
"Based on current waffle flag settings, users cannot be assigned to multiple portfolios."
"Based on current waffle flag settings, users cannot be assigned to multiple portfolios.",
code="has_existing_permissions",
)

if existing_invitations.exists():
raise ValidationError(
"This user is already assigned to a portfolio invitation. "
"Based on current waffle flag settings, users cannot be assigned to multiple portfolios."
"Based on current waffle flag settings, users cannot be assigned to multiple portfolios.",
code="has_existing_invitations",
)


Expand Down
12 changes: 2 additions & 10 deletions src/registrar/tests/test_views_portfolio.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3873,11 +3873,7 @@ def test_member_invite_for_previously_invited_member(self, mock_send_email):
# verify messages
self.assertContains(
response,
(
"This user is already assigned to a portfolio invitation. "
"Based on current waffle flag settings, users cannot be assigned "
"to multiple portfolios."
),
f"{self.invited_member_email} has already been invited to another .gov organization.",
)

# Validate Database has not changed
Expand Down Expand Up @@ -3915,11 +3911,7 @@ def test_member_invite_for_existing_member(self, mock_send_email):
# Verify messages
self.assertContains(
response,
(
"This user is already assigned to a portfolio. "
"Based on current waffle flag settings, users cannot be "
"assigned to multiple portfolios."
),
f"{self.user.email} is already a member of another .gov organization.",
)

# Validate Database has not changed
Expand Down