Issue #2975 - domain and domain info portfolio fields -[HOTGOV] #3044

CocoByte · 2024-11-07T16:50:27Z

Ticket 2975

Resolves #2975

Changes

Added @Property functions to DomainInfo to pull data from Portfolio when a portfolio is present
Updated admin.py so that DomainInfo and Domain tables in /admin use the converted fields in their columns and filters
Updated csv exports for the following;
- Domain, Domain Info, and Domain Request admin tables
- All analytic tables
Updated unit tests to use converted fields

Context for reviewers

Setup

Code Review Verification Steps

Go to /admin and verify that the following csv exports use converted fields instead of the native data for Domains, Domain Requests, and Domain Info;

Domains table
Domain Requests table
Domain Info table
All analytics csv exports

Also verify that the analytics displays use converted fields.

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

Filter

…main-info-portfolio-fields

github-actions · 2024-11-07T16:54:51Z

🥳 Successfully deployed to developer sandbox nl.

…main-info-portfolio-fields

github-actions · 2024-11-07T21:37:28Z

🥳 Successfully deployed to developer sandbox nl.

github-actions · 2024-11-13T08:26:28Z

🥳 Successfully deployed to developer sandbox nl.

src/registrar/admin.py

abroddrick

See comments for changes requested and comments. Also I was only running this locally and already noticing a significant slowdown in performance. Can you please put this on a sandbox so we can see if the slow down is just a local oriented thing? If you want to use mine you can.

src/registrar/admin.py

src/registrar/models/domain_request.py

src/registrar/models/domain_information.py

src/registrar/admin.py

src/registrar/tests/test_reports.py

github-actions · 2024-12-03T22:07:38Z

🥳 Successfully deployed to developer sandbox nl.

github-actions · 2024-12-04T08:21:58Z

🥳 Successfully deployed to developer sandbox nl.

zandercymatics

The custom fields don't appear to be sortable on the domain request or domain information tables, though they appear to be working on domains directly

src/registrar/admin.py

src/registrar/tests/test_reports.py

src/registrar/utility/csv_export.py

zandercymatics · 2024-12-04T16:22:04Z

src/registrar/utility/csv_export.py

@@ -262,6 +243,25 @@ def get_annotated_queryset(cls, **kwargs):
    @classmethod
    def get_model_annotation_dict(cls, **kwargs):
        return convert_queryset_to_dict(cls.get_annotated_queryset(**kwargs), is_model=False)
+
+    @classmethod
+    def export_data_to_csv(cls, csv_file, **kwargs):


(optional / nitpick) mind moving this back to line 216?

src/registrar/utility/csv_export.py

…main-info-portfolio-fields

github-actions · 2024-12-05T04:06:12Z

🥳 Successfully deployed to developer sandbox nl.

…main-info-portfolio-fields

github-actions · 2024-12-05T17:52:05Z

🥳 Successfully deployed to developer sandbox nl.

github-actions · 2024-12-05T17:54:12Z

🥳 Successfully deployed to developer sandbox nl.

zandercymatics

See comments, but should be good to merge otherwise. I do recommend a second pair of eyes on this one but up to you on that.

There is a performance slowdown on/admin I think associated with the lookups (for loops), but at least locally/sandboxes it seemed okay. That said, I think we should tackle that in a follow-on as this ticket has already gone through a few iterations of that

zandercymatics · 2024-12-05T18:53:38Z

src/registrar/models/domain_information.py

        if self.portfolio:
-            return self.portfolio.federal_agency
-        return self.federal_agency
+            return self.portfolio.federal_agency.agency


(nitpick) shouldn't we check on self.portfolio and self.portfolio.federal_agency?

zandercymatics · 2024-12-05T18:58:09Z

src/registrar/tests/test_reports.py

-            "adomain10.gov,Ready,2024-04-03,(blank),Federal,Armed Forces Retirement Home,,,,(blank),,,,"
+            "adomain10.gov,Ready,2024-04-03,(blank),Federal,Armed Forces Retirement Home,,,, ,,(blank),,"


It seems like a errant space is being added here, for some reason and on the other tests (see just before (blank))

zandercymatics · 2024-12-05T19:04:46Z

src/registrar/tests/test_reports.py

+            "adomain2.gov,Dns needed,(blank),(blank),Federal - Executive,"
+            "Portfolio 1 Federal Agency,,,, ,,(blank),"


this was originally: "adomain2.gov,Dns needed,(blank),(blank),Interstate,,,,,(blank),,,"
It seems like the agency type changed from interstate to federal - executive. Is this expected?

Yes, I editted the data to make it clear when we are pulling from portfolio data or not. adomain2 is affected across all these tests

src/registrar/tests/test_reports.py

src/registrar/utility/csv_export.py

src/registrar/admin.py

…main-info-portfolio-fields

comments addressed and subsequent reviews have been made

github-actions · 2024-12-05T21:43:44Z

🥳 Successfully deployed to developer sandbox nl.

github-actions · 2024-12-05T21:58:23Z

🥳 Successfully deployed to developer sandbox nl.

github-actions · 2024-12-06T00:13:48Z

🥳 Successfully deployed to developer sandbox nl.

CocoByte added 5 commits November 1, 2024 17:37

Initial draft

9f9147e

WIP on nl/2975-domain-and-domain-info-portfolio-fields

7292926

Filter

cleanup stray readonly field

0ac4e57

Merge remote-tracking branch 'origin/main' into nl/2975-domain-and-do…

dcc0a51

…main-info-portfolio-fields

updates to DomainAdmin

b0e1b5a

CocoByte requested a review from abroddrick as a code owner November 7, 2024 16:50

CocoByte added 2 commits November 7, 2024 11:01

revert read-only field

ae0eb45

Merge remote-tracking branch 'origin/main' into nl/2975-domain-and-do…

c481c3c

…main-info-portfolio-fields

CocoByte added 2 commits November 13, 2024 01:16

export updates

32232c5

Remove logs

ba06660

CocoByte changed the title ~~[DRAFT] Issue #2975 - domain and domain info portfolio fields~~ Issue #2975 - domain and domain info portfolio fields -[sandbox pending] Nov 14, 2024

CocoByte changed the title ~~Issue #2975 - domain and domain info portfolio fields -[sandbox pending]~~ Issue #2975 - domain and domain info portfolio fields -[NL] Nov 14, 2024

CocoByte changed the title ~~Issue #2975 - domain and domain info portfolio fields -[NL]~~ Issue #2975 - domain and domain info portfolio fields -[sandbox pending] Nov 14, 2024

CocoByte requested review from therealslimhsiehdy, Matt-Spence, rachidatecs, dave-kennedy-ecs, erinysong, asaki222 and zandercymatics November 20, 2024 05:43

abroddrick reviewed Nov 20, 2024

View reviewed changes

src/registrar/admin.py Outdated Show resolved Hide resolved

abroddrick previously requested changes Nov 20, 2024

View reviewed changes

asaki222 assigned asaki222 and unassigned asaki222 Nov 22, 2024

CocoByte added 3 commits November 24, 2024 22:01

Cleanup

503d214

Added missing converted fields to domain request

76ac7a0

Updates to analytics exports

88554e0

Re-added missing function (merge error)

6ca5356

asaki222 reviewed Dec 2, 2024

View reviewed changes

src/registrar/tests/test_reports.py Outdated Show resolved Hide resolved

src/registrar/tests/test_reports.py Outdated Show resolved Hide resolved

CocoByte added 2 commits December 2, 2024 13:00

A few updates based on feedback

a6308ed

Remove converted values from exports + fixes

9a8ac32

Efficiency updates and sorting fixes

fb2fad6

zandercymatics reviewed Dec 4, 2024

View reviewed changes

CocoByte added 5 commits December 4, 2024 14:09

Merge remote-tracking branch 'origin/main' into nl/2975-domain-and-do…

88e27c8

…main-info-portfolio-fields

remove dead code (missed in another merge)

3b9e9e3

Cleanup and unit test fixes

87a9655

cleanup

d5f7993

linted

958d6dc

CocoByte added 2 commits December 5, 2024 10:43

more unit test adjustments

0a36b10

Merge remote-tracking branch 'origin/main' into nl/2975-domain-and-do…

162ba73

…main-info-portfolio-fields

zandercymatics approved these changes Dec 5, 2024

View reviewed changes

CocoByte added 2 commits December 5, 2024 12:58

Merge remote-tracking branch 'origin/main' into nl/2975-domain-and-do…

a81cf60

…main-info-portfolio-fields

unit test fix

77d4f0a

remove maxDiff = None

4e1797a

Merge branch 'main' into nl/2975-domain-and-domain-info-portfolio-fields

4be2aa1

CocoByte merged commit 42df091 into main Dec 6, 2024
10 checks passed

CocoByte deleted the nl/2975-domain-and-domain-info-portfolio-fields branch December 6, 2024 00:23

zandercymatics mentioned this pull request Dec 18, 2024

(draft) #3242: Revert @property changes in csv_export - (BUGFIX) #3241

Closed

32 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #2975 - domain and domain info portfolio fields -[HOTGOV] #3044

Issue #2975 - domain and domain info portfolio fields -[HOTGOV] #3044

CocoByte commented Nov 7, 2024 •

edited

Loading

github-actions bot commented Nov 7, 2024

github-actions bot commented Nov 7, 2024

github-actions bot commented Nov 13, 2024

abroddrick left a comment

github-actions bot commented Dec 3, 2024

github-actions bot commented Dec 4, 2024

zandercymatics left a comment

zandercymatics Dec 4, 2024

github-actions bot commented Dec 5, 2024

github-actions bot commented Dec 5, 2024

github-actions bot commented Dec 5, 2024

zandercymatics left a comment

zandercymatics Dec 5, 2024

zandercymatics Dec 5, 2024

zandercymatics Dec 5, 2024

CocoByte Dec 5, 2024

github-actions bot commented Dec 5, 2024

github-actions bot commented Dec 5, 2024

github-actions bot commented Dec 6, 2024

		"adomain10.gov,Ready,2024-04-03,(blank),Federal,Armed Forces Retirement Home,,,,(blank),,,,"
		"adomain10.gov,Ready,2024-04-03,(blank),Federal,Armed Forces Retirement Home,,,, ,,(blank),,"

		"adomain2.gov,Dns needed,(blank),(blank),Federal - Executive,"
		"Portfolio 1 Federal Agency,,,, ,,(blank),"

Issue #2975 - domain and domain info portfolio fields -[HOTGOV] #3044

Issue #2975 - domain and domain info portfolio fields -[HOTGOV] #3044

Conversation

CocoByte commented Nov 7, 2024 • edited Loading

Ticket 2975

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 Nov 7, 2024

github-actions bot commented Nov 7, 2024

github-actions bot commented Nov 13, 2024

abroddrick left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 3, 2024

github-actions bot commented Dec 4, 2024

zandercymatics left a comment

Choose a reason for hiding this comment

zandercymatics Dec 4, 2024

Choose a reason for hiding this comment

github-actions bot commented Dec 5, 2024

github-actions bot commented Dec 5, 2024

github-actions bot commented Dec 5, 2024

zandercymatics left a comment

Choose a reason for hiding this comment

zandercymatics Dec 5, 2024

Choose a reason for hiding this comment

zandercymatics Dec 5, 2024

Choose a reason for hiding this comment

zandercymatics Dec 5, 2024

Choose a reason for hiding this comment

CocoByte Dec 5, 2024

Choose a reason for hiding this comment

github-actions bot commented Dec 5, 2024

github-actions bot commented Dec 5, 2024

github-actions bot commented Dec 6, 2024

CocoByte commented Nov 7, 2024 •

edited

Loading