#2402: Design Review for Analyst Portfolio View - [ZA] - MIGRATION #2842

zandercymatics · 2024-09-23T17:29:51Z

Ticket

Resolves #2402

Changes

All of the following changes are in django admin.

Portfolio table:

Added an "organization_type" and "federal_type" columns
Removed the "federal_agency" column
Changed the help text under the search bar to "Search by organization name."

On the portfolio page:

Added view, edit, and add permissions for analysts
Made the "organization_name" field readonly for analysts
Added a details table for administrators, members, domain requests, and domains. These all now link either to the table or to the individual record.
Changed the order of portfolio fields
Dynamically show/hide the organization name field depending on the federal type. It is hidden if it is not portfolio.
Added a "create a new senior official" button if none is auto-populated.
Tweaked existing javascript to account for these changes

On the UserPortfolioPermission page:

Added a "roles" table
Added a search bar
Changed the str display of UserPortfolioPermission

On the federal agencies field:

Removed the helper text below federal_type
Renamed "initials" to "acronym"
- Updated helper text to read: "Acronym commonly used to reference the federal agency (optional) "
Updated the helper text for "FCEB" to read: "Federal Civilian Executive Branch (FCEB)

On the suborganization page:

Added a search helper text that reads: "Search by suborganization."
Updated the "Name" field to read "Suborganization" (verbose_name)
Removed the helper text below the "Suborganization" field
If there are no requests or domains associated with a suborganization, display "No domains" or "No domain requests" below those headings.

On the User page:

Updated the blue-bar heading for "Portfolio information" to read: "Associated portfolios"
Within that section, the display of "portfolio" now matches the styling we use for "suborganizations" on the portfolio page
Rather than hiding the "Associated portfolios" section if the user isn't associated with one, we just show the text "no portfolios"
If there are no requests or domains associated with a user, display "No domains" or "No domain requests" below those headings.

On the Domain information / Domain request pages:

Updated helper for portfolio text to read: “If blank, request is not associated with a portfolio.”
"Sub organization" is now displayed as "Suborganization"
Updated helper text for suborganization to read: “If blank, request is associated with the overarching organization for this portfolio."

Context for reviewers

This PR addresses changes related to the design review of the analyst portfolio view. So - essentially milestone features that touch on /admin. You will need to test the changes detailed above.

Setup

Visit and test the following pages in django admin as an analyst:

The portfolio table
The portfolio page
The UserPermissionRole page
The suborganization page
The user page
The domain request page
The domain page

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
Added at least 2 developers as PR reviewers (only 1 will need to approve)
Messaged on Slack or in standup to notify the team that a PR is ready for review
Changes to “how we do things” are documented in READMEs and or onboarding guide
If any model was updated to modify/add/delete columns, makemigrations was ran and the associated migrations file has been commited.

Ensured code standards are met (Original Developer)

All new functions and methods are commented using plain language
Did dependency updates in Pipfile also get changed 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)

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)
Add at least 1 designer as PR reviewer

As a code reviewer, I have

Reviewed, tested, and left feedback about the changes

Pulled this branch locally and tested it
Reviewed this code and left comments
Checked that all code is adequately covered by tests
Made it clear which comments need to be addressed before this work is merged
If any model was updated to modify/add/delete columns, makemigrations was ran and the associated migrations file has been commited.

Ensured code standards are met (Code reviewer)

All new functions and methods are commented using plain language
Interactions with external systems are wrapped in try/except
Error handling exists for unusual or missing values
(Rarely needed) Did dependency updates in Pipfile also get changed in requirements.txt?

Validated user-facing changes as a developer

Note: Multiple code reviewers can share the checklists above, a second reviewers should not make a duplicate checklist

As a designer reviewer, I have

Verified that the changes match the design intention

Checked that the design translated visually
Checked behavior
Checked different states (empty, one, some, error)
Checked for landmarks, page heading structure, and links
Tried to break the intended flow

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

Screenshots

github-actions · 2024-09-23T17:45:29Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-09-23T17:51:58Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-09-23T18:25:14Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-09-23T18:30:35Z

🥳 Successfully deployed to developer sandbox za.

zandercymatics · 2024-09-23T18:54:22Z

@Katherine-Osos All those changes should be in!

github-actions · 2024-09-30T18:10:54Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-09-30T18:32:29Z

🥳 Successfully deployed to developer sandbox za.

asaki222 · 2024-09-30T19:22:14Z

src/registrar/admin.py

+    get_all_action_needed_reason_emails,
+    get_action_needed_reason_default_email,
+    get_field_links_as_list,
+)
 from registrar.models import Contact, Domain, DomainRequest, DraftDomain, User, Website, SeniorOfficial


This is great cleanup here~

asaki222 · 2024-10-01T04:59:40Z

src/registrar/admin.py

+        return get_field_links_as_list(queryset, "portfolio", msg_for_none="No portfolios.")
+
+    portfolios.short_description = "Portfolios"  # type: ignore
+


what does '#type: ignore' mean?

Its for ignoring type checking with our linter. Its just a bit overzealous here so we've been doing that on these particular functions

asaki222 · 2024-10-01T05:28:42Z

src/registrar/admin.py

@@ -3052,7 +3039,7 @@ class PortfolioAdmin(ListHeaderAdmin):
        ("Senior official", {"fields": ["senior_official"]}),
    ]

-    list_display = ("organization_name", "federal_agency", "creator")
+    list_display = ("organization_name", "organization_type", "federal_type", "creator")
    search_fields = ["organization_name"]
    search_help_text = "Search by organization name."
    readonly_fields = [


on the changes description, it says that help text is Search by portfolio organization

Ah sorry - this is correct! I just forgot to fix that in the ticket. We eventually renamed it to this. Great eye!

src/registrar/assets/js/get-gov-admin.js

asaki222 · 2024-10-01T06:13:25Z

src/registrar/assets/js/get-gov-admin.js

            }
        })
        .catch(error => console.error("Error fetching senior official: ", error));

    }

+    function updateSeniorOfficialDropdown(dropdown, seniorOfficialId, seniorOfficialName) {
+        if (!seniorOfficialId || !seniorOfficialName || !seniorOfficialName.trim()){
+            // Clear the field if the SO doesn't exist


this might be a nitpick. Would !seniorOfficialName and !seniorOfficialName.trim() both pass if seniorOfficialName was invalid?

If the senior official name contains a space, I think it would evaluate to true. Its basically catching that particular edge case.

I'd have to look again but I think the compiler knows how to optimize this such that if !seniorOfficialName, then it'll just enter the loop without computing the rest. It works this way in python as you can do if something and something.field and something.field.another_field and it doesn't error out

Katherine-Osos · 2024-10-01T22:15:10Z

@zandercymatics On the Domain Request and Domains pages … I changed my mind about the “Portfolio” helper text. Let’s remove that completely; it seems unnecessary (but keep the helper text for “Suborganization.”)

Other than that, this looks great! I'll approve once that helper text is removed!

github-actions · 2024-10-02T15:00:54Z

🥳 Successfully deployed to developer sandbox za.

asaki222

LGTM

zandercymatics · 2024-10-02T15:36:26Z

@Katherine-Osos Removed! Can you take another look?

Katherine-Osos

Woot woot! Looks great, @zandercymatics!

github-actions · 2024-10-03T18:59:18Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-10-03T19:10:38Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-10-04T15:42:19Z

🥳 Successfully deployed to developer sandbox za.

Matt-Spence

Good stuff

Matt-Spence · 2024-10-03T19:45:10Z

src/registrar/assets/js/get-gov-admin.js

+                    let seniorOfficialLink = `<a href=/admin/registrar/seniorofficial/${seniorOfficialId}/change/>${seniorOfficialName}</a>`
+                    readonlySeniorOfficial.innerHTML = seniorOfficialName ? seniorOfficialLink : "-";


This seniorOfficialId isn't user-supplied data is it? I'm a bit fuzzy on what the api call above will return, so I just want to check what the data.id that's being used here actually is. We need to be careful when supplying data in href's that will be passed into an innerHTML object like it is below, as it creates a possible vector for cross-site-scripting attacks. I don't think it's an issue here but want to double check.

This seniorOfficialId isn't user-supplied data is it

Nope, this is the PK of the senior official field. I believe it gets pulled from the model. Since only superusers / analysts can access this field (and since it pulls from a table they already have access to) I figured this approach would be OK.

I'm a bit fuzzy on what the api call above will return

Check out the get_senior_official_from_federal_agency_json function. It basically just returns a json representation of the senior official object which they would normally have access to on the senior official table

github-actions · 2024-10-04T16:03:37Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-10-04T16:27:43Z

🥳 Successfully deployed to developer sandbox za.

zandercymatics added 5 commits September 20, 2024 14:32

Expose portfolios to analysts

5d2fec8

review changes: minor tweaks

887af43

Add bold for organization label

83615cf

Merge branch 'main' into za/2402-design-review

afe4989

Fix migrations + readonly views

280bc23

zandercymatics requested a review from abroddrick as a code owner September 23, 2024 17:29

zandercymatics added 2 commits September 23, 2024 11:41

Merge branch 'main' into za/2402-design-review

1ec548a

Update _admin.scss

4597133

zandercymatics marked this pull request as draft September 23, 2024 17:42

lint + tests

aa926ec

zandercymatics added 2 commits September 23, 2024 12:20

Fix tests and lint

d822714

Update admin.py

9ccea3f

zandercymatics marked this pull request as ready for review September 24, 2024 14:11

zandercymatics changed the title ~~(draft) Issue #2402: design review - [ZA]~~ (draft) Issue #2402: Design Review for Analyst Portfolio View- [ZA] Sep 24, 2024

zandercymatics requested review from therealslimhsiehdy, Matt-Spence, CocoByte, rachidatecs, dave-kennedy-ecs, erinysong, asaki222, Katherine-Osos and a team and removed request for a team and Katherine-Osos September 24, 2024 16:46

Matt-Spence self-assigned this Sep 30, 2024

zandercymatics changed the title ~~Issue #2402: Design Review for Analyst Portfolio View - [ZA] - MIGRATIONS~~ #2402: Design Review for Analyst Portfolio View - [ZA] - MIGRATIONS Sep 30, 2024

zandercymatics changed the title ~~#2402: Design Review for Analyst Portfolio View - [ZA] - MIGRATIONS~~ #2402: Design Review for Analyst Portfolio View - [ZA] - MIGRATION Sep 30, 2024

Fix migrations

1122edc

fix test

0e7969f

asaki222 reviewed Sep 30, 2024

View reviewed changes

asaki222 self-assigned this Sep 30, 2024

asaki222 reviewed Oct 1, 2024

View reviewed changes

src/registrar/assets/js/get-gov-admin.js Show resolved Hide resolved

asaki222 reviewed Oct 1, 2024

View reviewed changes

Remove helper text

f67c4e0

asaki222 reviewed Oct 2, 2024

View reviewed changes

Katherine-Osos approved these changes Oct 2, 2024

View reviewed changes

Merge branch 'main' into za/2402-design-review

2f8408e

fix migrations (merge conflict

8d0c1ba

Matt-Spence approved these changes Oct 4, 2024

View reviewed changes

Merge branch 'main' into za/2402-design-review

a219521

zandercymatics merged commit 14e8150 into main Oct 4, 2024
10 checks passed

zandercymatics deleted the za/2402-design-review branch October 4, 2024 16:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#2402: Design Review for Analyst Portfolio View - [ZA] - MIGRATION #2842

#2402: Design Review for Analyst Portfolio View - [ZA] - MIGRATION #2842

zandercymatics commented Sep 23, 2024 •

edited

Loading

github-actions bot commented Sep 23, 2024

github-actions bot commented Sep 23, 2024

github-actions bot commented Sep 23, 2024

github-actions bot commented Sep 23, 2024

zandercymatics commented Sep 23, 2024

github-actions bot commented Sep 30, 2024

github-actions bot commented Sep 30, 2024

asaki222 Sep 30, 2024

asaki222 Oct 1, 2024

zandercymatics Oct 1, 2024

asaki222 Oct 1, 2024

zandercymatics Oct 1, 2024 •

edited

Loading

asaki222 Oct 1, 2024

zandercymatics Oct 1, 2024 •

edited

Loading

Katherine-Osos commented Oct 1, 2024

github-actions bot commented Oct 2, 2024

asaki222 left a comment

zandercymatics commented Oct 2, 2024

Katherine-Osos left a comment

github-actions bot commented Oct 3, 2024

github-actions bot commented Oct 3, 2024

github-actions bot commented Oct 4, 2024

Matt-Spence left a comment

Matt-Spence Oct 3, 2024

zandercymatics Oct 4, 2024 •

edited

Loading

github-actions bot commented Oct 4, 2024

github-actions bot commented Oct 4, 2024

		return get_field_links_as_list(queryset, "portfolio", msg_for_none="No portfolios.")

		portfolios.short_description = "Portfolios" # type: ignore

		let seniorOfficialLink = `<a href=/admin/registrar/seniorofficial/${seniorOfficialId}/change/>${seniorOfficialName}</a>`
		readonlySeniorOfficial.innerHTML = seniorOfficialName ? seniorOfficialLink : "-";

#2402: Design Review for Analyst Portfolio View - [ZA] - MIGRATION #2842

#2402: Design Review for Analyst Portfolio View - [ZA] - MIGRATION #2842

Conversation

zandercymatics commented Sep 23, 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

Ensured code standards are met (Code reviewer)

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

Screenshots

github-actions bot commented Sep 23, 2024

github-actions bot commented Sep 23, 2024

github-actions bot commented Sep 23, 2024

github-actions bot commented Sep 23, 2024

zandercymatics commented Sep 23, 2024

github-actions bot commented Sep 30, 2024

github-actions bot commented Sep 30, 2024

asaki222 Sep 30, 2024

Choose a reason for hiding this comment

asaki222 Oct 1, 2024

Choose a reason for hiding this comment

zandercymatics Oct 1, 2024

Choose a reason for hiding this comment

asaki222 Oct 1, 2024

Choose a reason for hiding this comment

zandercymatics Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

asaki222 Oct 1, 2024

Choose a reason for hiding this comment

zandercymatics Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Katherine-Osos commented Oct 1, 2024

github-actions bot commented Oct 2, 2024

asaki222 left a comment

Choose a reason for hiding this comment

zandercymatics commented Oct 2, 2024

Katherine-Osos left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 3, 2024

github-actions bot commented Oct 3, 2024

github-actions bot commented Oct 4, 2024

Matt-Spence left a comment

Choose a reason for hiding this comment

Matt-Spence Oct 3, 2024

Choose a reason for hiding this comment

zandercymatics Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Oct 4, 2024

github-actions bot commented Oct 4, 2024

zandercymatics commented Sep 23, 2024 •

edited

Loading

zandercymatics Oct 1, 2024 •

edited

Loading

zandercymatics Oct 1, 2024 •

edited

Loading

zandercymatics Oct 4, 2024 •

edited

Loading