From 4a06bb1d0f988c74c667c57c9350c69bdfe52c66 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 23 May 2024 11:52:23 -0400 Subject: [PATCH] commented, and code cleaned up --- src/registrar/assets/js/get-gov.js | 87 +++++++++++++++++-- src/registrar/assets/js/uswds-edited.js | 5 ++ src/registrar/tests/test_views.py | 16 ++-- .../tests/test_views_domains_json.py | 6 +- .../tests/test_views_requests_json.py | 14 ++- src/registrar/views/domain_requests_json.py | 2 +- src/registrar/views/domains_json.py | 1 - 7 files changed, 111 insertions(+), 20 deletions(-) diff --git a/src/registrar/assets/js/get-gov.js b/src/registrar/assets/js/get-gov.js index 84e4d6e34..b46596a49 100644 --- a/src/registrar/assets/js/get-gov.js +++ b/src/registrar/assets/js/get-gov.js @@ -835,15 +835,18 @@ function hideDeletedForms() { HookupYesNoListener("additional_details-has_cisa_representative",'cisa-representative', null) })(); - +/** + * Initialize USWDS tooltips by calling initialization method. Requires that uswds-edited.js + * be loaded before get-gov.js. uswds-edited.js adds the tooltip module to the window to be + * accessible directly in get-gov.js + * + */ function initializeTooltips() { function checkTooltip() { - //console.log('Checking for tooltip...'); + // Check that the tooltip library is loaded, and if not, wait and retry if (window.tooltip && typeof window.tooltip.init === 'function') { - console.log('Tooltip is available, initializing...'); window.tooltip.init(); } else { - console.log('Tooltip not available, retrying...'); // Retry after a short delay setTimeout(checkTooltip, 100); } @@ -851,14 +854,36 @@ function initializeTooltips() { checkTooltip(); } +/** + * Initialize USWDS modals by calling on method. Requires that uswds-edited.js be loaded + * before get-gov.js. uswds-edited.js adds the modal module to the window to be accessible + * directly in get-gov.js. + * initializeModals adds modal-related DOM elements, based on other DOM elements existing in + * the page. It needs to be called only once for any particular DOM element; otherwise, it + * will initialize improperly. Therefore, if DOM elements change dynamically and include + * DOM elements with modal classes, unloadModals needs to be called before initializeModals. + * + */ function initializeModals() { window.modal.on(); } +/** + * Unload existing USWDS modals by calling off method. Requires that uswds-edited.js be + * loaded before get-gov.js. uswds-edited.js adds the modal module to the window to be + * accessible directly in get-gov.js. + * See note above with regards to calling this method relative to initializeModals. + * + */ function unloadModals() { window.modal.off(); } +/** + * An IIFE that listens for DOM Content to be loaded, then executes. This function + * initializes the domains list and associated functionality on the home page of the app. + * + */ document.addEventListener('DOMContentLoaded', function() { let currentPage = 1; let currentSortBy = 'id'; @@ -866,7 +891,15 @@ document.addEventListener('DOMContentLoaded', function() { let domainsWrapper = document.querySelector('.domains-wrapper'); let noDomainsWrapper = document.querySelector('.no-domains-wrapper'); + /** + * Loads rows in the domains list, as well as updates pagination around the domains list + * based on the supplied attributes. + * @param {*} page - the page number of the results (starts with 1) + * @param {*} sortBy - the sort column option + * @param {*} order - the sort order {asc, desc} + */ function loadPage(page, sortBy = currentSortBy, order = currentOrder) { + //fetch json of page of domains, given page # and sort fetch(`/get-domains-json/?page=${page}&sort_by=${sortBy}&order=${order}`) .then(response => response.json()) .then(data => { @@ -875,6 +908,7 @@ document.addEventListener('DOMContentLoaded', function() { return; } + // handle the display of proper messaging in the event that no domains exist in the list if (data.domains.length) { domainsWrapper.classList.remove('display-none'); noDomainsWrapper.classList.add('display-none'); @@ -883,6 +917,7 @@ document.addEventListener('DOMContentLoaded', function() { noDomainsWrapper.classList.remove('display-none'); } + // identify the DOM element where the domain list will be inserted into the DOM const domainList = document.querySelector('.dotgov-table__registered-domains tbody'); domainList.innerHTML = ''; @@ -922,8 +957,10 @@ document.addEventListener('DOMContentLoaded', function() { `; domainList.appendChild(row); }); + // initialize tool tips immediately after the associated DOM elements are added initializeTooltips(); + // update pagination updatePagination(data.page, data.num_pages, data.has_previous, data.has_next); currentPage = page; currentSortBy = sortBy; @@ -932,11 +969,19 @@ document.addEventListener('DOMContentLoaded', function() { .catch(error => console.error('Error fetching domains:', error)); } - + /** + * Update the pagination below the domains list. + * @param {*} currentPage - the current page number (starting with 1) + * @param {*} numPages - the number of pages indicated by the domains list response + * @param {*} hasPrevious - if there is a page of results prior to the current page + * @param {*} hasNext - if there is a page of results after the current page + */ function updatePagination(currentPage, numPages, hasPrevious, hasNext) { + // identify the DOM element where the pagination will be inserted const paginationContainer = document.querySelector('#domains-pagination .usa-pagination__list'); paginationContainer.innerHTML = ''; + // pagination should only be displayed if there are more than one pages of results paginationContainer.classList.toggle('display-none', numPages <= 1); if (hasPrevious) { @@ -989,9 +1034,12 @@ document.addEventListener('DOMContentLoaded', function() { header.addEventListener('click', function() { const sortBy = this.getAttribute('data-sortable'); let order = 'asc'; + // sort order will be ascending, unless the currently sorted column is ascending, and the user + // is selecting the same column to sort in descending order if (sortBy === currentSortBy) { order = currentOrder === 'asc' ? 'desc' : 'asc'; } + // load the results with the updated sort loadPage(1, sortBy, order); }); }); @@ -1000,6 +1048,11 @@ document.addEventListener('DOMContentLoaded', function() { loadPage(1); }); +/** + * An IIFE that listens for DOM Content to be loaded, then executes. This function + * initializes the domain requests list and associated functionality on the home page of the app. + * + */ document.addEventListener('DOMContentLoaded', function() { let currentPage = 1; let currentSortBy = 'id'; @@ -1007,7 +1060,15 @@ document.addEventListener('DOMContentLoaded', function() { let domainRequestsWrapper = document.querySelector('.domain-requests-wrapper'); let noDomainRequestsWrapper = document.querySelector('.no-domain-requests-wrapper'); + /** + * Loads rows in the domain requests list, as well as updates pagination around the domain requests list + * based on the supplied attributes. + * @param {*} page - the page number of the results (starts with 1) + * @param {*} sortBy - the sort column option + * @param {*} order - the sort order {asc, desc} + */ function loadDomainRequestsPage(page, sortBy = currentSortBy, order = currentOrder) { + //fetch json of page of domain requests, given page # and sort fetch(`/get-domain-requests-json/?page=${page}&sort_by=${sortBy}&order=${order}`) .then(response => response.json()) .then(data => { @@ -1016,6 +1077,7 @@ document.addEventListener('DOMContentLoaded', function() { return; } + // handle the display of proper messaging in the event that no domain requests exist in the list if (data.domain_requests.length) { domainRequestsWrapper.classList.remove('display-none'); noDomainRequestsWrapper.classList.add('display-none'); @@ -1024,9 +1086,12 @@ document.addEventListener('DOMContentLoaded', function() { noDomainRequestsWrapper.classList.remove('display-none'); } + // identify the DOM element where the domain request list will be inserted into the DOM const tbody = document.querySelector('.dotgov-table__domain-requests tbody'); tbody.innerHTML = ''; + // remove any existing modal elements from the DOM so they can be properly re-initialized + // after the DOM content changes and there are new delete modal buttons added unloadModals(); data.domain_requests.forEach(request => { const domainName = request.requested_domain ? request.requested_domain : `New domain request (${new Date(request.created_at).toLocaleString()} UTC)`; @@ -1070,8 +1135,10 @@ document.addEventListener('DOMContentLoaded', function() { `; tbody.appendChild(row); }); + // initialize modals immediately after the DOM content is updated initializeModals(); + // update the pagination after the domain requests list is updated updateDomainRequestsPagination(data.page, data.num_pages, data.has_previous, data.has_next); currentPage = page; currentSortBy = sortBy; @@ -1080,7 +1147,15 @@ document.addEventListener('DOMContentLoaded', function() { .catch(error => console.error('Error fetching domain requests:', error)); } + /** + * Update the pagination below the domain requests list. + * @param {*} currentPage - the current page number (starting with 1) + * @param {*} numPages - the number of pages indicated by the domain request list response + * @param {*} hasPrevious - if there is a page of results prior to the current page + * @param {*} hasNext - if there is a page of results after the current page + */ function updateDomainRequestsPagination(currentPage, numPages, hasPrevious, hasNext) { + // identify the DOM element where pagination is contained const paginationContainer = document.querySelector('#domain-requests-pagination .usa-pagination__list'); paginationContainer.innerHTML = ''; @@ -1136,6 +1211,8 @@ document.addEventListener('DOMContentLoaded', function() { header.addEventListener('click', function() { const sortBy = this.getAttribute('data-sortable'); let order = 'asc'; + // sort order will be ascending, unless the currently sorted column is ascending, and the user + // is selecting the same column to sort in descending order if (sortBy === currentSortBy) { order = currentOrder === 'asc' ? 'desc' : 'asc'; } diff --git a/src/registrar/assets/js/uswds-edited.js b/src/registrar/assets/js/uswds-edited.js index de92cac6c..f0f5886c9 100644 --- a/src/registrar/assets/js/uswds-edited.js +++ b/src/registrar/assets/js/uswds-edited.js @@ -5354,6 +5354,7 @@ modal = { } }; module.exports = modal; +// modified uswds.js to add modal module to window so that it is accessible to other js window.modal = modal; },{"../../uswds-core/src/js/config":35,"../../uswds-core/src/js/utils/focus-trap":47,"../../uswds-core/src/js/utils/scrollbar-width":51,"../../uswds-core/src/js/utils/select-or-matches":52}],29:[function(require,module,exports){ @@ -5543,6 +5544,7 @@ const getColumnHeaders = table => { * @param {HTMLTableHeaderCellElement} header */ const updateSortLabel = (header, headerName) => { + // modified original function to add headerName, as there were instances where tooltip had several lines of extraneous spaces if (headerName == null) headerName = header.innerText; const sortedAscending = header.getAttribute(SORTED) === ASCENDING; @@ -5647,6 +5649,7 @@ const toggleSort = (header, isAscending) => { */ const createHeaderButton = (header, headerName) => { + // modified original function to add headerName, as there were instances where tooltip had several lines of extraneous spaces const buttonEl = document.createElement("button"); buttonEl.setAttribute("tabindex", "0"); buttonEl.classList.add(SORT_BUTTON_CLASS); @@ -5678,6 +5681,7 @@ const table = behavior({ init(root) { const sortableHeaders = select(SORTABLE_HEADER, root); sortableHeaders.forEach(header => { + // modified original function to add headerName, as there were instances where tooltip had several lines of extraneous spaces createHeaderButton(header, header.innerText); }); const firstSorted = sortableHeaders.filter(header => header.getAttribute(SORTED) === ASCENDING || header.getAttribute(SORTED) === DESCENDING)[0]; @@ -6162,6 +6166,7 @@ const tooltip = behavior({ hide: hideToolTip }); module.exports = tooltip; +// modified uswds.js to add tooltip module to window so that it is accessible to other js window.tooltip = tooltip; },{"../../uswds-core/src/js/config":35,"../../uswds-core/src/js/utils/behavior":45,"../../uswds-core/src/js/utils/is-in-viewport":48,"../../uswds-core/src/js/utils/select-or-matches":52}],34:[function(require,module,exports){ diff --git a/src/registrar/tests/test_views.py b/src/registrar/tests/test_views.py index 24f6ffbf8..83054d6bd 100644 --- a/src/registrar/tests/test_views.py +++ b/src/registrar/tests/test_views.py @@ -185,11 +185,10 @@ class HomeTests(TestWithUser): user=self.user, domain=test_domain, role=UserDomainRole.Roles.MANAGER ) - # Grab the response + # Grab the json response for domain list response = self.client.get("/get-domains-json/") - # Make sure the user can actually see the domain. - # We expect two instances because of SR content. + # Make sure the domain is in the list. self.assertContains(response, domain_name, count=1) # Check that we have the right text content. @@ -208,11 +207,10 @@ class HomeTests(TestWithUser): UserDomainRole.objects.get_or_create(user=self.user, domain=test_domain, role=UserDomainRole.Roles.MANAGER) - # Grab the response + # Grab the json response of the domains list response = self.client.get("/get-domains-json/") - # Make sure the user can actually see the domain. - # We expect two instances because of SR content. + # Make sure the domain is in the response self.assertContains(response, "expired.gov", count=1) # Check that we have the right text content. @@ -229,9 +227,10 @@ class HomeTests(TestWithUser): UserDomainRole.objects.get_or_create(user=self.user, domain=test_domain, role=UserDomainRole.Roles.MANAGER) - # Grab the response + # Grab the json response of the domains list response = self.client.get("/get-domains-json/") + # Make sure domain is in the response self.assertContains(response, "imexpired.gov", count=1) # Make sure the expiration date is None @@ -248,9 +247,10 @@ class HomeTests(TestWithUser): UserDomainRole.objects.get_or_create(user=self.user, domain=test_domain_2, role=UserDomainRole.Roles.MANAGER) - # Grab the response + # Grab the json response of the domains list response = self.client.get("/get-domains-json/") + # Make sure the response contains the domain self.assertContains(response, "notexpired.gov", count=1) # Make sure the expiration date is None diff --git a/src/registrar/tests/test_views_domains_json.py b/src/registrar/tests/test_views_domains_json.py index deff194a3..d15349cb1 100644 --- a/src/registrar/tests/test_views_domains_json.py +++ b/src/registrar/tests/test_views_domains_json.py @@ -20,13 +20,14 @@ class GetDomainsJsonTest(TestWithUser, WebTest): UserDomainRole.objects.create(user=self.user, domain=self.domain3) def test_get_domains_json_unauthenticated(self): - # Logout the user + """ for an unauthenticated user, test that the user is redirected for auth """ self.app.reset() response = self.client.get(reverse("get_domains_json")) self.assertEqual(response.status_code, 302) def test_get_domains_json_authenticated(self): + """ Test that an authenticated user gets the list of 3 domains.""" response = self.app.get(reverse("get_domains_json")) self.assertEqual(response.status_code, 200) data = response.json @@ -45,6 +46,7 @@ class GetDomainsJsonTest(TestWithUser, WebTest): self.assertIn(self.domain3.id, domain_ids) def test_pagination(self): + """ Test that pagination is correct in the response """ response = self.app.get(reverse("get_domains_json"), {"page": 1}) self.assertEqual(response.status_code, 200) data = response.json @@ -56,6 +58,7 @@ class GetDomainsJsonTest(TestWithUser, WebTest): self.assertEqual(data["num_pages"], 1) def test_sorting(self): + """ test that sorting works properly in the response """ response = self.app.get(reverse("get_domains_json"), {"sort_by": "expiration_date", "order": "desc"}) self.assertEqual(response.status_code, 200) data = response.json @@ -73,6 +76,7 @@ class GetDomainsJsonTest(TestWithUser, WebTest): self.assertEqual(expiration_dates, sorted(expiration_dates)) def test_sorting_by_state_display(self): + """ test that the state_display sorting works properly """ response = self.app.get(reverse("get_domains_json"), {"sort_by": "state_display", "order": "asc"}) self.assertEqual(response.status_code, 200) data = response.json diff --git a/src/registrar/tests/test_views_requests_json.py b/src/registrar/tests/test_views_requests_json.py index ee193a6c9..c40da39a7 100644 --- a/src/registrar/tests/test_views_requests_json.py +++ b/src/registrar/tests/test_views_requests_json.py @@ -13,7 +13,7 @@ class DomainRequestViewTest(TestWithUser, WebTest): self.domain_requests = [ DomainRequest.objects.create( creator=self.user, - requested_domain=None, # Assuming requested_domain is an optional field + requested_domain=None, submission_date="2024-01-01", status=DomainRequest.DomainRequestStatus.STARTED, created_at="2024-01-01", @@ -41,7 +41,7 @@ class DomainRequestViewTest(TestWithUser, WebTest): ), DomainRequest.objects.create( creator=self.user, - requested_domain=None, # Assuming requested_domain is an optional field + requested_domain=None, submission_date="2024-05-01", status=DomainRequest.DomainRequestStatus.STARTED, created_at="2024-05-01", @@ -69,7 +69,7 @@ class DomainRequestViewTest(TestWithUser, WebTest): ), DomainRequest.objects.create( creator=self.user, - requested_domain=None, # Assuming requested_domain is an optional field + requested_domain=None, submission_date="2024-09-01", status=DomainRequest.DomainRequestStatus.STARTED, created_at="2024-09-01", @@ -98,6 +98,7 @@ class DomainRequestViewTest(TestWithUser, WebTest): ] def test_get_domain_requests_json_authenticated(self): + """ test that domain requests are returned properly for an authenticated user """ response = self.app.get(reverse("get_domain_requests_json")) self.assertEqual(response.status_code, 200) data = response.json @@ -114,6 +115,8 @@ class DomainRequestViewTest(TestWithUser, WebTest): self.assertNotEqual(domain_request["status"], "Approved") def test_pagination(self): + """ Test that pagination works properly. There are 11 total non-approved requests and + a page size of 10 """ response = self.app.get(reverse("get_domain_requests_json"), {"page": 1}) self.assertEqual(response.status_code, 200) data = response.json @@ -135,6 +138,7 @@ class DomainRequestViewTest(TestWithUser, WebTest): self.assertEqual(data["num_pages"], 2) def test_sorting(self): + """ test that sorting works properly on the result set """ response = self.app.get(reverse("get_domain_requests_json"), {"sort_by": "submission_date", "order": "desc"}) self.assertEqual(response.status_code, 200) data = response.json @@ -152,7 +156,9 @@ class DomainRequestViewTest(TestWithUser, WebTest): self.assertEqual(submission_dates, sorted(submission_dates)) def test_filter_approved_excluded(self): - response = self.app.get(reverse("get_domain_requests_json")) + """ test that approved requests are excluded from result set. """ + # sort in reverse chronological order of submission date, since most recent request is approved + response = self.app.get(reverse("get_domain_requests_json"), {"sort_by": "submission_date", "order": "desc"}) self.assertEqual(response.status_code, 200) data = response.json diff --git a/src/registrar/views/domain_requests_json.py b/src/registrar/views/domain_requests_json.py index d92c61f69..295776005 100644 --- a/src/registrar/views/domain_requests_json.py +++ b/src/registrar/views/domain_requests_json.py @@ -20,7 +20,7 @@ def get_domain_requests_json(request): sort_by = f"-{sort_by}" domain_requests = domain_requests.order_by(sort_by) page_number = request.GET.get("page", 1) - paginator = Paginator(domain_requests, 10) # Adjust the number of items per page as needed + paginator = Paginator(domain_requests, 10) page_obj = paginator.get_page(page_number) domain_requests_data = [ diff --git a/src/registrar/views/domains_json.py b/src/registrar/views/domains_json.py index 1c94ee463..48ccde483 100644 --- a/src/registrar/views/domains_json.py +++ b/src/registrar/views/domains_json.py @@ -41,7 +41,6 @@ def get_domains_json(request): "state": domain.state, "state_display": domain.state_display(), "get_state_help_text": domain.get_state_help_text(), - # Add other fields as necessary } for domain in page_obj.object_list ]