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

Tp2000 959 measure search results page revamp #1021

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
26 changes: 20 additions & 6 deletions common/jinja2/components/sort_by.jinja
Original file line number Diff line number Diff line change
@@ -1,14 +1,28 @@

{% macro sort_by(request, sort_by, title, base_url, anchor="") %}
{% macro sort_by(request, sort_by, title, base_url, query_params, anchor="") %}

{% if request.GET.sort_by == sort_by and request.GET.order == "desc" %}
<a class="govuk-link govuk-link--no-visited-state sort-icon--down" href="{{ base_url }}?sort_by={{ sort_by }}&order=asc{{ anchor }}">
{% if request.GET.sort_by == sort_by and request.GET.ordered == "desc" %}

{% if query_params %}
<a class="govuk-link govuk-link--no-visited-state sort-icon--down" href="{{ base_url }}&sort_by={{ sort_by }}&ordered=asc{{ anchor }}">
{{ title }} <img src="{{ static('common/images/sort_icon.svg') }}" alt="">
</a>
{% else %}
<a class="govuk-link govuk-link--no-visited-state sort-icon--down" href="{{ base_url }}?sort_by={{ sort_by }}&ordered=asc{{ anchor }}">
{{ title }} <img src="{{ static('common/images/sort_icon.svg') }}" alt="">
</a>
{% endif %}

{% else %}
{% if query_params %}
<a class="govuk-link govuk-link--no-visited-state sort-icon--up" href="{{ base_url }}&sort_by={{ sort_by }}&ordered=desc{{ anchor }}">
{{ title }} <img src="{{ static('common/images/sort_icon.svg') }}" alt="">
</a>
{% else %}
<a class="govuk-link govuk-link--no-visited-state sort-icon--up" href="{{ base_url }}?sort_by={{ sort_by }}&order=desc{{ anchor }}">
{% else %}
<a class="govuk-link govuk-link--no-visited-state sort-icon--up" href="{{ base_url }}?sort_by={{ sort_by }}&ordered=desc{{ anchor }}">
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 changed the name of the order to ordered as it clashed with the order number in the measures filter

{{ title }} <img src="{{ static('common/images/sort_icon.svg') }}" alt="">
</a>
{% endif %}
{% endif %}

{% endmacro %}
{% endmacro %}
2 changes: 2 additions & 0 deletions common/jinja2/layouts/list_vertical.jinja
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
{% from "components/input/macro.njk" import govukInput %}
{% from "components/table/macro.njk" import govukTable %}

{% from "components/sort_by.jinja" import sort_by %}

{% set create_url = create_url or "create" %}

{% set page_title = "Find and edit " ~ object_list.model._meta.verbose_name_plural %}
Expand Down
4 changes: 2 additions & 2 deletions common/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,7 @@ def get_queryset(self):

def get_ordering(self):
sort_by = self.request.GET.get("sort_by")
order = self.request.GET.get("order")
ordered = self.request.GET.get("ordered")
assert hasattr(
self,
"sort_by_fields",
Expand All @@ -399,7 +399,7 @@ def get_ordering(self):
if hasattr(self, "custom_sorting") and self.custom_sorting.get(sort_by):
sort_by = self.custom_sorting.get(sort_by)

if order == "desc":
if ordered == "desc":
sort_by = f"-{sort_by}"

return sort_by
Expand Down
30 changes: 25 additions & 5 deletions measures/jinja2/includes/measures/list.jinja
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
{% from "includes/measures/conditions.jinja" import conditions_list %}
{% from "components/button/macro.njk" import govukButton %}
{% from "components/warning-text/macro.njk" import govukWarningText %}
{% from "components/sort_by.jinja" import sort_by %}

{# Sets out checkbox #}
{% macro checkbox_item(field) -%}
Expand All @@ -26,6 +27,23 @@
<div id="check-all-checkbox"></div>
{%- endset %}

{% set sid %}
{{ sort_by(request, "sid", "ID", base_url, query_params) }}
{% endset %}

{% set measure_type %}
{{ sort_by(request, "measure_type", "Type", base_url, query_params) }}
{% endset %}

{% set geo_area %}
{{ sort_by(request, "geo_area", "Geographical area", base_url, query_params) }}
{% endset %}

{% set start_date %}
{{ sort_by(request, "start_date", "Start date", base_url, query_params) }}
{% endset %}


{# sets out form #}
<form method="post">
<input type="hidden" name="csrfmiddlewaretoken" value="{{ csrf_token }}">
Expand Down Expand Up @@ -62,22 +80,24 @@
{"text": create_link(measure.order_number.get_url(), measure.order_number.order_number) if measure.order_number else '-'},
{"text": footnotes_display(measure.footnoteassociationmeasure_set.current())},
{"text": conditions_list(measure) if measure.conditions.latest_approved() else "-", "classes": "govuk-!-width-one-quarter"},
{"text": create_link(url("regulation-ui-detail", kwargs={"role_type": measure.generating_regulation.role_type,"regulation_id": measure.generating_regulation.regulation_id}), measure.generating_regulation.regulation_id) if measure.generating_regulation.regulation_id else '-'},
]) or "" }}
{% endfor %}
{{ govukTable({
"head": [
{"html": checkbox_check_all},
{"text": "ID"},
{"text": "Type"},
{"text": "Commodity code"},
{"text": "Start date"},
{"text": sid},
{"text": measure_type},
{"text": "Commodity"},
{"text": start_date},
{"text": "End date"},
{"text": "Duties"},
{"text": "Additional code"},
{"text": "Geographical area"},
{"text": geo_area},
{"text": "Quota"},
{"text": "Footnote"},
{"text": "Conditions"},
{"text": "Regulation"},
],
"rows": table_rows,
"classes": "govuk-table-m"
Expand Down
125 changes: 124 additions & 1 deletion measures/jinja2/measures/list.jinja
Original file line number Diff line number Diff line change
@@ -1,4 +1,127 @@
{% extends "layouts/layout.jinja" %}
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 changed this template as the design wants some changes to the wording of the pagination, so thought it would make sense just to have it all in one template as it's a one off different design


{% from "components/button/macro.njk" import govukButton %}
{% from "components/input/macro.njk" import govukInput %}
{% from "components/table/macro.njk" import govukTable %}

{% set form_url = "measure-ui-list" %}
{% set list_include = "includes/measures/list.jinja" %}

{%- include "layouts/list_vertical.jinja" -%}
{% set page_title = "Find and edit " ~ object_list.model._meta.verbose_name_plural %}

{% block content %}
<h1 class="govuk-heading-xl">{{ page_title }}</h1>

<div class="full-width-search">
<div class="govuk-!-margin-bottom-5">
<h2 class="govuk-heading-m govuk-!-margin-bottom-5">Search and filter</h2>
<p class="govuk-body">
Select one or more options to search
</p>
<form method="get" action="{{ url(form_url) }}">
{{ crispy(filter.form) }}
</form>
</div>


<div class="govuk-!-margin-top-3">
{% if paginator.count > 0 %}

{% set objects_count = '{0:,}'.format(paginator.count) %}
{% if paginator.limit_breached %}
{% set objects_count = objects_count ~ '+' %}
{% endif %}

<p class="govuk-body-l">
{{ objects_count }} results
</p>

{% endif %}
{% if object_list %}
{% include list_include %}
{% else %}
<p class="govuk-body">There are no results for your search, please:</p>
<ul class="govuk-list govuk-list--bullet">
<li>check the spelling of your keywords</li>
<li>use more general keywords</li>
<li>select or deselect different filters</li>
<li>get help via our <a class="govuk-link" href="#">support and feedback form</a></li>
</ul>
{% endif %}


{% if page_obj.has_other_pages() %}
{% set objects_count = '{0:,}'.format(paginator.count) %}
{% set page_count = '{0:,}'.format(paginator.num_pages) %}
{% if paginator.limit_breached %}
{% set objects_count = 'more than ' ~ objects_count %}
{% set page_count = 'more than ' ~ page_count %}
{% endif %}

<nav class="pagination tamato-clearfix" role="navigation" aria-label="Pagination Navigation">
<div class="govuk-body">
Showing {{ page_obj|length }} of {{ objects_count }}
{# Polymorphic TrackedModelsQuerySet instances don't have a valid `model`. #}
{% if items_name %}
{{items_name}}
{% elif page_obj.object_list.model %}
{{ page_obj.object_list.model._meta.verbose_name_plural if paginator.count > 1 else page_obj.object_list.model._meta.verbose_name }}
{% else %}
"items"
{% endif %}
</div>
<div class="govuk-body align-left">
Page {{page_obj.number}} of {{ page_count }}
</div>
<ul class="govuk-list align-right">
{% if page_obj.has_previous() %}
<li>
<a
class="govuk-link govuk-!-margin-right-1"
href="?{{ query_transform(request, page=page_obj.previous_page_number())}}"
rel="prev"
aria-label="Goto Page {{ page_obj.previous_page_number() }}"
>
Prev
</a>
</li>
{% endif %}
{% for page_link in page_links %}
{% set isCurrent = page_link == page_obj.number %}
<li
class="{{ 'govuk-!-margin-left-2 govuk-!-margin-right-2' if isCurrent else 'govuk-!-margin-right-1'}}"
{% if isCurrent %}aria-current="true"{% endif %}
>
{% if page_link == '...' or page_link == page_obj.number|string %}
{{ page_link }}
{% else %}
<a
class="govuk-link"
href="?{{ query_transform(request, page=page_link) }}"
aria-label="Goto Page {{ page_link }}"
>{{ '{0:,}'.format(page_link|int) }}</a>
{% endif %}
</li>
{% endfor %}
{% if paginator.limit_breached %}
<li class="govuk-!-margin-right-1" >...</li>
{% endif %}
{% if page_obj.has_next() %}
<li>
<a
class="govuk-link"
href="?{{ query_transform(request, page=page_obj.next_page_number()) }}"
rel="next"
aria-label="Goto Page {{ page_obj.next_page_number() }}"
>
Next
</a>
</li>
{% endif %}
</ul>
</nav>
{% endif %}

</div>
</div>
{% endblock %}
59 changes: 57 additions & 2 deletions measures/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,11 @@

from common.forms import unprefix_formset_data
from common.models import TrackedModel
from common.pagination import build_pagination_list
from common.serializers import AutoCompleteSerializer
from common.util import TaricDateRange
from common.validators import UpdateType
from common.views import SortingMixin
from common.views import TamatoListView
from common.views import TrackedModelDetailMixin
from common.views import TrackedModelDetailView
Expand Down Expand Up @@ -125,21 +127,46 @@ class MeasureSearch(FilterView):
filterset_class = MeasureFilter

def form_valid(self, form):
print(form)
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 need to take this out oops

return HttpResponseRedirect(reverse("measure-ui-list"))


class MeasureList(MeasureSelectionMixin, MeasureMixin, FormView, TamatoListView):
class MeasureList(
MeasureSelectionMixin,
MeasureMixin,
SortingMixin,
FormView,
TamatoListView,
):
"""UI endpoint for viewing and filtering Measures."""

template_name = "measures/list.jinja"
filterset_class = MeasureFilter
form_class = SelectableObjectsForm
sort_by_fields = ["sid", "measure_type", "geo_area", "start_date"]
custom_sorting = {
"measure_type": "measure_type__sid",
"geo_area": "geographical_area__area_id",
"start_date": "valid_between",
}

def dispatch(self, *args, **kwargs):
if not self.request.GET:
return HttpResponseRedirect(reverse("measure-ui-search"))
return super().dispatch(*args, **kwargs)

def get_queryset(self):
queryset = super().get_queryset()

ordering = self.get_ordering()

if ordering:
if isinstance(ordering, str):
ordering = (ordering,)
queryset = queryset.order_by(*ordering)

return queryset

def get_form_kwargs(self):
kwargs = super().get_form_kwargs()
page = self.paginator.get_page(self.request.GET.get("page", 1))
Expand All @@ -153,14 +180,42 @@ def paginator(self):
return MeasurePaginator(self.filterset.qs, per_page=20)

def get_context_data(self, **kwargs):
context = super().get_context_data(**kwargs)
context = {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The get_context_data call ruined everything I had set in the paginator. It goes up the calling tree and somehow undoes all of the max counts, limits breached etc. I've taken the approach to manually do it in this get context.. and this seems to work and apply on the page.

paginator = self.paginator
page = paginator.get_page(self.request.GET.get("page", 1))
context["is_paginated"] = True
context["filter"] = kwargs["filter"]
context["form"] = self.get_form()
context["view"] = self
context["paginator"] = paginator
context["page_obj"] = page
context["object_list"] = page.object_list
context["page_links"] = build_pagination_list(
page.number,
page.paginator.num_pages,
)
measure_selections = [
SelectableObjectsForm.object_id_from_field_name(name)
for name in self.measure_selections
]
context["measure_selections"] = Measure.objects.filter(
pk__in=measure_selections,
)
context["query_params"] = True

# Remove sort by and order here, as the queryset will have already been ordered
if "sort_by" and "ordered" in self.filterset.data:
cleaned_filterset = self.filterset.data.copy()
cleaned_filterset.pop("sort_by")
cleaned_filterset.pop("ordered")
context[
"base_url"
] = f'{reverse("measure-ui-list")}?{urlencode(cleaned_filterset)}'
else:
context[
"base_url"
] = f'{reverse("measure-ui-list")}?{urlencode(self.filterset.data)}'

return context

def get_initial(self):
Expand Down
2 changes: 1 addition & 1 deletion settings/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -736,7 +736,7 @@
LIMITED_PAGINATOR_MAX_COUNT = 200
# Default max number of objects that will be accurately counted by MeasurePaginator.
MEASURES_PAGINATOR_MAX_COUNT = int(
os.environ.get("MEASURES_PAGINATOR_MAX_COUNT", "1000"),
os.environ.get("MEASURES_PAGINATOR_MAX_COUNT", "30000"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whacking the limit up from the previously unapplied 1000.. could be a mistake but 1000 measures doesn't seem sensible for what TM's will want to do at a time

)

# key used to instantiate GOVUK Notify python client
Expand Down
Loading