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

Issue #2595: update domain request view only pages - [ZA] #2753

Merged
merged 15 commits into from
Sep 19, 2024
Merged
20 changes: 20 additions & 0 deletions src/registrar/models/domain_request.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
from registrar.models.utility.generic_helper import CreateOrUpdateOrganizationTypeHelper
from registrar.utility.errors import FSMDomainRequestError, FSMErrorCodes
from registrar.utility.constants import BranchChoices
from auditlog.models import LogEntry

from .utility.time_stamped_model import TimeStampedModel
from ..utility.email import send_templated_email, EmailSendingError
Expand Down Expand Up @@ -589,11 +590,25 @@ def get_action_needed_reason_label(cls, action_needed_reason: str):
verbose_name="last updated on",
help_text="Date of the last status update",
)

notes = models.TextField(
null=True,
blank=True,
)

def get_first_status_set_date(self, status):
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, would we expect any cases where we'd want to get the date a request was first given a certain status aside from started?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe for scripts or some info for django admin, but I'm not really concrete on it. I left this function around in case we need it, but honestly if you think it'd be more clear to just consolidate these I am totally OK with that too

Copy link
Contributor

Choose a reason for hiding this comment

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

Also don't have a strong opinion so I'm happy to go with whatever data from audit logs you think would be useful in the future!

"""Returns the date when the domain request was first set to the given status."""
log_entry = (
LogEntry.objects.filter(content_type__model="domainrequest", object_pk=self.pk, changes__status__1=status)
.order_by("-timestamp")
.first()
)
return log_entry.timestamp.date() if log_entry else None

def get_first_status_started_date(self):
"""Returns the date when the domain request was put into the status "started" for the first time"""
return self.get_first_status_set_date(DomainRequest.DomainRequestStatus.STARTED)

@classmethod
def get_statuses_that_send_emails(cls):
"""Returns a list of statuses that send an email to the user"""
Expand Down Expand Up @@ -1154,6 +1169,11 @@ def to_dict(self):
data[field.name] = field.value_from_object(self)
return data

def get_formatted_cisa_rep_name(self):
"""Returns the cisa representatives name in Western order."""
names = [n for n in [self.cisa_representative_first_name, self.cisa_representative_last_name] if n]
return " ".join(names) if names else "Unknown"

def _is_federal_complete(self):
# Federal -> "Federal government branch" page can't be empty + Federal Agency selection can't be None
return not (self.federal_type is None or self.federal_agency is None)
Expand Down
19 changes: 19 additions & 0 deletions src/registrar/models/utility/generic_helper.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import time
import logging
from urllib.parse import urlparse, urlunparse, urlencode
from django.urls import resolve, Resolver404

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -315,3 +316,21 @@ def convert_queryset_to_dict(queryset, is_model=True, key="id"):
request_dict = {value[key]: value for value in queryset}

return request_dict


def get_url_name(path):
"""
Given a URL path, returns the corresponding URL name defined in urls.py.

Args:
path (str): The URL path to resolve.

Returns:
str or None: The URL name if it exists, otherwise None.
"""
try:
match = resolve(path)
return match.url_name
except Resolver404:
logger.error(f"No matching URL name found for path: {path}")
return None
113 changes: 78 additions & 35 deletions src/registrar/templates/domain_request_status.html
Original file line number Diff line number Diff line change
Expand Up @@ -8,33 +8,30 @@
{% block content %}
<main id="main-content" class="grid-container">
<div class="grid-col desktop:grid-offset-2 desktop:grid-col-8">
{% comment %}
TODO: Uncomment in #2596
{% if portfolio %}
{% url 'domain-requests' as url %}
<nav class="usa-breadcrumb padding-top-0" aria-label="Domain request breadcrumb">
<ol class="usa-breadcrumb__list">
<li class="usa-breadcrumb__list-item">
{% else %}
{% url 'home' as url %}
{% endif %}
<nav class="usa-breadcrumb padding-top-0" aria-label="Domain request breadcrumb">
<ol class="usa-breadcrumb__list">
<li class="usa-breadcrumb__list-item">
{% if portfolio %}
<a href="{{ url }}" class="usa-breadcrumb__link"><span>Domain requests</span></a>
</li>
<li class="usa-breadcrumb__list-item usa-current" aria-current="page">
<span>{{ DomainRequest.requested_domain.name }}</span
>
</li>
</ol>
</nav>
{% else %}{% endcomment %}
{% url 'home' as url %}
<a href="{{ url }}" class="breadcrumb__back">
<svg class="usa-icon" aria-hidden="true" focusable="false" role="img">
<use xlink:href="{% static 'img/sprite.svg' %}#arrow_back"></use>
</svg>

<p class="margin-left-05 margin-top-0 margin-bottom-0 line-height-sans-1">
Back to manage your domains
</p>
</a>
{% comment %} {% endif %}{% endcomment %}
{% else %}
<a href="{{ url }}" class="usa-breadcrumb__link"><span>Manage your domains</span></a>
{% endif %}
</li>
<li class="usa-breadcrumb__list-item usa-current" aria-current="page">
{% if not DomainRequest.requested_domain and DomainRequest.status == DomainRequest.DomainRequestStatus.STARTED %}
<span>New domain request</span>
{% else %}
<span>{{ DomainRequest.requested_domain.name }}</span>
{% endif %}
</li>
</ol>
</nav>

<h1>Domain request for {{ DomainRequest.requested_domain.name }}</h1>
<div
class="usa-summary-box dotgov-status-box margin-top-3 padding-left-2"
Expand All @@ -48,25 +45,71 @@ <h1>Domain request for {{ DomainRequest.requested_domain.name }}</h1>
<span class="text-bold text-primary-darker">
Status:
</span>
{% if DomainRequest.status == 'approved' %} Approved
{% elif DomainRequest.status == 'in review' %} In review
{% elif DomainRequest.status == 'rejected' %} Rejected
{% elif DomainRequest.status == 'submitted' %} Submitted
{% elif DomainRequest.status == 'ineligible' %} Ineligible
{% else %}ERROR Please contact technical support/dev
{% endif %}
{{ DomainRequest.get_status_display|default:"ERROR Please contact technical support/dev" }}
</p>
</div>
</div>
<br>
<p><b class="review__step__name">Last updated:</b> {{DomainRequest.updated_at|date:"F j, Y"}}</p>


{% with statuses=DomainRequest.DomainRequestStatus last_submitted=DomainRequest.last_submitted_date|date:"F j, Y" first_submitted=DomainRequest.first_submitted_date|date:"F j, Y" last_status_update=DomainRequest.last_status_update|date:"F j, Y" %}
{% comment %}
These are intentionally seperated this way.
There is some code repetition, but it gives us more flexibility rather than a dense reduction.
Leave it this way until we've solidified our requirements.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just checking if there ended up being any requirements set or any tickets to set these requirements on the way we display status and if we yes, if we should reference them here!

Copy link
Contributor Author

@zandercymatics zandercymatics Sep 19, 2024

Choose a reason for hiding this comment

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

No tickets in particular that I am aware of! That is a good callout though and I'm glad you thought of that.

These pages are still undergoing design review + we are fleshing out the org feature. Personally, I think these dates may shift around - or we may decide to add some more in the future. That was my thinking, at least

{% endcomment %}
{% if DomainRequest.status == statuses.STARTED %}
{% with first_started_date=DomainRequest.get_first_status_started_date|date:"F j, Y" %}
<p class="margin-top-1">
{% comment %}
A newly created domain request will not have a value for last_status update.
This is because the status never really updated.
However, if this somehow goes back to started we can default to displaying that new date.
{% endcomment %}
<b class="review__step__name">Started on:</b> {{last_status_update|default:first_started_date}}
</p>
{% endwith %}
{% elif DomainRequest.status == statuses.SUBMITTED %}
<p class="margin-top-1 margin-bottom-1">
<b class="review__step__name">Submitted on:</b> {{last_submitted|default:first_submitted }}
</p>
<p class="margin-top-1">
<b class="review__step__name">Last updated on:</b> {{DomainRequest.updated_at|date:"F j, Y"}}
</p>
{% elif DomainRequest.status == statuses.ACTION_NEEDED %}
<p class="margin-top-1 margin-bottom-1">
<b class="review__step__name">Submitted on:</b> {{last_submitted|default:first_submitted }}
</p>
<p class="margin-top-1">
<b class="review__step__name">Last updated on:</b> {{DomainRequest.updated_at|date:"F j, Y"}}
</p>
{% elif DomainRequest.status == statuses.REJECTED %}
<p class="margin-top-1 margin-bottom-1">
<b class="review__step__name">Submitted on:</b> {{last_submitted|default:first_submitted }}
</p>
<p class="margin-top-1">
<b class="review__step__name">Rejected on:</b> {{last_status_update}}
</p>
{% elif DomainRequest.status == statuses.WITHDRAWN %}
<p class="margin-top-1 margin-bottom-1">
<b class="review__step__name">Submitted on:</b> {{last_submitted|default:first_submitted }}
</p>
<p class="margin-top-1">
<b class="review__step__name">Withdrawn on:</b> {{last_status_update}}
</p>
{% else %}
{% comment %} Shown for in_review, approved, ineligible {% endcomment %}
<p class="margin-top-1">
<b class="review__step__name">Last updated on:</b> {{DomainRequest.updated_at|date:"F j, Y"}}
</p>
{% endif %}

{% if DomainRequest.status != 'rejected' %}
<p>{% include "includes/domain_request.html" %}</p>
<p><a href="{% url 'domain-request-withdraw-confirmation' pk=DomainRequest.id %}" class="usa-button usa-button--outline withdraw_outline">
Withdraw request</a>
</p>
{% endif %}
{% endwith %}
</div>

<div class="grid-col desktop:grid-offset-2 maxw-tablet">
Expand Down Expand Up @@ -141,8 +184,8 @@ <h2 class="text-primary-darker"> Summary of your domain request </h2>
{% if DomainRequest %}
<h3 class="register-form-review-header">CISA Regional Representative</h3>
<ul class="usa-list usa-list--unstyled margin-top-0">
{% if domain_request.cisa_representative_first_name %}
{{domain_request.cisa_representative_first_name}} {{domain_request.cisa_representative_last_name}}
{% if DomainRequest.cisa_representative_first_name %}
{{ DomainRequest.get_formatted_cisa_rep_name }}
{% else %}
No
{% endif %}
Expand Down
10 changes: 5 additions & 5 deletions src/registrar/templates/includes/header_extended.html
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
{% else %}
{% url 'no-portfolio-domains' as url %}
{% endif %}
<a href="{{ url }}" class="usa-nav-link{% if 'domain'|in_path:request.path %} usa-current{% endif %}">
<a href="{{ url }}" class="usa-nav-link{% if path|is_domain_subpage %} usa-current{% endif %}">
Domains
</a>
</li>
Expand All @@ -59,7 +59,7 @@
{% url 'domain-requests' as url %}
<button
type="button"
class="usa-accordion__button usa-nav__link{% if 'request'|in_path:request.path %} usa-current{% endif %}"
class="usa-accordion__button usa-nav__link{% if path|is_domain_request_subpage %} usa-current{% endif %}"
aria-expanded="false"
aria-controls="basic-nav-section-two"
>
Expand All @@ -80,13 +80,13 @@
<!-- user has view but no edit permissions -->
{% elif has_any_requests_portfolio_permission %}
{% url 'domain-requests' as url %}
<a href="{{ url }}" class="usa-nav-link{% if 'request'|in_path:request.path %} usa-current{% endif %}">
<a href="{{ url }}" class="usa-nav-link{% if path|is_domain_request_subpage %} usa-current{% endif %}">
Domain requests
</a>
<!-- user does not have permissions -->
{% else %}
{% url 'no-portfolio-requests' as url %}
<a href="{{ url }}" class="usa-nav-link{% if 'request'|in_path:request.path %} usa-current{% endif %}">
<a href="{{ url }}" class="usa-nav-link{% if path|is_domain_request_subpage %} usa-current{% endif %}">
Domain requests
</a>
{% endif %}
Expand All @@ -104,7 +104,7 @@
<li class="usa-nav__primary-item">
{% url 'organization' as url %}
<!-- Move the padding from the a to the span so that the descenders do not get cut off -->
<a href="{{ url }}" class="usa-nav-link padding-y-0 {% if request.path == '/organization/' %} usa-current{% endif %}">
<a href="{{ url }}" class="usa-nav-link padding-y-0 {% if path|is_portfolio_subpage %} usa-current{% endif %}">
<span class="ellipsis ellipsis--23 ellipsis--desktop-50 padding-y-1 desktop:padding-y-2">
{{ portfolio.organization_name }}
</span>
Expand Down
65 changes: 65 additions & 0 deletions src/registrar/templatetags/custom_filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
import re
from registrar.models.domain_request import DomainRequest
from phonenumber_field.phonenumber import PhoneNumber
from registrar.views.domain_request import DomainRequestWizard

from registrar.models.utility.generic_helper import get_url_name

register = template.Library()
logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -174,3 +177,65 @@ def has_contact_info(user):
@register.filter
def model_name_lowercase(instance):
return instance.__class__.__name__.lower()


@register.filter(name="is_domain_subpage")
def is_domain_subpage(path):
"""Checks if the given page is a subpage of domains.
Takes a path name, like '/domains/'."""
# Since our pages aren't unified under a common path, we need this approach for now.
url_names = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think this is necessary to address in this PR, but I imagine this may get bigger as we introduce pages/subpages so should we consider a ticket to revisit this and refactor our paths in a way that's easier to recognize a page's more meta traits? @abroddrick

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree 100% with doing a refactor on our urls - and I share your concern about this list growing. Do you mind branching this off into a slack thread?

I'm in favor of a refactor ticket on these as you mentioned!

"domains",
"no-portfolio-domains",
"domain",
"domain-users",
"domain-dns",
"domain-dns-nameservers",
"domain-dns-dnssec",
"domain-dns-dnssec-dsdata",
"domain-your-contact-information",
"domain-org-name-address",
"domain-senior-official",
"domain-security-email",
"domain-users-add",
"domain-request-delete",
"domain-user-delete",
"invitation-delete",
]
return get_url_name(path) in url_names


@register.filter(name="is_domain_request_subpage")
def is_domain_request_subpage(path):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dave-kennedy-ecs / @rachidatecs Just for your awareness, I had to add this logic to fix the headers. They weren't displaying correctly on many subpages. I've include this for portfolio and domains.

The easier solution here would have been to check a url like domain-requests/{some url} though I don't think we current follow that scheme

"""Checks if the given page is a subpage of domain requests.
Takes a path name, like '/requests/'."""
# Since our pages aren't unified under a common path, we need this approach for now.
url_names = [
"domain-requests",
"no-portfolio-requests",
"domain-request-status",
"domain-request-withdraw-confirmation",
"domain-request-withdrawn",
"domain-request-delete",
]

# The domain request wizard pages don't have a defined path,
# so we need to check directly on it.
wizard_paths = [
DomainRequestWizard.EDIT_URL_NAME,
DomainRequestWizard.URL_NAMESPACE,
DomainRequestWizard.NEW_URL_NAME,
]
return get_url_name(path) in url_names or any(wizard in path for wizard in wizard_paths)


@register.filter(name="is_portfolio_subpage")
def is_portfolio_subpage(path):
"""Checks if the given page is a subpage of portfolio.
Takes a path name, like '/organization/'."""
# Since our pages aren't unified under a common path, we need this approach for now.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as the above comment that it may be worth revisiting how we organize our URL paths if we're adding more nested pages in the future

url_names = [
"organization",
"senior-official",
]
return get_url_name(path) in url_names
18 changes: 18 additions & 0 deletions src/registrar/tests/test_templatetags.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@
find_index,
slice_after,
contains_checkbox,
is_domain_request_subpage,
is_domain_subpage,
is_portfolio_subpage,
)


Expand Down Expand Up @@ -90,3 +93,18 @@ def test_contains_checkbox_without_checkbox(self):
]
result = contains_checkbox(html_list)
self.assertFalse(result) # Expecting False

def test_is_domain_subpage(self):
"""Tests if the path is recognized as a domain subpage."""
self.assertTrue(is_domain_subpage("/domains/"))
self.assertFalse(is_domain_subpage("/"))

def test_is_domain_request_subpage(self):
"""Tests if the path is recognized as a domain request subpage."""
self.assertTrue(is_domain_request_subpage("/requests/"))
self.assertFalse(is_domain_request_subpage("/"))

def test_is_portfolio_subpage(self):
"""Tests if the path is recognized as a portfolio subpage."""
self.assertTrue(is_portfolio_subpage("/organization/"))
self.assertFalse(is_portfolio_subpage("/"))
Loading
Loading