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

Feature/ucd 36 custom application history page #279

Merged
merged 5 commits into from
Sep 11, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
# Generated by Django 4.2.13 on 2024-09-09 14:08

from django.db import migrations


class Migration(migrations.Migration):
dependencies = [
("request", "0019_historicalregistrypublishedperson_and_more"),
]

operations = [
migrations.RemoveField(
model_name="historicalregistrantperson",
name="history_user",
),
migrations.RemoveField(
model_name="historicalregistrarperson",
name="history_user",
),
migrations.RemoveField(
model_name="historicalregistrarperson",
name="registrar",
),
migrations.RemoveField(
model_name="historicalregistrypublishedperson",
name="history_user",
),
migrations.DeleteModel(
name="HistoricalRegistrant",
),
migrations.DeleteModel(
name="HistoricalRegistrantPerson",
),
migrations.DeleteModel(
name="HistoricalRegistrarPerson",
),
migrations.DeleteModel(
name="HistoricalRegistryPublishedPerson",
),
]
6 changes: 1 addition & 5 deletions request_a_govuk_domain/request/models/organisation.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
from django.utils.translation import gettext_lazy as _
from django.db import models

from django.utils.translation import gettext_lazy as _
from simple_history.models import HistoricalRecords


Expand Down Expand Up @@ -49,9 +48,6 @@ class Registrant(models.Model):
name = models.CharField()
type = models.CharField(choices=RegistrantTypeChoices.choices, max_length=100)

# maintain history
history = HistoricalRecords()

class Meta:
unique_together = ("name", "type")

Expand Down
4 changes: 0 additions & 4 deletions request_a_govuk_domain/request/models/person.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
from django.db import models
from phonenumber_field.modelfields import PhoneNumberField
from simple_history.models import HistoricalRecords


class Person(models.Model):
Expand All @@ -13,9 +12,6 @@ class Person(models.Model):
email_address = models.EmailField(max_length=320)
phone_number = PhoneNumberField(blank=True)

# maintain history
history = HistoricalRecords(inherit=True)

def __str__(self):
return self.name

Expand Down
2 changes: 1 addition & 1 deletion request_a_govuk_domain/request/models/review.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@


# We've added simple-history to the dependencies but need to implement it,
# principally for this this class.
# principally for this class.
class Review(models.Model):
"""
An extension of the Application class (has a one-to-one) relationship
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
{% load i18n %}
{% load url from simple_history_compat %}
{% load admin_urls %}
{% load getattribute from getattributes %}
{% load admin_tags %}
<style>
.full_width {
width: 100%;
align-items: center;
}

h2 {
margin-top: 2%;
}
</style>
<table id="change-history" class="table table-bordered table-striped full_width">
<thead>
<tr>
<th scope="col">{% trans 'Date/time' %}</th>
<th scope="col">{% trans 'User' %}</th>
<th scope="col">{% trans 'Status' %}</th>

</tr>
</thead>
<tbody>
{% with filtered_actions=action_list %}
{% for action in filtered_actions %}
{% should_display action as should_display %}
{% if should_display %}
<tr>
<td>
<a href="{% url opts|admin_urlname:'simple_history' object.pk action.pk %}">{{ action.history_date }}</a>
</td>
<td>
{% if action.history_object.last_updated_by %}
{% url admin_user_view action.history_object.last_updated_by.id as admin_user_url %}
{% if admin_user_url %}
<a href="{{ admin_user_url }}">{{ action.history_object | owner_filter }}</a>
{% else %}
{{ action.history_object.last_updated_by }}
{% endif %}
{% else %}
{% trans "-" %}
{% endif %}
</td>
<td>
{{ action.history_object.status | capfirst }}
</td>
</tr>
{% endif %}
{% endfor %}
{% endwith %}
</tbody>
</table>

<h2>Full event history</h2>
<table id="change-history" class="table table-bordered table-striped full_width">
<thead>
<tr>
<th scope="col">{% trans 'Date/time' %}</th>
<th scope="col">{% trans 'Event' %}</th>
<th scope="col">{% trans 'Changed by' %}</th>
<th scope="col">{% trans 'Change fields' %}</th>
Copy link
Contributor

Choose a reason for hiding this comment

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

"Changed Fields" is better I think

</tr>
</thead>
<tbody>
{% for action in action_list %}
<tr>
<td>
<a href="{% url opts|admin_urlname:'simple_history' object.pk action.pk %}">{{ action.history_date }}</a>
</td>
<td>{{ action.get_history_type_display }}</td>
<td>
{% if action.history_object.last_updated_by %}
{% url admin_user_view action.history_object.last_updated_by.id as admin_user_url %}
{% if admin_user_url %}
<a href="{{ admin_user_url }}">{{ action.history_object.last_updated_by }}</a>
{% else %}
{{ action.history_object.last_updated_by }}
{% endif %}
{% else %}
{% trans "-" %}
{% endif %}
</td>
<td>
<i>{{ action | changed_fields }}</i>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why <i>? I don't think it's needed. It actually makes the text harder to read. Maybe you could also use something to join using commas as opposed to displaying the raw string. Better abc, def than ['abc', 'def']

</td>
</tr>
{% endfor %}
</tbody>
</table>
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
{% extends "admin/object_history.html" %}
{% load i18n %}
{% load url from simple_history_compat %}
{% load admin_urls %}
{% load display_list from simple_history_admin_list %}

{% block content %}
<div id="content-main">
<h2>{% blocktrans %} Status change history{% endblocktrans %}</h2>
Copy link
Contributor

Choose a reason for hiding this comment

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

why blocktrans?

<div class="govuk-table">
{% if action_list %}
{% display_list %}
{% else %}
<p>{% trans "This object doesn't have a change history." %}</p>
{% endif %}
</div>
</div>
{% endblock %}
Empty file.
53 changes: 53 additions & 0 deletions request_a_govuk_domain/request/templatetags/admin_tags.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
from django import template

register = template.Library()


@register.filter(is_safe=True)
def changed_fields(obj):
"""
Function to derive the changed fields from an object.
If none of the fields are changed and still, there is a history object, then it suggests
that the application was saved while updating the review.
:param obj:
:return:
"""
if obj.prev_record:
delta = obj.diff_against(obj.prev_record)
changes = list(filter(lambda x: x != "last_updated_by", delta.changed_fields))
return changes if changes else "Updated through review"
return "-"


@register.filter(is_safe=True)
def owner_filter(app):
"""
Extract the owner information from the application object.
"""
return (
f"{app.last_updated_by.username}"
if app.last_updated_by != app.owner
else f"{app.last_updated_by.username} (owner)"
)


@register.simple_tag
def should_display(action, **kwargs):
"""
Check the given history object should be displayed.
Returns true if this is the last record in the history or if there is a difference in the following
fields.
# status
# last_updated_by
# owner
:param action:
:param kwargs:
:return:
"""
return (
not action.next_record
or action.next_record.history_object.status != action.history_object.status
or action.next_record.history_object.last_updated_by
!= action.history_object.last_updated_by
or action.next_record.history_object.owner != action.history_object.owner
)