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.

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

#3317: Don't allow non-org model domain requests for federal agencies in a portfolio [ZA] #3408

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

Conversation

zandercymatics
Copy link
Contributor

@zandercymatics zandercymatics commented Jan 27, 2025

Ticket

Resolves #3317

Changes

  • Hides "in-use" federal agencies from the federal agency dropdown (if a portfolio exists AND if the waffle flag is enabled or not)
  • Refactors the _form_complete logic to be more consistent across usage and fix code duplication

Context for reviewers

In the near future, we intend to enable the organization_requests waffle flag. When we do so, we anticipate that some users, particularly those with started domain requests, may not be on said portfolio. Since federal agency and portfolio are the same contextually -- and since we only want "verified" users to be making requests -- we want to hide "in-use" federal agencies for those users from the dropdown list.

To achieve this, I opted to not set the underlying federal agency to none. I instead am targeting the queryset, and I modified the organization_contact step unlock logic to take this into account. This logic is then piped (all using the same function) to the summary page as well, and the final _form_complete step.

During the ticket that implemented the domain request flow for portfolio, this form complete logic was simplified by only using a length compare on db_check_for_unlocking_steps. It was chosen at the time to keep the old logic intact, but this PR finishes the refactor of that area and cleans that bit of the code up. The reason is that I had to take the portfolio logic into account on the summary page, form compete page, and also the side bar unlock.

It is worth noting that a script will be ran that will clean up the federal agency field on started domain requests, so this logic will primarily target new domain requests after that is ran. But it is designed to be robust enough to handle misses.

Setup

Testing new domain requests

  1. Enable the organization_feature and organization_requests waffle flags.
  2. Create a portfolio with the same name as an existing federal agency. A great one to use is AMTRAK.
  3. Remove the user profile permission of your user while keeping said flags turned on.
  4. Create a new federal domain request on the non-org page
  5. On the organization page, note that AMTRAK is no longer in the dropdown

Testing existing domain requests

  1. Create a federal domain request for AMTRAK, complete it entirely but don't submit it.
  2. Enable the organization_feature and organization_requests waffle flags.
  3. Create a portfolio with the same name as an existing federal agency. A great one to use is AMTRAK.
  4. Remove the user profile permission of your user while keeping said flags turned on.
  5. Visit your existing domain request, note that the request should now be "incomplete" on organization and on the summary (and it should not let you submit)

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

@zandercymatics zandercymatics changed the title [draft] #3317: hide org federal agencies [draft] #3317: Don't allow non-org model domain requests for federal agencies in a portfolio Jan 27, 2025
Copy link

🥳 Successfully deployed to developer sandbox za.

Copy link

🥳 Successfully deployed to developer sandbox za.

1 similar comment
Copy link

🥳 Successfully deployed to developer sandbox za.

@zandercymatics zandercymatics changed the title [draft] #3317: Don't allow non-org model domain requests for federal agencies in a portfolio [draft] #3317: Don't allow non-org model domain requests for federal agencies in a portfolio [NO SANDBOX] Jan 27, 2025
Copy link

🥳 Successfully deployed to developer sandbox za.

@zandercymatics zandercymatics changed the title [draft] #3317: Don't allow non-org model domain requests for federal agencies in a portfolio [NO SANDBOX] [draft] #3317: Don't allow non-org model domain requests for federal agencies in a portfolio [ZA] Jan 28, 2025
{% if domain_request.organization_name %}
{% if domain_request.unlock_organization_contact %}
Copy link
Contributor Author

@zandercymatics zandercymatics Jan 27, 2025

Choose a reason for hiding this comment

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

This change syncs the unlock state of each step to the incomplete dialog on the domain request page -- which further reduces code duplication.

I've just added it to this specific step and unlock_other_contacts for now

@@ -10,3 +11,18 @@ def flag_is_active_for_user(user, flag_name):
request = HttpRequest()
request.user = user
return flag_is_active(request, flag_name)


def flag_is_active_anywhere(flag_name):
Copy link
Contributor Author

@zandercymatics zandercymatics Jan 28, 2025

Choose a reason for hiding this comment

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

We intend to use the organization_requests flag kind of like a y/n switch, where we intend to turn it on for everyone at once. This function is a drop-in replacement for doing flag_is_active(request=None, "organization_requests" because that relies moreso on an implementation bug and breaks when only specific users are defined. So - relying on something we define rather than a hack

@dave-kennedy-ecs -- you worked on this area extensively though, any thoughts on this? An alternative is of course just using said approach with those caveats

@zandercymatics zandercymatics changed the title [draft] #3317: Don't allow non-org model domain requests for federal agencies in a portfolio [ZA] #3317: Don't allow non-org model domain requests for federal agencies in a portfolio [ZA] Jan 28, 2025
Copy link

🥳 Successfully deployed to developer sandbox za.

)

self.domain_request.other_contacts.add(other)
Copy link
Contributor Author

@zandercymatics zandercymatics Jan 28, 2025

Choose a reason for hiding this comment

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

This test will be moved to another file. I'm leaving it in test_models for now so its easier to read the git log

Copy link

🥳 Successfully deployed to developer sandbox za.

Copy link

🥳 Successfully deployed to developer sandbox za.

Comment on lines +1303 to +1311
return (
self.federal_agency is not None
or self.organization_name is not None
or self.address_line1 is not None
or self.city is not None
or self.state_territory is not None
or self.zipcode is not None
or self.urbanization is not None
)
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 was the original logic for this unlock_contact page - but would we want this to be an and statement on all required fields?

Comment on lines -1393 to -1395
def _is_interstate_complete(self):
# Interstate -> "About your organization" page can't be empty
return self.about_your_organization is not None
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of these functions actually presently exist on the DomainRequestWizard!

@@ -434,20 +424,38 @@ def db_check_for_unlocking_steps(self):
Queries the DB for a domain request and returns a list of unlocked steps."""
return [key for key, is_unlocked_checker in self.unlocking_steps.items() if is_unlocked_checker(self)]

def form_is_complete(self):
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 is a modification of the old logic, org_steps_complete = len(self.db_check_for_unlocking_steps()) == len(self.steps). The domain request form on the org model had a key difference as compared to the standard one: it didn't have "conditional" steps. Length comparison only works without those, so this approach is more complete and takes into account all required steps and all currently unlocked steps

Copy link

🥳 Successfully deployed to developer sandbox za.

Copy link

🥳 Successfully deployed to developer sandbox za.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't allow non-org model domain requests for federal agencies in a portfolio
1 participant