Ticket #2756: Edit member access page - [ZA] #3216

zandercymatics · 2024-12-11T20:21:42Z

Ticket

Resolves #2765

Changes

Adds a member edit access page
Bug fix for the no domains access page

Context for reviewers

Setup

To test this PR, enable the organization_feature and organization_members flag. On members, click edit on a member.

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 · 2024-12-11T20:25:56Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-12-11T20:36:04Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-12-11T20:55:41Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-12-11T21:08:13Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-12-12T20:20:02Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-12-13T16:38:41Z

🥳 Successfully deployed to developer sandbox za.

zandercymatics · 2024-12-13T16:42:30Z

src/registrar/models/utility/portfolio_helper.py

@@ -16,7 +18,28 @@ class UserPortfolioRoleChoices(models.TextChoices):

    @classmethod
    def get_user_portfolio_role_label(cls, user_portfolio_role):


Unrelated bug I found in some weird data edge cases. Doesn't hurt to include

zandercymatics · 2024-12-13T16:44:09Z

src/registrar/models/utility/portfolio_helper.py

+            return f"Unknown ({user_portfolio_role})"
+
+    @classmethod
+    def get_role_description(cls, user_portfolio_role):


I usually don't like encoding large strings in python like this, but all things considered this seemed like the best spot for this sort of thing as opposed to a squirrelled away html file

github-actions · 2024-12-13T16:58:24Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-12-13T17:49:04Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-12-13T18:35:53Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-12-13T18:59:17Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-12-16T20:28:06Z

🥳 Successfully deployed to developer sandbox za.

CocoByte

Looking through the code...
Overall, nice work! I know we talked through some of the changes in person, but I added a few comments anyways to indicate I am good with alot of the refactoring.

Just a few requests to add code comments.

CocoByte · 2024-12-17T17:27:34Z

src/registrar/admin.py

@@ -4004,6 +4004,10 @@ def changelist_view(self, request, extra_context=None):
        if extra_context is None:
            extra_context = {}
        extra_context["dns_prototype_flag"] = flag_is_active_for_user(request.user, "dns_prototype_flag")
+        # Normally you have to first enable the org feature then navigate to an org before you see these.
+        # Lets just auto-populate it on page load to make development easier.


CocoByte · 2024-12-17T17:29:39Z

src/registrar/assets/src/js/getgov/portfolio-member-page.js

-        populatePermissionDetails('new-member-basic-permissions');
+      if (selectedAccess && selectedAccess.value === 'organization_admin') {
+        populatePermissionDetails('member-admin-permissions');
+        accessText = "Admin"


I like this separation of data and display text

CocoByte · 2024-12-17T17:32:38Z

src/registrar/assets/src/js/getgov/main.js

@@ -44,5 +36,7 @@ initMembersTable();
 initMemberDomainsTable();
 initEditMemberDomainsTable();

-initPortfolioMemberPageToggle();
+// Init the portfolio new member page
+initPortfolioMemberPageRadio();


I feel like we should start bundling inits()

Yes, I agree. On a few of the other tickets we have worked on recently, we have consolidated inits to one init per page in the main.js.

CocoByte · 2024-12-17T17:33:46Z

src/registrar/assets/src/js/getgov/radios.js

@@ -38,21 +38,21 @@ export function hookupYesNoListener(radioButtonName, elementIdToShowIfYes, eleme
 **/
 export function hookupRadioTogglerListener(radioButtonName, valueToElementMap) {
    // Get the radio buttons
-    let radioButtons = document.querySelectorAll('input[name="'+radioButtonName+'"]');
+    let radioButtons = document.querySelectorAll(`input[name="${radioButtonName}"]`);


Nice cleanup

CocoByte · 2024-12-17T17:37:27Z

src/registrar/forms/portfolio.py

        return cleaned_data
+
+    def map_cleaned_data_to_instance(self, cleaned_data, instance):


(Change Request): Can you add more comments here to explain this architecture? I know we went over it, but it can be hard to know at first glance what is going on if you are looking at it for the first time.

CocoByte · 2024-12-17T17:43:19Z

src/registrar/models/user_portfolio_permission.py

@@ -110,7 +110,7 @@ def _get_portfolio_permissions(self):
        return self.get_portfolio_permissions(self.roles, self.additional_permissions)

    @classmethod
-    def get_portfolio_permissions(cls, roles, additional_permissions):
+    def get_portfolio_permissions(cls, roles, additional_permissions, get_list=True):


(Change Request): Add comment to quickly describe the usage of get_list

CocoByte · 2024-12-17T17:53:53Z

src/registrar/templates/portfolio_members_add_new.html

-    {% endfor %}
-  </div>
+  {% with add_class="usa-radio__input--tile" add_legend_class="usa-sr-only" %}
+    {% input_with_errors form.role %}


Definitely cleaner!

CocoByte · 2024-12-17T17:54:03Z

src/registrar/models/utility/portfolio_helper.py

+            return f"Unknown ({user_portfolio_role})"
+
+    @classmethod
+    def get_role_description(cls, user_portfolio_role):


CocoByte · 2024-12-17T17:55:45Z

src/registrar/templates/portfolio_members_add_new.html

    <h2>Admin access permissions</h2>
    <p>Member permissions available for admin-level acccess.</p>

    <h3 class="summary-item__title
    text-primary-dark
    margin-bottom-0">Organization domain requests</h3>
    {% with group_classes="usa-form-editable usa-form-editable--no-border padding-top-0" %}
-      {% input_with_errors form.admin_org_domain_request_permissions %}
+      {% input_with_errors form.domain_request_permission_admin %}


I like this name better, too

CocoByte · 2024-12-17T17:56:51Z

src/registrar/views/portfolios.py

        if form.is_valid():
+            # Check if user is removing their own admin or edit role


CocoByte · 2024-12-17T21:30:24Z

@dave-kennedy-ecs, @rachidatecs -- looks like this refactor causes a conflict in the logic for e-mail sending behavior (the ticket you guys are working on). Let Zander and I know if you'd like to huddle on this. I think fundamentally it is just deciding where we want this logic to be and coordinating that part of the code.

github-actions · 2024-12-18T18:49:36Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-12-18T18:55:23Z

🥳 Successfully deployed to developer sandbox za.

zandercymatics · 2024-12-18T18:58:01Z

src/registrar/templates/portfolio_no_domains.html

-{% block messages %}
-    {% include "includes/form_messages.html" %}
-{% endblock %}


Fixes a bug where messages display twice on this page due to base views implementing this

github-actions · 2024-12-18T18:59:46Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-12-18T19:04:03Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-12-18T19:11:07Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-12-18T19:13:32Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-12-18T19:34:19Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-12-18T19:39:58Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-12-18T20:06:54Z

🥳 Successfully deployed to developer sandbox za.

dave-kennedy-ecs

LGTM

github-actions · 2024-12-18T21:47:15Z

🥳 Successfully deployed to developer sandbox za.

zandercymatics added 6 commits December 9, 2024 10:38

Copy template

8dfb183

Form structure

ed9d215

fine comb

bc3a96a

Finish logic for permission setting

ae770ab

add logic for invitation

2bc8d38

Consolidate

0ab963d

zandercymatics requested a review from abroddrick as a code owner December 11, 2024 20:21

Fix js

4f3663e

wrap up js

9fe87fb

Consolidate js

6662d82

Rework NewMemberView (will readd email logic)

d7ec32d

zandercymatics added 3 commits December 12, 2024 15:14

Add stub for email adding

f6e95cc

Update portfolios.py

2d0c4e7

Merge branch 'main' into za/2756-edit-member-access

ba170c3

zandercymatics commented Dec 13, 2024

View reviewed changes

cleanup

d48c140

the perpetual struggle

d52a44a

fix tests + cleanup

d214948

Polish of paint

24480ec

Merge branch 'main' into za/2756-edit-member-access

c5184e5

rachidatecs self-assigned this Dec 17, 2024

CocoByte reviewed Dec 17, 2024

View reviewed changes

Remove changes to newmember

437981f

zandercymatics added 2 commits December 18, 2024 11:51

cleanup pt 2

08082fb

Update portfolio-member-page.js

6965607

zandercymatics commented Dec 18, 2024

View reviewed changes

Fix some merge things

adfd6be

zandercymatics added 2 commits December 18, 2024 12:06

Readd modelforms

c348089

Update portfolio.py

a9923f4

zandercymatics added 3 commits December 18, 2024 12:25

PR suggestions

f19ff3c

Update portfolio.py

0e1d07d

Update portfolio.py

a314649

Condense logic and change names

dec9e33

dave-kennedy-ecs approved these changes Dec 18, 2024

View reviewed changes

remove log

297be33

zandercymatics merged commit f475f4a into main Dec 18, 2024
9 of 10 checks passed

zandercymatics deleted the za/2756-edit-member-access branch December 18, 2024 22:08

		@@ -16,7 +18,28 @@ class UserPortfolioRoleChoices(models.TextChoices):

		@classmethod
		def get_user_portfolio_role_label(cls, user_portfolio_role):

		return cleaned_data

		def map_cleaned_data_to_instance(self, cleaned_data, instance):

		if form.is_valid():
		# Check if user is removing their own admin or edit role

Ticket #2756: Edit member access page - [ZA] #3216

Ticket #2756: Edit member access page - [ZA] #3216

Conversation

zandercymatics commented Dec 11, 2024 • 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 Dec 11, 2024

github-actions bot commented Dec 11, 2024

github-actions bot commented Dec 11, 2024

github-actions bot commented Dec 11, 2024

github-actions bot commented Dec 12, 2024

github-actions bot commented Dec 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 13, 2024

github-actions bot commented Dec 13, 2024

github-actions bot commented Dec 13, 2024

github-actions bot commented Dec 13, 2024

github-actions bot commented Dec 16, 2024

CocoByte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CocoByte commented Dec 17, 2024

github-actions bot commented Dec 18, 2024

github-actions bot commented Dec 18, 2024

Choose a reason for hiding this comment

github-actions bot commented Dec 18, 2024

github-actions bot commented Dec 18, 2024

github-actions bot commented Dec 18, 2024

github-actions bot commented Dec 18, 2024

github-actions bot commented Dec 18, 2024

github-actions bot commented Dec 18, 2024

github-actions bot commented Dec 18, 2024

dave-kennedy-ecs left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 18, 2024

zandercymatics commented Dec 11, 2024 •

edited

Loading