-
Notifications
You must be signed in to change notification settings - Fork 20
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.
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
#3234: Modify create_federal_portfolio script to add requested suborg fields - [AG] #3303
Conversation
🥳 Successfully deployed to developer sandbox ag. |
2 similar comments
🥳 Successfully deployed to developer sandbox ag. |
🥳 Successfully deployed to developer sandbox ag. |
🥳 Successfully deployed to developer sandbox ag. |
🥳 Successfully deployed to developer sandbox ag. |
fields_to_update = ["requested_suborganization", "suborganization_city", "suborganization_state_territory"] | ||
self.mass_update_records(DomainRequest, filter_conditions, fields_to_update) | ||
|
||
def custom_filter(self, records): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: maybe this function should be renamed to something more semantic within the context of its confines.
Basically all this does is after the initial filter, it does an exclude on the existing set of records
🥳 Successfully deployed to developer sandbox ag. |
🥳 Successfully deployed to developer sandbox ag. |
🥳 Successfully deployed to developer sandbox ag. |
🥳 Successfully deployed to developer sandbox ag. |
🥳 Successfully deployed to developer sandbox ag. |
🥳 Successfully deployed to developer sandbox ag. |
1 similar comment
🥳 Successfully deployed to developer sandbox ag. |
🥳 Successfully deployed to developer sandbox ag. |
This comment does not apply to your changes, but to the create_federal_portfolio.py file. add_arguments method could use explanation in its method comment for the --branch option. |
@dave-kennedy-ecs Good find, thanks. I added a better comment to it which you may find helpful, as well |
@@ -111,6 +170,8 @@ def handle_populate_portfolio(self, federal_agency, parse_domains, parse_request | |||
if parse_requests or both: | |||
self.handle_portfolio_requests(portfolio, federal_agency) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: why is handle_portfolio_domains only called if created, but handle_portfolio_requests is called regardless of whether it is a creation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't entirely remember, to be honest. This script has "outgrown its shoes" a few times, so honestly probably a leftover
In a follow-on PR I am not checking on "created" for create_suborganizations either, so it probably makes sense to remove that created check, imo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, got it. Thanks for the explanation. I don't think it effects this PR, as it is focused on domain request.
Review of code is done. Script is easy to read and follow. And appears to meet the ACs. Will now test in AG. |
🥳 Successfully deployed to developer sandbox ag. |
…age.get.gov into ag/3234-test-existing-script
🥳 Successfully deployed to developer sandbox ag. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested successfully locally and on AG sandbox
Approved
🥳 Successfully deployed to developer sandbox ag. |
Ticket
Resolves #3234
Changes
Context for reviewers
This PR adds some missing data to the
create_federal_portfolio
script -- we are now adding data to "requested suborganization, suborganization city, and suborganization state_territory" if the domain request is requesting a suborganization name that does not yet currently exist.Additionally, it does some normalization on org names so we don't get duplicates.
Note: Department of Defense has a lot of good data, which may be helpful when testing this PR.
Setup
This is on getgov-ag so you can view the results there. Specifically, I'd recommend looking at department of defense and its started domains, as well. To test this locally, create a new domain request with the organization_name, state, and state_territory fields filled out. Then add a portfolio to said domain request, and run the script
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
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.
As a designer reviewer, I have
Verified that the changes match the design intention
Validated user-facing changes as a designer
References
Screenshots