#2934: Django Admin - Dynamic Domain Request edit page [DK] #3040
Conversation
|
🥳 Successfully deployed to developer sandbox dk. |
|
🥳 Successfully deployed to developer sandbox dk. |
|
🥳 Successfully deployed to developer sandbox dk. |
|
🥳 Successfully deployed to developer sandbox dk. |
|
🥳 Successfully deployed to developer sandbox dk. |
dave-kennedy-ecs
changed the title
|
🥳 Successfully deployed to developer sandbox dk. |
|
🥳 Successfully deployed to developer sandbox dk. |
|
|
||
| # Add suborganizations related to this portfolio | ||
| suborganizations = portfolio.portfolio_suborganizations.all().values("id", "name") | ||
| results = [{"id": sub["id"], "text": sub["name"]} for sub in suborganizations] |
There was a problem hiding this comment.
(Q) Just looking at the pattern of use, can't you use model to dict here for each item? I know we wouldn't be encoding "text" => "name" though
There was a problem hiding this comment.
BTW not blocking or necessarily a suggestion (I'm neutral on it), just curious as to if you can
There was a problem hiding this comment.
I'm fairly certain that the select2 expects responses with "id" and "text" defined. And to get this to work properly, we are updating the data-ajax--url of the select2 dom object so that the javascript which we don't have control over calls this view directly and processes the response. So, for this one, I think we need to leave in the format it is.
There was a problem hiding this comment.
Ah I gotcha, thanks for the context on this one. Did not know that. That makes sense
| {% comment %}fields_always_present=True will shortcut the contact_detail_list template when | ||
| 1. Senior official field should be hidden on domain request because no portfoloio is selected, which is desirable | ||
| 2. A portfolio is selected but there is no senior official on the portfolio, where the shortcut is not desirable | ||
| To solve 2, we use an else No additional contact information found on field.field.name == "portfolio_senior_official" | ||
| and we hide the placeholders from detail_table_fieldset in JS{% endcomment %} |
There was a problem hiding this comment.
Very nice comment, this was helpful to have. Thanks.
| {% elif field.field.name == "portfolio_senior_official" %} | ||
| <div class="readonly"> | ||
| {% if original_object.portfolio.senior_official %} | ||
| <a href="{% url 'admin:registrar_seniorofficial_change' original_object.portfolio.senior_official.id %}">{{ field.contents }}</a> | ||
| {% else %} | ||
| No senior official found.<br> | ||
| {% endif %} | ||
| </div> |
There was a problem hiding this comment.
This approach is totally fine, per our past convo about it. I do have a helper though which you can use to do this exact thing without having to define it manually, I'll give an example in a follow-on comment
| def portfolio_senior_official(self, obj) -> Optional[SeniorOfficial]: | ||
| return obj.portfolio.senior_official if obj.portfolio and obj.portfolio.senior_official else None |
There was a problem hiding this comment.
| def portfolio_senior_official(self, obj) -> Optional[SeniorOfficial]: | |
| return obj.portfolio.senior_official if obj.portfolio and obj.portfolio.senior_official else None | |
| def portfolio_senior_official(self, obj) -> Optional[SeniorOfficial]: | |
| # Queryset is the list that gets iterated on. | |
| # model_name is what model the link will, well, link to: `<a href={{reverse_of_model_name}}>` | |
| # attribute_name is what is displayed between the `<a>` tag: `<a>{{obj.attribute_name}}</a>` | |
| # separator does a join on each `<a>` element that gets generated and returns the resu;t | |
| # msg_for_none is what is returned if the given queryset is empty | |
| queryset = Portfolio.objects.filter(id=obj.portfolio.id, senior_official__isnull=False) | |
| return get_field_links_as_list(queryset, model_name="seniorofficial", attribute_name="senior_official", separator=",", msg_for_none="No senior official found.") |
This is the shortcut as mentioned earlier. The separator and queryset is a minor hack: a current limitation of get_field_links is that it expects a list of items (as it was designed for that), hence the separator / queryset dependency. However with a list size of either one or zero, it will look as you'd expect. What is needed is basically the same exact function but it doesn't operate on a queryset list.
No need to use this, but it may be helpful to know in any event
There was a problem hiding this comment.
Thanks Zander. I think I will keep this as is in this case. I think it makes most sense for this logic to be in the template, if it is simple enough to define there.
| function updatePortfolioSeniorOfficial(senior_official) { | ||
| if (senior_official) { | ||
| let seniorOfficialName = [senior_official.first_name, senior_official.last_name].join(' '); | ||
| let seniorOfficialLink = `<a href=/admin/registrar/seniorofficial/${senior_official.id}/change/ class='test'>${seniorOfficialName}</a>` |
There was a problem hiding this comment.
| let seniorOfficialLink = `<a href=/admin/registrar/seniorofficial/${senior_official.id}/change/ class='test'>${seniorOfficialName}</a>` | |
| let seniorOfficialLink = `<a href="/admin/registrar/seniorofficial/${senior_official.id}/change">${seniorOfficialName}</a>` |
(Nice to have but optional) - May also be a nice addition to have this reference a div/input element with the reverse to this url path. Aside from that, just minor cleanup stuff
| if (suborganizationDropdown.data('select2')) { | ||
| suborganizationDropdown.select2('destroy'); | ||
| } |
There was a problem hiding this comment.
(Comment - no change) I've messed with this too and I wonder if there is a way around destroying the picker and recreating it. I wouldn't worry about doing it, but just speaking theoretically I think it may be possible to change the url this is bound to on page load, and where it pulls data for search params, when initializing it via python
| // Initial handling of these groups. | ||
| updateFormGroupVisibility(isStatus); | ||
|
|
||
| // Listen to change events and handle rejectionReasonFormGroup display, then save status to session storage |
There was a problem hiding this comment.
Seems like an old comment?
| @@ -33,17 +33,24 @@ | |||
|
|
|||
| {# Phone #} | |||
| {% if user.phone %} | |||
| <span id="contact_info_phone">{{ user.phone }}</span> | |||
| <span class="contact_info_phone">{{ user.phone }}</span> | |||
There was a problem hiding this comment.
Nice changes, thanks
| <br> | ||
| {% else %} | ||
| None<br> | ||
| {% endif %} | ||
|
|
||
| {% elif fields_always_present %} |
There was a problem hiding this comment.
(Question - No change needed) This contact_detail_list file has been getting complex as it has grown. Did you have any trouble using this as the ticket was made?
| response = self.client.get(self.api_url, {"id": self.portfolio.id}) | ||
| self.assertEqual(response.status_code, 200) | ||
| portfolio = response.json() | ||
| self.assertEqual(portfolio["id"], self.portfolio.id) |
There was a problem hiding this comment.
(Optional) I've never used it, but you may be able to use assertJSONEqual (link) instead. Would minorly benefit readability but this is readable as is
self.assertJSONEqual(
response.content.decode(), # or response.json() -- not sure
{.....some data}
)
There was a problem hiding this comment.
NOTE - this is on the test that is testing the entire API response, it looks like the rest of the content got cut off
|
🥳 Successfully deployed to developer sandbox dk. |
Ticket
Resolves #2934
Changes
Context for reviewers
Setup
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