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

#2771 + #3015: Create requesting entity page - [MEOWARD] #2973

Merged
merged 52 commits into from
Nov 1, 2024

Conversation

zandercymatics
Copy link
Contributor

@zandercymatics zandercymatics commented Oct 23, 2024

Ticket

Resolves #2771 and #3015

Changes

  • Added a "requesting entity" domain request form step + added page to pa11y
  • Added new fields to domainrequest and domain information: requested_suborganization, suborganization_city, suborganization_state_territory
  • Added new fields to django admin, and only show them for superusers (for now)
  • Added javascript to show/hide the new fields in django admin when the user is making a new suborganization request
  • Added javascript for special form display logic on the requesting entity page
  • Added a custom submission email for organization domain requests (toggles between new and old if a portfolio exists)
  • Updated the request review page to include data from the requesting entity step (with logic for if a portfolio is selected vs a suborg vs a custom one)
  • Added the new review page to the portfolio status manage page (as it was using the old non-portfolio one)
  • Updated the domain request status page to display the new summary

Context for reviewers

Primarily, this PR does four things for portfolio domain requests:

  1. Adds a new requesting entity step
  2. Updates the existing request summaries to include this step
  3. Adds a new submission confirmation email to instead include information from the portfolio request
  4. Updates the domain request status page (and viewonly page) summaries to match the new request (Requests owned by a user should match the view for domain requests they don't own #3015)
  5. Adds the new requesting_suborganization, suborganization_city, and suborganization_state_territory fields to django admin. They are readonly for analysts, and these fields are hidden when the organization flag is off, when no portfolio is selected, or when a portfolio is selected and a suborganization is selected

Note: this PR does not include logic to automatically create a suborganization when a domain request is approved. This is intentional as there currently is not a determination on if that should be done.

Setup

Testing requesting entity
First, enable the organization_feature and organization_request flags. Then start a new domain request. Requesting entity should be the first option you see.
There are three flows on this page. These all should match the figma.

Testing the domain request summaries
After testing the step above, complete the domain request and go to the review your domain request page. These should match the figma. The only exception to note is that if you just select a suborganization, no state or city will display as there is no data to pull from (another ticket will add that to the suborganization object).

Additionally, verify that this summary is displayed correctly on the "view" and "manage" contexts. You can test the manage context by submitting your domain, and the view by just clicking on a random domain request on the portfolio. (#3015)

Testing Django Admin
The requesting_entity, requesting_suborganization, and requesting_state_territory fields should only be visible when the domain request has a portfolio and when no suborganization is selected. Additionally, these fields should only be visible for superusers as of now.

Testing email
Submit a domain request and ensure that all data displays correctly. You should see a new email with new content.

  • Ensure that these new fields are hidden on django admin with the org flag off

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

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.

Copy link

🥳 Successfully deployed to developer sandbox za.

Copy link

🥳 Successfully deployed to developer sandbox za.

Copy link

🥳 Successfully deployed to developer sandbox za.

Copy link

🥳 Successfully deployed to developer sandbox za.

Copy link

🥳 Successfully deployed to developer sandbox za.

Copy link

🥳 Successfully deployed to developer sandbox za.

Copy link

🥳 Successfully deployed to developer sandbox za.

Copy link

github-actions bot commented Nov 1, 2024

🥳 Successfully deployed to developer sandbox za.

Copy link

github-actions bot commented Nov 1, 2024

🥳 Successfully deployed to developer sandbox za.

Copy link

github-actions bot commented Nov 1, 2024

🥳 Successfully deployed to developer sandbox za.

@abroddrick
Copy link
Contributor

@zandercymatics why do you have the "blocked from starting a new domain request" in this PR?

@abroddrick
Copy link
Contributor

Responding to @rachidatecs 's message

Scenario 1: Suborganization should not show as blank. Maybe 'Other'?

In django admin, we do want it blank so that an analysts can easily see that suborg needs to be selected. We also don't want to add in an "other" record into the suborganization table, so inorder to show "other" on django admin we would need to do more js to customize admin to show other. This doesn't seem needed and just increases the js complexity on admin.

Scenario 2: I am able to select a suborg that does not belong to the portfolio

Funny enough this is an AC on the ticket you (Rachid) and Dave are assigned to "the "suborganization" field should only show suborgs associated with the selected portfolio."

Issue 3:
Nav on the request page is missing all links except for Members?
That's a good catch! The figma shows that nothing should show in that top nav besides organization (which i disagree with a bit, given it makes navigating back really unintuitive). @zandercymatics I believe this was actually done in your ticket for the first domain request ticket. Did maybe some bug occur making the top nav incorrect or was that AC always missed?
the figma:
image

  1. Sidenav seems buggy on Purpose and Additional details on the org request page:
    Did you have those fields filled in at all, or were you on a brand new request? I would suggest @zandercymatics double check that this does exist on other sandboxes. If this is introduced in this PR we want to make sure to fix it.

@rachidatecs
Copy link
Contributor

In a conversation with Zander today, it seems like the 'other' selection should logically be its own option in our DB IF that 'other' selection is persistent.

On the other hand, if that 'other' selection is just a placeholder for the selected new suborg, and in subsequent work, that requested suborg will be made into an actual suborg and the requesting domain_request gets revised so that suborg=requested_suborg, than using a 'hacked' blank value makes sense.

@abroddrick @Katherine-Osos we think this is a core distinction that should really influence how we move forward.

@zandercymatics
Copy link
Contributor Author

zandercymatics commented Nov 1, 2024

@abroddrick Hey Alysia! Just got off a call with Rachid. Leaving some notes though @rachidatecs feel free to respond too!

Did you have those fields filled in at all, or were you on a brand new request? I would suggest @zandercymatics double check that this does exist on other sandboxes. If this is introduced in this PR we want to make sure to fix it.

He did. Rachid and I met and determined this was not an issue. These two values were empty strings with spaces, i.e. " " (did print on that particular record). This can only be added via admin or fixtures, not the form

@zandercymatics I believe this was actually done in your ticket for the first domain request ticket. Did maybe some bug occur making the top nav incorrect or was that AC always missed?
the figma:

it was missing an if statement around member. I am actually not sure as to the cause. Not a bug in that sense But I do not know why that was missing, I know there were a few merge conflicts I had to deal with so that would be my first guess. I'd have to dig into it

@zandercymatics
Copy link
Contributor Author

why do you have the "blocked from starting a new domain request" in this PR?

Ah sorry, just linked the wrong ticket

@zandercymatics zandercymatics changed the title #2771 + #2927: Create requesting entity page - [MEOWARD] #2771 + #3015: Create requesting entity page - [MEOWARD] Nov 1, 2024
Copy link

github-actions bot commented Nov 1, 2024

🥳 Successfully deployed to developer sandbox za.

1 similar comment
Copy link

github-actions bot commented Nov 1, 2024

🥳 Successfully deployed to developer sandbox za.

@@ -0,0 +1,177 @@
# Generated by Django 4.2.10 on 2024-10-30 17:59
Copy link
Contributor

Choose a reason for hiding this comment

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

BLOCKING: Delete both migrations and rerun makemigrations to generate 1 file

if not cleaned_data.get("suborganization_city"):
self.add_error("suborganization_city", "Enter details for your city.")
if not cleaned_data.get("suborganization_state_territory"):
self.add_error("suborganization_state_territory", "Enter details for your state or territory.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: Why aren't we using error definitions on the form fields instead?

@zandercymatics
Copy link
Contributor Author

Q: Why aren't we using error definitions on the form fields instead?

Because technically none of the fields on the form can be required, since we use javascript to determine shown/hidden fields. The alternative would be to toggle the "required" property and delete errors

@rachidatecs rachidatecs self-requested a review November 1, 2024 17:05
Copy link

github-actions bot commented Nov 1, 2024

🥳 Successfully deployed to developer sandbox za.

Copy link

github-actions bot commented Nov 1, 2024

🥳 Successfully deployed to developer sandbox za.

Copy link

github-actions bot commented Nov 1, 2024

🥳 Successfully deployed to developer sandbox za.

@zandercymatics zandercymatics merged commit 835c0ae into main Nov 1, 2024
10 checks passed
@zandercymatics zandercymatics deleted the za/2771-create-requesting-entity-page branch November 1, 2024 18:05
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.

Create Requesting Entity Page
3 participants