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

Use hx-preserve with table checkboxes #148

Merged
merged 14 commits into from
Oct 16, 2024
Merged
Show file tree
Hide file tree
Changes from 2 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
37 changes: 35 additions & 2 deletions process_manager/tables.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"""Tables for the process_manager app."""

import django_tables2 as tables
from django.utils.safestring import mark_safe

restart_column_template = (
"<a href={href} onclick=\"return confirm('{message}')\">{text}</a>".format(
Expand All @@ -26,6 +27,16 @@
"<a href=\"{% url 'process_manager:logs' record.uuid %}\">LOGS</a>"
)

header_checkbox_hyperscript = "on click set .row-checkbox.checked to my.checked"

row_checkbox_hyperscript = """
on click
if <input.row-checkbox:not(:checked)/> is empty
cc-a marked this conversation as resolved.
Show resolved Hide resolved
set #header-checkbox.checked to true
Copy link
Collaborator

Choose a reason for hiding this comment

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

In plain(er) English, this means that if the list of rows that are not checked is empty, then the header checkbox should be checked - as all the rows are checked - right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct.

else
set #header-checkbox.checked to false
"""


class ProcessTable(tables.Table):
"""Defines and Process Table for the data from the Process Manager."""
Expand All @@ -43,6 +54,28 @@ class Meta: # noqa: D106
select = tables.CheckBoxColumn(
accessor="uuid",
verbose_name="Select",
checked="checked",
attrs={"th__input": {"onclick": "toggle(this)"}},
attrs={
"th__input": {
"id": "header-checkbox",
"hx-preserve": "true",
"_": header_checkbox_hyperscript,
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does this underscore represent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is the html attribute that hyperscript uses to store its scripts on individual elements.

}
},
)

def render_select(self, value: str) -> str:
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's value here? Is the uuid of the column? Whatever it is, maybe add it to the docstring.

Also, I'm not sure when this method is called, or by whom.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks good catch. I wish there was a way to get automatic checks for this but I've never been able to find one.

It's called during table rendering. It's detected by class magicness and called in place of the render method of the column.

Copy link
Contributor

Choose a reason for hiding this comment

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

mkdocs gives warnings about malformed docstrings (=errors if you pass --strict), but I don't know if it warns about missing args. Were we going to add API documentation at some point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Planning to add it for next sprint.

"""Customise behaviour of checkboxes in the select column.

Overriding the default render method for this column is required as the
hx-preserve attitribute requires all elements to have unique id values. We also
need to add the hyperscript required for the header checkbox behaviour.

Called during table rendering.

Args:
value: uuid from the table row data
"""
return mark_safe(
Copy link
Contributor

Choose a reason for hiding this comment

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

So render_select is magically called when rendering self.select = tables.CheckBoxCol..., and creates a custom id for the row checkbox, so that hx-preserve can preserve each row cbox on refresh.

What is mark_safe doing here exactly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://docs.djangoproject.com/en/5.1/ref/utils/#django.utils.safestring.mark_safe It tells the Django templating system that the html generated is safe and isn't vurnerable to injection attacks so it can just be rendered directly. Otherwise the templating system by default will try to escape everything.

f'<input type="checkbox" name="select" value="{value}" id="{value}-input" '
f'hx-preserve="true" class="row-checkbox" _="{row_checkbox_hyperscript}"'
)
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{% load render_table from django_tables2 %}
<div hx-post="{% url 'process_manager:process_table' %}"
hx-trigger="load delay:1s, click from:input"
<div hx-get="{% url 'process_manager:process_table' %}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it get now because we are no longer sending data (the selected rows) to the backend?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right.

hx-trigger="load delay:1s"
hx-swap="outerHTML">{% render_table table %}</div>
7 changes: 0 additions & 7 deletions process_manager/views/partials.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ def process_table(request: HttpRequest) -> HttpResponse:
no check boxes selected. POST renders the table with checked boxes for any table row
with a uuid provided in the select key of the request data.
"""
selected_rows = request.POST.getlist("select", [])
session_info = get_session_info()

status_enum_lookup = dict(item[::-1] for item in ProcessInstance.StatusCode.items())
Expand All @@ -39,7 +38,6 @@ def process_table(request: HttpRequest) -> HttpResponse:
"session": metadata.session,
"status_code": status_enum_lookup[process_instance.status_code],
"exit_code": process_instance.return_code,
"checked": (uuid in selected_rows),
}
)
table = ProcessTable(table_data)
Expand All @@ -48,11 +46,6 @@ def process_table(request: HttpRequest) -> HttpResponse:
table_configurator = django_tables2.RequestConfig(request)
table_configurator.configure(table)

# If all rows are selected, we check the header box
# The value is irrelevant, we just need to set the attribute
if len(selected_rows) == len(table_data):
table.columns["select"].attrs["th__input"]["checked"] = "checked"

return render(
request=request,
context=dict(table=table),
Expand Down
48 changes: 8 additions & 40 deletions tests/process_manager/views/test_partial_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
from unittest.mock import MagicMock
from uuid import uuid4

import pytest
from django.urls import reverse
from pytest_django.asserts import assertTemplateUsed

Expand All @@ -16,13 +15,16 @@ class TestProcessTableView(LoginRequiredTest):

endpoint = reverse("process_manager:process_table")

@pytest.mark.parametrize("method", ("get", "post"))
def test_method(self, method, auth_client, mocker):
def test_get(self, auth_client, mocker):
"""Tests basic calls of view method."""
self._mock_session_info(mocker, [])
response = getattr(auth_client, method)(self.endpoint)
uuids = [str(uuid4()) for _ in range(5)]
self._mock_session_info(mocker, uuids)
response = auth_client.get(self.endpoint)
assert response.status_code == HTTPStatus.OK
assert isinstance(response.context["table"], ProcessTable)
table = response.context["table"]
assert isinstance(table, ProcessTable)
for row, uuid in zip(table.data.data, uuids):
assert row["uuid"] == uuid
cc-a marked this conversation as resolved.
Show resolved Hide resolved

def _mock_session_info(self, mocker, uuids):
"""Mocks views.get_session_info with ProcessInstanceList like data."""
Expand All @@ -34,40 +36,6 @@ def _mock_session_info(self, mocker, uuids):
mock().data.values.__iter__.return_value = instance_mocks
return mock

def test_post_checked_rows(self, mocker, auth_client):
"""Tests table data is correct when post data is included."""
all_uuids = [str(uuid4()) for _ in range(5)]
selected_uuids = all_uuids[::2]

self._mock_session_info(mocker, all_uuids)

response = auth_client.post(self.endpoint, data=dict(select=selected_uuids))
assert response.status_code == HTTPStatus.OK
table = response.context["table"]
assert isinstance(table, ProcessTable)

for row in table.data.data:
assert row["checked"] == (row["uuid"] in selected_uuids)
assert "checked" not in table.columns["select"].attrs["th__input"]

def test_post_header_checked(self, mocker, auth_client):
"""Tests header checkbox is checked if all rows are checked."""
all_uuids = [str(uuid4()) for _ in range(5)]
selected_uuids = all_uuids

self._mock_session_info(mocker, all_uuids)

response = auth_client.post(self.endpoint, data=dict(select=selected_uuids))
assert response.status_code == HTTPStatus.OK
table = response.context["table"]
assert isinstance(table, ProcessTable)

# All rows should be checked
assert all(row["checked"] for row in table.data.data)

# So header should be checked as well
assert table.columns["select"].attrs["th__input"]["checked"] == "checked"


class TestMessagesView(LoginRequiredTest):
"""Test the process_manager.views.messages view function."""
Expand Down