-
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
Changes from 42 commits
e04337f
b3e9096
65f1e62
860f8f4
888d77b
514cd8a
263ec02
086d197
5b629bd
ab7a6ac
0374efb
939b11e
150a59c
5cb3298
bde6c5e
5e23ebe
d9ec108
87d51c1
178d127
14bfeb7
738ff0f
d44bcad
934d06d
0933fe4
9531076
7e2e75d
d0aff60
b09e0ca
bc8789b
ee71ba4
b3faa00
a94a5b2
c465b7f
dfb59a6
4f1febf
bb9cb52
98842c1
603e2eb
b01e707
f0ba596
5109384
ba61ec8
1d34598
83720c3
b05a62e
7cdfb7a
c59289d
7141eae
a1d37ee
706dd4f
cf0fec1
e4c15ee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,7 @@ | |
BaseYesNoForm, | ||
BaseDeletableRegistrarForm, | ||
) | ||
from registrar.models import Contact, DomainRequest, DraftDomain, Domain, FederalAgency | ||
from registrar.models import Contact, DomainRequest, DraftDomain, Domain, FederalAgency, Suborganization | ||
from registrar.templatetags.url_helpers import public_site_url | ||
from registrar.utility.enums import ValidationReturnType | ||
from registrar.utility.constants import BranchChoices | ||
|
@@ -22,10 +22,151 @@ | |
|
||
|
||
class RequestingEntityForm(RegistrarForm): | ||
organization_name = forms.CharField( | ||
label="Organization name", | ||
"""The requesting entity form contains a dropdown for suborganizations, | ||
and some (hidden by default) input fields that allow the user to request for a suborganization. | ||
All of these fields are not required by default, but as we use javascript to conditionally show | ||
and hide some of these, they then become required in certain circumstances.""" | ||
|
||
# IMPORTANT: This is tied to DomainRequest.is_requesting_new_suborganization(). | ||
# This is due to the from_database method on DomainRequestWizard. | ||
# Add a hidden field to store if the user is requesting a new suborganization. | ||
# This hidden boolean is used for our javascript to communicate to us and to it. | ||
# If true, the suborganization form will auto select a js value "Other". | ||
# If this selection is made on the form (tracked by js), then it will toggle the form value of this. | ||
# In other words, this essentially tracks if the suborganization field == "Other". | ||
# "Other" is just an imaginary value that is otherwise invalid. | ||
# Note the logic in `def clean` and line 2744 in get-gov.js | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. comment: don't include line numbers in code comments please, they will fall out of date There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, I'll include the function name instead |
||
is_requesting_new_suborganization = forms.BooleanField(required=False, widget=forms.HiddenInput()) | ||
|
||
sub_organization = forms.ModelChoiceField( | ||
label="Suborganization name", | ||
required=False, | ||
queryset=Suborganization.objects.none(), | ||
empty_label="--Select--", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. change before merging: add an error message here see response to you question here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah thanks didn't catch that! |
||
) | ||
requested_suborganization = forms.CharField( | ||
label="Requested suborganization", | ||
required=False, | ||
error_messages={"required": "Enter the name of your organization."}, | ||
) | ||
suborganization_city = forms.CharField( | ||
label="City", | ||
required=False, | ||
error_messages={"required": "Enter the city where your organization is located."}, | ||
) | ||
suborganization_state_territory = forms.ChoiceField( | ||
label="State, territory, or military post", | ||
required=False, | ||
choices=[("", "--Select--")] + DomainRequest.StateTerritoryChoices.choices, | ||
error_messages={ | ||
"required": ("Select the state, territory, or military post where your organization is located.") | ||
}, | ||
) | ||
|
||
def __init__(self, *args, **kwargs): | ||
"""Override of init to add the suborganization queryset""" | ||
super().__init__(*args, **kwargs) | ||
|
||
if self.domain_request.portfolio: | ||
self.fields["sub_organization"].queryset = Suborganization.objects.filter( | ||
portfolio=self.domain_request.portfolio | ||
) | ||
|
||
def clean_sub_organization(self): | ||
"""On suborganization clean, set the suborganization value to None if the user is requesting | ||
a custom suborganization (as it doesn't exist yet)""" | ||
|
||
# If it's a new suborganization, return None (equivalent to selecting nothing) | ||
if self.cleaned_data.get("is_requesting_new_suborganization"): | ||
return None | ||
|
||
# Otherwise just return the suborg as normal | ||
return self.cleaned_data.get("sub_organization") | ||
|
||
def full_clean(self): | ||
"""Validation logic to remove the custom suborganization value before clean is triggered. | ||
Without this override, the form will throw an 'invalid option' error.""" | ||
# Remove the custom other field before cleaning | ||
data = self.data.copy() if self.data else None | ||
|
||
# Remove the 'other' value from suborganization if it exists. | ||
# This is a special value that tracks if the user is requesting a new suborg. | ||
suborganization = self.data.get("portfolio_requesting_entity-sub_organization") | ||
if suborganization and "other" in suborganization: | ||
data["portfolio_requesting_entity-sub_organization"] = "" | ||
|
||
# Set the modified data back to the form | ||
self.data = data | ||
|
||
# Call the parent's full_clean method | ||
super().full_clean() | ||
|
||
def clean(self): | ||
"""Custom clean implementation to handle our desired logic flow for suborganization. | ||
Given that these fields often rely on eachother, we need to do this in the parent function.""" | ||
cleaned_data = super().clean() | ||
|
||
# Do some custom error validation if the requesting entity is a suborg. | ||
# Otherwise, just validate as normal. | ||
suborganization = self.cleaned_data.get("sub_organization") | ||
is_requesting_new_suborganization = self.cleaned_data.get("is_requesting_new_suborganization") | ||
|
||
# Get the value of the yes/no checkbox from RequestingEntityYesNoForm. | ||
# Since self.data stores this as a string, we need to convert "True" => True. | ||
requesting_entity_is_suborganization = self.data.get( | ||
"portfolio_requesting_entity-requesting_entity_is_suborganization" | ||
) | ||
if requesting_entity_is_suborganization == "True": | ||
if is_requesting_new_suborganization: | ||
# Validate custom suborganization fields | ||
if not cleaned_data.get("requested_suborganization"): | ||
self.add_error("requested_suborganization", "Enter details for your organization name.") | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Change before merging Why don't these errors match the ones you have above in the field definitions? If the others aren't triggering due to the setup, they should be removed to avoid confusion and the content copied here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. Just old content that evaded me I guess, the errors that get thrown are the ones in clean as these are all "optional" There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||
elif not suborganization: | ||
self.add_error("sub_organization", "Select a suborganization.") | ||
|
||
return cleaned_data | ||
|
||
|
||
class RequestingEntityYesNoForm(BaseYesNoForm): | ||
"""The yes/no field for the RequestingEntity form.""" | ||
|
||
# This first option will change dynamically | ||
form_choices = ((False, "Current Organization"), (True, "A suborganization. (choose from list)")) | ||
|
||
# IMPORTANT: This is tied to DomainRequest.is_requesting_new_suborganization(). | ||
# This is due to the from_database method on DomainRequestWizard. | ||
field_name = "requesting_entity_is_suborganization" | ||
|
||
def __init__(self, *args, **kwargs): | ||
"""Extend the initialization of the form from RegistrarForm __init__""" | ||
super().__init__(*args, **kwargs) | ||
if self.domain_request.portfolio: | ||
self.form_choices = ( | ||
(False, self.domain_request.portfolio), | ||
(True, "A suborganization. (choose from list)"), | ||
) | ||
self.fields[self.field_name] = self.get_typed_choice_field() | ||
|
||
@property | ||
def form_is_checked(self): | ||
""" | ||
Determines if the requesting entity is a suborganization, or a portfolio. | ||
For suborganizations, users have the ability to request a new one if the | ||
desired suborg doesn't exist. We expose additional fields that denote this, | ||
like `requested_suborganization`. So we also check on those. | ||
""" | ||
# True means that the requesting entity is a suborganization, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion Key info on your return values should exist in the docstring for the function. """ |
||
# whereas False means that the requesting entity is a portfolio. | ||
if self.domain_request.requesting_entity_is_suborganization(): | ||
return True | ||
elif self.domain_request.requesting_entity_is_portfolio(): | ||
return False | ||
else: | ||
return None | ||
|
||
|
||
class OrganizationTypeForm(RegistrarForm): | ||
|
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 did not catch this on the first pass but I think we should simply have these fields as readonly for analysts. This AC indicates so:
The Admin Readonly view for Portfolio Request, Domain info/Domain does not show the requested suborganization, suborg_city and state fields if the user is selecting a suborganization from the list. But it displays the requested suborganization, city/state fields when the user selects the "Other" option.
If that's the case, we should use pre-established patters for analyst readonly / superuser editable fields.
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.
There was an AC from @abroddrick which I did not list which specifies that these fields should be hidden if the organization flag is off. Adding that now
That said, I just reworked this which plays into your suggestion. I think it works better
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 would agree read only better follows our pre-existing patterns. However, I do think design wants those fields to not show. That said, @Katherine-Osos could we have the requested suborganization, suborg_city and state fields just turn read-only when suborganization is filled in and still provide a similiar user experience for analysts?