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.

Sign up for GitHub

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

#2232: Accessible markup and unit test coverage for admin lists #3290

Merged
merged 6 commits into from
Jan 8, 2025
Merged

#2232: Accessible markup and unit test coverage for admin lists #3290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
49c9002
accessible markup and unit test coverage
rachidatecs Jan 3, 2025
61bfec9
avoid flooding console
rachidatecs Jan 3, 2025
3b31a5c
merge main
rachidatecs Jan 3, 2025
60fcc88
change assert to satisfy linting
rachidatecs Jan 6, 2025
d71485e
Update src/registrar/tests/test_admin_request.py
rachidatecs Jan 8, 2025
e8ef9ea
Merge branch 'main' into el/2232-admin-list-markup
rachidatecs Jan 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions src/registrar/assets/src/sass/_theme/_admin.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,40 @@ div#content > h2 {
}
}

.module {
.margin-left-0 {
margin-left: 0;
}
.margin-top-0 {
margin-top: 0;
}
.padding-left-0 {
padding-left: 0;
}
}

.admin-list-inline {
li {
float: left;
padding-top: 0;
margin-right: 4px;
}
li:not(:last-child)::after {
content: ",";
}
}

.form-row {
.margin-y-0 {
margin-top: 0;
margin-bottom: 0;
}
.padding-y-0 {
padding-top: 0;
padding-bottom: 0;
}
}

// Fixes a display issue where the list was entirely white, or had too much whitespace
.select2-dropdown {
display: inline-grid !important;
Expand Down
38 changes: 27 additions & 11 deletions src/registrar/templates/django/admin/includes/detail_table_fieldset.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,22 +109,38 @@
This ONLY applies to analysts. For superusers, its business as usual.
{% endcomment %}
<div class="readonly">
{% with total_websites=field.contents|split:", " %}
{% for website in total_websites %}
<a href="{{ website }}" target="_blank" class="padding-top-1 current-website__{{forloop.counter}}">{{ website }}</a>{% if not forloop.last %}, {% endif %}
{# Acts as a <br> #}
{% if total_websites|length < 5 %}
<div class="display-block margin-top-1"></div>
{% with total_websites=field.contents|split:", " %}
{% if total_websites|length == 1 %}
<p class="margin-y-0 padding-y-0">
<a href="{{ total_websites.0 }}" target="_blank">
{{ total_websites.0 }}
</a>
</p>
{% elif total_websites|length > 1 %}
<ul class="margin-top-0 margin-left-0 padding-left-0{% if total_websites|length > 5 %} admin-list-inline{% endif %}">
{% for website in total_websites %}
{% comment %}White space matters: do NOT reformat the following line{% endcomment %}
<li><a href="{{ website }}" target="_blank">{{ website }}</a></li>
{% endfor %}
</ul>
{% endif %}
Comment on lines +112 to 126
Copy link
Contributor

Choose a reason for hiding this comment

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

Beautiful

{% endfor %}
{% endwith %}
{% endwith %}
</div>
{% elif field.field.name == "alternative_domains" %}
<div class="readonly">
{% with current_path=request.get_full_path %}
{% for alt_domain in original_object.alternative_domains.all %}
<a href="{% url 'admin:registrar_website_change' alt_domain.id %}?{{ 'return_path='|add:current_path }}">{{ alt_domain }}</a>{% if not forloop.last %}, {% endif %}
{% endfor %}
{% if original_object.alternative_domains.all|length == 1 %}
<p class="margin-y-0 padding-y-0">
<a href="{% url 'admin:registrar_website_change' original_object.alternative_domains.all.0.id %}?{{ 'return_path='|add:current_path }}" target="_blank">{{ original_object.alternative_domains.all.0 }}</a>
</p>
{% elif original_object.alternative_domains.all|length > 1 %}
<ul class="margin-top-0 margin-left-0 padding-left-0 admin-list-inline">
{% for alt_domain in original_object.alternative_domains.all %}
{% comment %}White space matters: do NOT reformat the following line{% endcomment %}
<li><a href="{% url 'admin:registrar_website_change' alt_domain.id %}?{{ 'return_path='|add:current_path }}" target="_blank">{{alt_domain}}</a></li>
{% endfor %}
</ul>
{% endif %}
Comment on lines +132 to +143
Copy link
Contributor

@zandercymatics zandercymatics Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah I see what you mean regarding the repetition. You made the right choice not to generalize it.
I think this is the "right way" to do things. I'm curious as to your thoughts: since it is the right way, do you think we should make it "known" in some way to other devs that we should follow this pattern in this case? Or does it not matter in your opinion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you think it's worth it, we can discuss in eng huddle

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it'd hurt, but its not too important either. I can add an optional PSA about when to list a list and when not to

{% endwith %}
</div>
{% elif field.field.name == "domain_managers" or field.field.name == "invited_domain_managers" %}
Expand Down
25 changes: 17 additions & 8 deletions src/registrar/tests/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
from django.utils.timezone import make_aware
from datetime import date, datetime, timedelta
from django.utils import timezone
from django.utils.html import strip_spaces_between_tags

from registrar.models import (
Contact,
Expand Down Expand Up @@ -107,6 +108,11 @@ def get_time_aware_date(date=datetime(2023, 11, 1)):
return timezone.make_aware(date)


def normalize_html(html):
"""Normalize HTML by removing newlines and extra spaces."""
return strip_spaces_between_tags(" ".join(html.split()))


class GenericTestHelper(TestCase):
"""A helper class that contains various helper functions for TestCases"""

Expand Down Expand Up @@ -1017,8 +1023,9 @@ def create_ready_domain():
# TODO in 1793: Remove the federal agency/updated federal agency fields
def completed_domain_request( # noqa
has_other_contacts=True,
has_current_website=True,
has_alternative_gov_domain=True,
# pass empty [] if you want current_websites or alternative_domains set to None
current_websites=["city.com"],
alternative_domains=["city1.gov"],
has_about_your_organization=True,
has_anything_else=True,
has_cisa_representative=True,
Expand Down Expand Up @@ -1050,8 +1057,6 @@ def completed_domain_request( # noqa
phone="(555) 555 5555",
)
domain, _ = DraftDomain.objects.get_or_create(name=name)
alt, _ = Website.objects.get_or_create(website="city1.gov")
current, _ = Website.objects.get_or_create(website="city.com")
other, _ = Contact.objects.get_or_create(
first_name="Testy",
last_name="Tester",
Expand Down Expand Up @@ -1118,10 +1123,14 @@ def completed_domain_request( # noqa

if has_other_contacts:
domain_request.other_contacts.add(other)
if has_current_website:
domain_request.current_websites.add(current)
if has_alternative_gov_domain:
domain_request.alternative_domains.add(alt)
if len(current_websites) > 0:
for website in current_websites:
current, _ = Website.objects.get_or_create(website=website)
domain_request.current_websites.add(current)
if len(alternative_domains) > 0:
for alternative_domain in alternative_domains:
alt, _ = Website.objects.get_or_create(website=alternative_domain)
domain_request.alternative_domains.add(alt)
if has_cisa_representative:
domain_request.cisa_representative_first_name = "CISA-first-name"
domain_request.cisa_representative_last_name = "CISA-last-name"
Expand Down
226 changes: 222 additions & 4 deletions src/registrar/tests/test_admin_request.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
multiple_unalphabetical_domain_objects,
MockEppLib,
GenericTestHelper,
normalize_html,
)
from unittest.mock import ANY, patch

Expand Down Expand Up @@ -1530,8 +1531,9 @@ def test_other_contacts_has_readonly_link(self):
self.assertContains(response, expected_url)

@less_console_noise_decorator
def test_other_websites_has_readonly_link(self):
"""Tests if the readonly other_websites field has links"""
def test_other_websites_has_one_readonly_link(self):
"""Tests if the readonly other_websites field has links.
Test markup for one website."""

# Create a fake domain request
domain_request = completed_domain_request(status=DomainRequest.DomainRequestStatus.IN_REVIEW)
Expand All @@ -1547,8 +1549,224 @@ def test_other_websites_has_readonly_link(self):
self.assertContains(response, domain_request.requested_domain.name)

# Check that the page contains the link we expect.
expected_url = '<a href="city.com" target="_blank" class="padding-top-1 current-website__1">city.com</a>'
self.assertContains(response, expected_url)
expected_markup = """
<p class="margin-y-0 padding-y-0">
<a href="city.com" target="_blank">
city.com
</a>
</p>
"""

normalized_expected = normalize_html(expected_markup)
normalized_response = normalize_html(response.content.decode("utf-8"))

index = normalized_response.find(normalized_expected)

# Assert that the expected markup is found in the response
if index == -1:
self.fail(
f"Expected markup not found in the response.\n\n"
f"Expected:\n{normalized_expected}\n\n"
f"Start index of mismatch: {index}\n\n"
f"Consider checking the surrounding response for context."
)
Comment on lines +1560 to +1572
Copy link
Contributor

Choose a reason for hiding this comment

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

I really like these tests. Nice work


@less_console_noise_decorator
def test_other_websites_has_few_readonly_links(self):
"""Tests if the readonly other_websites field has links.
Test markup for 5 or less websites."""

# Create a domain request with 4 current websites
domain_request = completed_domain_request(
status=DomainRequest.DomainRequestStatus.IN_REVIEW,
current_websites=["city.gov", "city2.gov", "city3.gov", "city4.gov"],
)

self.client.force_login(self.staffuser)
response = self.client.get(
"/admin/registrar/domainrequest/{}/change/".format(domain_request.pk),
follow=True,
)

# Make sure the page loaded, and that we're on the right page
self.assertEqual(response.status_code, 200)
self.assertContains(response, domain_request.requested_domain.name)

# Check that the page contains the link we expect.
expected_markup = """
<ul class="margin-top-0 margin-left-0 padding-left-0">
<li><a href="city.gov" target="_blank">city.gov</a></li>
<li><a href="city2.gov" target="_blank">city2.gov</a></li>
<li><a href="city3.gov" target="_blank">city3.gov</a></li>
<li><a href="city4.gov" target="_blank">city4.gov</a></li>
</ul>
"""

normalized_expected = normalize_html(expected_markup)
normalized_response = normalize_html(response.content.decode("utf-8"))

index = normalized_response.find(normalized_expected)

# Assert that the expected markup is found in the response
if index == -1:
self.fail(
f"Expected markup not found in the response.\n\n"
f"Expected:\n{normalized_expected}\n\n"
f"Start index of mismatch: {index}\n\n"
f"Consider checking the surrounding response for context."
)

@less_console_noise_decorator
def test_other_websites_has_lots_readonly_links(self):
"""Tests if the readonly other_websites field has links.
Test markup for 6 or more websites."""

# Create a domain requests with 6 current websites
domain_request = completed_domain_request(
status=DomainRequest.DomainRequestStatus.IN_REVIEW,
current_websites=["city.gov", "city2.gov", "city3.gov", "city4.gov", "city5.gov", "city6.gov"],
)

self.client.force_login(self.staffuser)
response = self.client.get(
"/admin/registrar/domainrequest/{}/change/".format(domain_request.pk),
follow=True,
)

# Make sure the page loaded, and that we're on the right page
self.assertEqual(response.status_code, 200)
self.assertContains(response, domain_request.requested_domain.name)

# Check that the page contains the link we expect.
expected_markup = """
<ul class="margin-top-0 margin-left-0 padding-left-0 admin-list-inline">
<li><a href="city.gov" target="_blank">city.gov</a></li>
<li><a href="city2.gov" target="_blank">city2.gov</a></li>
<li><a href="city3.gov" target="_blank">city3.gov</a></li>
<li><a href="city4.gov" target="_blank">city4.gov</a></li>
<li><a href="city5.gov" target="_blank">city5.gov</a></li>
<li><a href="city6.gov" target="_blank">city6.gov</a></li>
</ul>
"""

normalized_expected = normalize_html(expected_markup)
normalized_response = normalize_html(response.content.decode("utf-8"))

index = normalized_response.find(normalized_expected)

# Assert that the expected markup is found in the response
if index == -1:
self.fail(
f"Expected markup not found in the response.\n\n"
f"Expected:\n{normalized_expected}\n\n"
f"Start index of mismatch: {index}\n\n"
f"Consider checking the surrounding response for context."
)

@less_console_noise_decorator
def test_alternative_domains_has_one_readonly_link(self):
"""Tests if the readonly alternative_domains field has links.
Test markup for one website."""

# Create a fake domain request
domain_request = completed_domain_request(status=DomainRequest.DomainRequestStatus.IN_REVIEW)

self.client.force_login(self.staffuser)
response = self.client.get(
"/admin/registrar/domainrequest/{}/change/".format(domain_request.pk),
follow=True,
)

# Make sure the page loaded, and that we're on the right page
self.assertEqual(response.status_code, 200)
self.assertContains(response, domain_request.requested_domain.name)

# Check that the page contains the link we expect.
website = Website.objects.filter(website="city1.gov").first()
base_url = "/admin/registrar/website"
return_path = f"/admin/registrar/domainrequest/{domain_request.pk}/change/"
expected_markup = f"""
<p class="margin-y-0 padding-y-0">
<a href="{base_url}/{website.pk}/change/?return_path={return_path}" target="_blank">city1.gov</a>
</p>
"""

normalized_expected = normalize_html(expected_markup)
normalized_response = normalize_html(response.content.decode("utf-8"))

index = normalized_response.find(normalized_expected)

# Assert that the expected markup is found in the response
if index == -1:
self.fail(
f"Expected markup not found in the response.\n\n"
f"Expected:\n{normalized_expected}\n\n"
f"Start index of mismatch: {index}\n\n"
f"Consider checking the surrounding response for context."
)

@less_console_noise_decorator
def test_alternative_domains_has_lots_readonly_link(self):
"""Tests if the readonly other_websites field has links.
Test markup for 6 or more websites."""

# Create a domain request with 6 alternative domains
domain_request = completed_domain_request(
status=DomainRequest.DomainRequestStatus.IN_REVIEW,
alternative_domains=[
"altcity1.gov",
"altcity2.gov",
"altcity3.gov",
"altcity4.gov",
"altcity5.gov",
"altcity6.gov",
],
)

self.client.force_login(self.staffuser)
response = self.client.get(
"/admin/registrar/domainrequest/{}/change/".format(domain_request.pk),
follow=True,
)

# Make sure the page loaded, and that we're on the right page
self.assertEqual(response.status_code, 200)
self.assertContains(response, domain_request.requested_domain.name)

# Check that the page contains the link we expect.
website1 = Website.objects.filter(website="altcity1.gov").first()
website2 = Website.objects.filter(website="altcity2.gov").first()
website3 = Website.objects.filter(website="altcity3.gov").first()
website4 = Website.objects.filter(website="altcity4.gov").first()
website5 = Website.objects.filter(website="altcity5.gov").first()
website6 = Website.objects.filter(website="altcity6.gov").first()
base_url = "/admin/registrar/website"
return_path = f"/admin/registrar/domainrequest/{domain_request.pk}/change/"
attr = 'target="_blank"'
expected_markup = f"""
<ul class="margin-top-0 margin-left-0 padding-left-0 admin-list-inline">
<li><a href="{base_url}/{website1.pk}/change/?return_path={return_path}" {attr}>altcity1.gov</a></li>
<li><a href="{base_url}/{website2.pk}/change/?return_path={return_path}" {attr}>altcity2.gov</a></li>
<li><a href="{base_url}/{website3.pk}/change/?return_path={return_path}" {attr}>altcity3.gov</a></li>
<li><a href="{base_url}/{website4.pk}/change/?return_path={return_path}" {attr}>altcity4.gov</a></li>
<li><a href="{base_url}/{website5.pk}/change/?return_path={return_path}" {attr}>altcity5.gov</a></li>
<li><a href="{base_url}/{website6.pk}/change/?return_path={return_path}" {attr}>altcity6.gov</a></li>
</ul>
"""

normalized_expected = normalize_html(expected_markup)
normalized_response = normalize_html(response.content.decode("utf-8"))

index = normalized_response.find(normalized_expected)

# Assert that the expected markup is found in the response
if index == -1:
self.fail(
f"Expected markup not found in the response.\n\n"
f"Expected:\n{normalized_expected}\n\n"
f"Start index of mismatch: {index}\n\n"
f"Consider checking the surrounding response for context."
)

@less_console_noise_decorator
def test_contact_fields_have_detail_table(self):
Expand Down
Loading
Loading