-
Notifications
You must be signed in to change notification settings - Fork 19
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
Conversation
🥳 Successfully deployed to developer sandbox za. |
🥳 Successfully deployed to developer sandbox za. |
1 similar comment
🥳 Successfully deployed to developer sandbox za. |
🥳 Successfully deployed to developer sandbox za. |
🥳 Successfully deployed to developer sandbox za. |
🥳 Successfully deployed to developer sandbox za. |
🥳 Successfully deployed to developer sandbox za. |
🥳 Successfully deployed to developer sandbox za. |
🥳 Successfully deployed to developer sandbox za. |
🥳 Successfully deployed to developer sandbox za. |
🥳 Successfully deployed to developer sandbox za. |
🥳 Successfully deployed to developer sandbox za. |
🥳 Successfully deployed to developer sandbox za. |
@zandercymatics why do you have the "blocked from starting a new domain request" in this PR? |
Responding to @rachidatecs 's message
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.
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."
|
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. |
@abroddrick Hey Alysia! Just got off a call with Rachid. Leaving some notes though @rachidatecs feel free to respond too!
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
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 |
Ah sorry, just linked the wrong ticket |
🥳 Successfully deployed to developer sandbox za. |
1 similar comment
🥳 Successfully deployed to developer sandbox za. |
@@ -0,0 +1,177 @@ | |||
# Generated by Django 4.2.10 on 2024-10-30 17:59 |
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.
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.") |
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.
Q: Why aren't we using error definitions on the form fields instead?
src/registrar/templates/includes/portfolio_request_review_steps.html
Outdated
Show resolved
Hide resolved
…s.html Co-authored-by: Rachid Mrad <[email protected]>
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 |
🥳 Successfully deployed to developer sandbox za. |
…sagov/manage.get.gov into za/2771-create-requesting-entity-page
🥳 Successfully deployed to developer sandbox za. |
🥳 Successfully deployed to developer sandbox za. |
Ticket
Resolves #2771 and #3015
Changes
Context for reviewers
Primarily, this PR does four things for portfolio domain requests:
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.
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