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

Jump to bottom

#3299: Script to update suborg values - [ZA] #3322

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

#3299: Script to update suborg values - [ZA] #3322

wants to merge 27 commits into from
+425 −23

Conversation

zandercymatics
Copy link
Contributor

@zandercymatics zandercymatics commented Jan 9, 2025 •
edited
Loading

Ticket

Resolves #3299

Changes

  • Updates the create_federal_agency script to:
    • Add the fields city, and state_territory to the suborganizations
  • Adds a script to patch_suborganizations which deletes duplicate suborg values (extra spaces in the name or weird capitalization) and replaces them with the "correct" suborg.
    • Adds some documentation on patch_suborganizations, as well
  • Adds two helpers: normalize_string and count_capitals

Context for reviewers

When we ran the create_federal_portfolio script, it did not do a name normalize stop for federal_agency.agency. This meant that we had some duplicate records of the same name, that only differed by stray whitespace at the beginning of the string. Additionally, some user inputted data was determined to be incorrect. This script corrects those records, and also fixes the original create_federal_agency script to add city and state_territory information to the suborg as well.

Setup

This script needs a cloned db of staging to test fully. You will need to verify that (at least a few of) the records outlined in this spreadsheet are fixed. Additionally, you will need to verify that on suborganizations - the "city" and "state_territory" fields exist.

I've ran the updated script on za, so feel free to prod around the data to verify that everything is working. Once you are done with that, if you would like to run the script, I will reset the za db back to staging so you can verify that functionality.

When running the script, you should expect to see 13 delete suborgs.

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

Screenshots

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 9, 2025

🥳 Successfully deployed to developer sandbox za.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 9, 2025

🥳 Successfully deployed to developer sandbox za.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 9, 2025

🥳 Successfully deployed to developer sandbox za.

@zandercymatics zandercymatics changed the title [Draft] #3299: Script to update suborg values [Draft] #3299: Script to update suborg values - ZA Jan 9, 2025
zandercymatics
zandercymatics commented Jan 9, 2025
View reviewed changes
src/registrar/management/commands/utility/terminal_helper.py
Comment on lines 403 to 413
proceed_execution = TerminalHelper.query_yes_no_exit(
f"""{TerminalColors.OKCYAN}
=====================================================
{prompt_title}
=====================================================
{verify_message}

{prompt_message}
{TerminalColors.FAIL}
Proceed? (Y = proceed, N = {action_description_for_selecting_no})
{TerminalColors.ENDC}"""
f"\n{TerminalColors.OKCYAN}"
"====================================================="
f"\n{prompt_title}\n"
"====================================================="
f"\n{verify_message}\n"
f"\n{prompt_message}\n"
f"{TerminalColors.FAIL}"
f"Proceed? (Y = proceed, N = {action_description_for_selecting_no})"
f"{TerminalColors.ENDC}"
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixes the spacing issue that has been present on this helper function, so now it is "left aligned" (in a sense)

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 9, 2025

🥳 Successfully deployed to developer sandbox za.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 9, 2025

🥳 Successfully deployed to developer sandbox za.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 9, 2025

🥳 Successfully deployed to developer sandbox za.

@zandercymatics
Revert "Lint part 2"
25cf2fe 
This reverts commit bf9824a.
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 9, 2025

🥳 Successfully deployed to developer sandbox za.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 9, 2025

🥳 Successfully deployed to developer sandbox za.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 9, 2025

🥳 Successfully deployed to developer sandbox za.

@zandercymatics zandercymatics changed the title [Draft] #3299: Script to update suborg values - ZA [Draft] #3299: Script to update suborg values - [ZA] Jan 10, 2025
zandercymatics
zandercymatics commented Jan 10, 2025
View reviewed changes
src/registrar/management/commands/create_federal_portfolio.py Outdated
Comment on lines 102 to 94
def handle_populate_portfolio(self, federal_agency, parse_domains, parse_requests, both):
"""Attempts to create a portfolio. If successful, this function will
also create new suborganizations"""
portfolio, created = self.create_portfolio(federal_agency)
if created:
self.create_suborganizations(portfolio, federal_agency)
if parse_domains or both:
self.handle_portfolio_domains(portfolio, federal_agency)
def handle_all_populate_portfolio(self, agencies, parse_domains, parse_requests, both):
Copy link
Contributor Author

@zandercymatics zandercymatics Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm just uncurling this logic into one function since we need to track all added suborgs, domains, and requests anyway. Ideally I'd put this in "handle" but we can't due to a C901 linter error. In any event, I think less function nesting is good.

Observation: I think its fine for now, but this script may need to be reorganized in the future as it continues to grow. Its grown in complexity since it was initially written

@github-actions GitHub Actions
Copy link

🥳 Successfully deployed to developer sandbox za.

1 similar comment
@github-actions GitHub Actions
Copy link

🥳 Successfully deployed to developer sandbox za.

@github-actions GitHub Actions
Copy link

🥳 Successfully deployed to developer sandbox za.

@github-actions GitHub Actions
Copy link

🥳 Successfully deployed to developer sandbox za.

@github-actions GitHub Actions
Copy link

🥳 Successfully deployed to developer sandbox za.

@zandercymatics zandercymatics changed the title [Draft] #3299: Script to update suborg values - [ZA] #3299: Script to update suborg values - [ZA] Jan 10, 2025
@github-actions GitHub Actions
Copy link

🥳 Successfully deployed to developer sandbox za.

@github-actions GitHub Actions
Copy link

🥳 Successfully deployed to developer sandbox za.

@github-actions GitHub Actions
Copy link

🥳 Successfully deployed to developer sandbox za.

@github-actions GitHub Actions
Copy link

🥳 Successfully deployed to developer sandbox za.

@dave-kennedy-ecs dave-kennedy-ecs self-assigned this Jan 14, 2025
@github-actions GitHub Actions
Copy link

🥳 Successfully deployed to developer sandbox za.

@asaki222 asaki222 self-assigned this Jan 14, 2025
@github-actions GitHub Actions
Copy link

🥳 Successfully deployed to developer sandbox za.

@asaki222
Copy link
Contributor

This looks good overall. I got a duplicate of Agricultural Research Service for suborganization. I also got it for General Services Administration but they have different portfolios.
Screenshot 2025-01-14 at 5 08 21 PM

asaki222
asaki222 reviewed Jan 14, 2025
View reviewed changes
src/registrar/management/commands/create_federal_portfolio.py
if parse_domains or both:
self.handle_portfolio_domains(portfolio, federal_agency)
also create new suborganizations"""
portfolio, _ = self.create_portfolio(federal_agency)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was the comment Returns the portfolio for the given federal_agency removed?

Copy link
Contributor Author

@zandercymatics zandercymatics Jan 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Good question! I just removed that because this function doesn't actually return anything - so I think it was a typo

src/registrar/management/commands/patch_suborganizations.py
class Command(BaseCommand):
help = "Clean up duplicate suborganizations that differ only by spaces and capitalization"

def handle(self, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

I really like the us of help text here!

src/registrar/management/commands/patch_suborganizations.py
normalize_string("USDA/OC"): {"replace_with": "USDA, Office of Communications"},
normalize_string("GSA, IC, OGP WebPortfolio"): {"replace_with": "GSA, IC, OGP Web Portfolio"},
normalize_string("USDA/ARS/NAL"): {"replace_with": "USDA, ARS, NAL"},
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious as to where these values came from? Are they supposed to be hardcorded?

Copy link
Contributor Author

@zandercymatics zandercymatics Jan 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

They come from this spreadsheet. The create_federal_portfolio script assumes that the org names the user provides are always correct, but some domains misspelt their org name. Since its just a misspelling, these corner cases are hard coded. Data cleanup basically

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asaki222 asaki222 asaki222 left review comments

@abroddrick abroddrick Awaiting requested review from abroddrick abroddrick is a code owner

@lizpearl lizpearl Awaiting requested review from lizpearl

@therealslimhsiehdy therealslimhsiehdy Awaiting requested review from therealslimhsiehdy

@Matt-Spence Matt-Spence Awaiting requested review from Matt-Spence

@CocoByte CocoByte Awaiting requested review from CocoByte

@rachidatecs rachidatecs Awaiting requested review from rachidatecs

@dave-kennedy-ecs dave-kennedy-ecs Awaiting requested review from dave-kennedy-ecs

@erinysong erinysong Awaiting requested review from erinysong

At least 1 approving review is required to merge this pull request.

Assignees

@dave-kennedy-ecs dave-kennedy-ecs

@asaki222 asaki222

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Script to update Suborganization values
3 participants
@zandercymatics @asaki222 @dave-kennedy-ecs