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

zandercymatics · 2025-03-05T17:39:41Z

Ticket

Resolves #3528

Changes

Changes the error message on the add new member form to differ from the one on django admin

Context for reviewers

This PR adds an override for the error message on the new member form

Setup

The easiest way to test this is to try to invite yourself as a member on the portfolio form, this will trigger the first error message. To test the invitation message, you will need to remove the invite on the current portfolio and add that invite to another portfolio. This can be done easily via django admin. Then invite that portfolio to the current portfolio.

Code Review Verification Steps

As the original developer, I have

Satisfied acceptance criteria and met development standards

Met the acceptance criteria, or will meet them in a subsequent PR
Created/modified automated tests
Update documentation in READMEs and/or onboarding guide

Ensured code standards are met (Original Developer)

If any updated dependencies on Pipfile, also update dependencies in requirements.txt.
Interactions with external systems are wrapped in try/except
Error handling exists for unusual or missing values

Validated user-facing changes (if applicable)

Tag @dotgov-designers in this PR's Reviewers for design review. If code is not user-facing, delete design reviewer checklist
Verify new pages have been added to .pa11yci file so that they will be tested with our automated accessibility testing
Checked keyboard navigability
Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI)

As a code reviewer, I have

Reviewed, tested, and left feedback about the changes

Pulled this branch locally and tested it
Verified code meets all checks above. Address any checks that are not satisfied
Reviewed this code and left comments. Indicate if comments must be addressed before code is merged
Checked that all code is adequately covered by tests
Verify migrations are valid and do not conflict with existing migrations

Validated user-facing changes as a developer

Note: Multiple code reviewers can share the checklists above, a second reviewer should not make a duplicate checklist. All checks should be checked before approving, even those labeled N/A.

New pages have been added to .pa11yci file so that they will be tested with our automated accessibility testing
Checked keyboard navigability
Meets all designs and user flows provided by design/product
Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI)
(Rarely needed) Tested as both an analyst and applicant user

As a designer reviewer, I have

Verified that the changes match the design intention

Checked that the design translated visually
Checked behavior. Comment any found issues or broken flows.
Checked different states (empty, one, some, error)
Checked for landmarks, page heading structure, and links

Validated user-facing changes as a designer

Checked keyboard navigability
Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI)
Tested with multiple browsers (check off which ones were used)
- Chrome
- Microsoft Edge
- FireFox
- Safari
(Rarely needed) Tested as both an analyst and applicant user

References

Code review best practices

Screenshots

github-actions · 2025-03-05T17:44:08Z

🥳 Successfully deployed to developer sandbox za.

zandercymatics · 2025-03-05T17:57:38Z

src/registrar/forms/portfolio.py

@@ -445,3 +445,26 @@ class PortfolioNewMemberForm(BasePortfolioMemberForm):
    class Meta:
        model = PortfolioInvitation
        fields = ["portfolio", "email", "roles", "additional_permissions"]
+
+    def _post_clean(self):


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!

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.

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

github-actions · 2025-03-05T18:02:27Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2025-03-05T18:52:37Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2025-03-05T19:01:00Z

🥳 Successfully deployed to developer sandbox za.

src/registrar/forms/portfolio.py

github-actions · 2025-03-06T17:17:57Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2025-03-06T17:23:03Z

🥳 Successfully deployed to developer sandbox za.

Matt-Spence

One question, otherwise looks great

Matt-Spence · 2025-03-06T20:39:37Z

src/registrar/forms/portfolio.py

@@ -445,3 +445,26 @@ class PortfolioNewMemberForm(BasePortfolioMemberForm):
    class Meta:
        model = PortfolioInvitation
        fields = ["portfolio", "email", "roles", "additional_permissions"]
+
+    def _post_clean(self):


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.

Matt-Spence · 2025-03-06T20:41:51Z

src/registrar/forms/portfolio.py

+
+            # 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__"]


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

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

Matt-Spence

Nice work

github-actions · 2025-03-06T21:42:05Z

🥳 Successfully deployed to developer sandbox za.

zandercymatics added 2 commits March 5, 2025 10:23

Override error in certain contexts

d4bcd03

Update portfolio.py

88e8b2d

lint and fix tests

f4c48b5

zandercymatics changed the title ~~[draft] #3528: Fix error handling on new member form~~ [draft] #3528: Fix error handling on new member form - [ZA] Mar 5, 2025

zandercymatics commented Mar 5, 2025

View reviewed changes

zandercymatics requested review from lizpearl, therealslimhsiehdy, Matt-Spence, CocoByte, rachidatecs, allly-b, dave-kennedy-ecs and erinysong March 5, 2025 18:01

Update portfolio.py

330a246

zandercymatics requested review from Katherine-MN and a team March 5, 2025 18:55

zandercymatics changed the title ~~[draft] #3528: Fix error handling on new member form - [ZA]~~ #3528: Fix error handling on new member form - [ZA] Mar 5, 2025

Update test_views_portfolio.py

4d77e29

zandercymatics commented Mar 6, 2025

View reviewed changes

src/registrar/forms/portfolio.py Show resolved Hide resolved

Update src/registrar/forms/portfolio.py

9ded05f

Matt-Spence self-assigned this Mar 6, 2025

Matt-Spence reviewed Mar 6, 2025

View reviewed changes

Matt-Spence approved these changes Mar 6, 2025

View reviewed changes

Merge branch 'main' into za/3528-add-member-error-handling

b694a78

zandercymatics merged commit 04603ce into main Mar 6, 2025
10 checks passed

zandercymatics deleted the za/3528-add-member-error-handling branch March 6, 2025 21:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

zandercymatics commented Mar 5, 2025 •

edited

Loading

github-actions bot commented Mar 5, 2025

zandercymatics Mar 5, 2025 •

edited

Loading

Matt-Spence Mar 6, 2025

zandercymatics Mar 6, 2025

github-actions bot commented Mar 5, 2025

github-actions bot commented Mar 5, 2025

github-actions bot commented Mar 5, 2025

github-actions bot commented Mar 6, 2025

github-actions bot commented Mar 6, 2025

Matt-Spence left a comment

Matt-Spence Mar 6, 2025

Matt-Spence Mar 6, 2025

zandercymatics Mar 6, 2025 •

edited

Loading

Matt-Spence left a comment

github-actions bot commented Mar 6, 2025

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

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

Conversation

zandercymatics commented Mar 5, 2025 • edited Loading

Ticket

Changes

Context for reviewers

Setup

Code Review Verification Steps

As the original developer, I have

Satisfied acceptance criteria and met development standards

Ensured code standards are met (Original Developer)

Validated user-facing changes (if applicable)

As a code reviewer, I have

Reviewed, tested, and left feedback about the changes

Validated user-facing changes as a developer

As a designer reviewer, I have

Verified that the changes match the design intention

Validated user-facing changes as a designer

References

Screenshots

github-actions bot commented Mar 5, 2025

zandercymatics Mar 5, 2025 • edited Loading

Choose a reason for hiding this comment

Matt-Spence Mar 6, 2025

Choose a reason for hiding this comment

zandercymatics Mar 6, 2025

Choose a reason for hiding this comment

github-actions bot commented Mar 5, 2025

github-actions bot commented Mar 5, 2025

github-actions bot commented Mar 5, 2025

github-actions bot commented Mar 6, 2025

github-actions bot commented Mar 6, 2025

Matt-Spence left a comment

Choose a reason for hiding this comment

Matt-Spence Mar 6, 2025

Choose a reason for hiding this comment

Matt-Spence Mar 6, 2025

Choose a reason for hiding this comment

zandercymatics Mar 6, 2025 • edited Loading

Choose a reason for hiding this comment

Matt-Spence left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 6, 2025

zandercymatics commented Mar 5, 2025 •

edited

Loading

zandercymatics Mar 5, 2025 •

edited

Loading

zandercymatics Mar 6, 2025 •

edited

Loading