Fix (probably) reversion for trainingitemqualification

RIGS/models.py

@@ -569,7 +569,7 @@ class Invoice(models.Model, RevisionMixin):
 
     @property
     def activity_feed_string(self):
-        return "#{} for Event {}".format(self.display_id, self.event.display_id)
+        return f"#{self.display_id} for Event {self.event.display_id}"
 
     def __str__(self):
         return "%i: %s (%.2f)" % (self.pk, self.event, self.balance)
@@ -605,11 +605,11 @@ class Payment(models.Model, RevisionMixin):
     reversion_hide = True
 
     def __str__(self):
-        return "%s: %d" % (self.get_method_display(), self.amount)
+        return f"{self.get_method_display()}: {self.amount}"
 
     @property
     def activity_feed_string(self):
-        return str("payment of £{}".format(self.amount))
+        return f"payment of £{self.amount}"
 
 
 def validate_url(value):

training/models.py

@@ -6,7 +6,7 @@ from django.utils.safestring import mark_safe
 from versioning.versioning import RevisionMixin
 
 
-@reversion.register(for_concrete_model=False, fields=[], follow=["qualifications_obtained", "level_qualifications"])
+@reversion.register(for_concrete_model=False, fields=[])
 class Trainee(Profile, RevisionMixin):
     class Meta:
         proxy = True
@@ -39,6 +39,10 @@ class Trainee(Profile, RevisionMixin):
     def display_id(self):
         return str(self)
 
+    @property
+    def full_name(self):
+        return self.first_name + " " + self.last_name
+
 
 class TrainingCategory(models.Model):
     reference_number = models.IntegerField(unique=True)

training/tests/test_unit.py

@@ -4,9 +4,10 @@ import pytest
 from django.utils import timezone
 from django.urls import reverse
 
-from pytest_django.asserts import assertFormError, assertRedirects, assertContains, assertNotContains
+from pytest_django.asserts import assertFormError, assertRedirects, assertContains, assertNotContains, assertURLEqual
 
 from training import models
+from reversion.models import Version, Revision
 
 
 def test_add_qualification(admin_client, trainee, admin_user):
@@ -19,6 +20,22 @@ def test_add_qualification(admin_client, trainee, admin_user):
     assertFormError(response, 'form', 'supervisor', 'Selected supervisor must actually *be* a supervisor...')
 
 
+def test_add_qualification_reversion(admin_client, trainee, training_item, supervisor):
+    url = reverse('add_qualification', kwargs={'pk': trainee.pk})
+    date = (timezone.now() + datetime.timedelta(days=-3)).strftime("%Y-%m-%d")
+    response = admin_client.post(url, {'date': date, 'supervisor': supervisor.pk, 'trainee': trainee.pk, 'item': training_item.pk, 'depth': 0, 'notes': ""})
+    print(response.content)
+    assert response.status_code == 302
+    qual = models.TrainingItemQualification.objects.last()
+    assert qual is not None
+    assert training_item.pk == qual.pk
+    # Ensure only one revision has been created
+    assert Revision.objects.count() == 1
+    response = admin_client.post(url, {'date': date, 'supervisor': supervisor.pk, 'trainee': trainee.pk, 'item': training_item.pk, 'depth': 1})
+    assert Revision.objects.count() == 2
+    assert Version.objects.count() == 4  # Two item qualifications and the trainee twice
+
+
 def test_add_requirement(admin_client, level):
     url = reverse('add_requirement', kwargs={'pk': level.pk})
     response = admin_client.post(url)

training/views.py

@@ -9,6 +9,7 @@ from django.db.models import Q, Count
 from PyRIGS.views import is_ajax, ModalURLMixin
 from training import models, forms
 from users import views
+from reversion.views import RevisionMixin
 
 
 class ItemList(generic.ListView):
@@ -34,7 +35,7 @@ class TraineeDetail(views.ProfileDetail):
         if self.request.user.pk == self.object.pk:
             context["page_title"] = "Your Training Record"
         else:
-            context["page_title"] = "{}'s Training Record".format(self.object.first_name + " " + self.object.last_name)
+            context["page_title"] = f"{self.object.full_name}'s Training Record"
         context["started_levels"] = self.object.started_levels()
         context["completed_levels"] = self.object.level_qualifications.all()
         context["categories"] = models.TrainingCategory.objects.all().prefetch_related('items')
@@ -63,7 +64,7 @@ class TraineeItemDetail(generic.ListView):
         context = super().get_context_data(**kwargs)
         trainee = models.Trainee.objects.get(pk=self.kwargs['pk'])
         context["trainee"] = models.Trainee.objects.get(pk=self.kwargs['pk'])
-        context["page_title"] = "Detailed Training Record for <a href='{}'>{}</a>".format(trainee.get_absolute_url(), trainee)
+        context["page_title"] = f"Detailed Training Record for <a href='{trainee.get_absolute_url()}'>{trainee}</a>"
         return context
 
 
@@ -73,7 +74,7 @@ class LevelDetail(generic.DetailView):
 
     def get_context_data(self, **kwargs):
         context = super().get_context_data(**kwargs)
-        context["page_title"] = "Training Level {} {}".format(self.object, self.object.get_icon)
+        context["page_title"] = f"Training Level {self.object} {self.object.get_icon}"
         context["users_with"] = map(lambda qual: qual.trainee, models.TrainingLevelQualification.objects.filter(level=self.object))
         context["u"] = models.Trainee.objects.get(pk=self.kwargs['u']) if 'u' in self.kwargs else self.request.user
         return context
@@ -120,8 +121,6 @@ class AddQualification(generic.CreateView, ModalURLMixin):
     model = models.TrainingItemQualification
     form_class = forms.QualificationForm
 
-    @transaction.atomic()
-    @reversion.create_revision()
     def form_valid(self, form, *args, **kwargs):
         reversion.add_to_revision(form.cleaned_data['trainee'])
         return super().form_valid(form, *args, **kwargs)
@@ -133,14 +132,15 @@ class AddQualification(generic.CreateView, ModalURLMixin):
             context['override'] = "base_ajax.html"
         else:
             context['override'] = 'base_training.html'
-        context['page_title'] = "Add Qualification for {}".format(models.Trainee.objects.get(pk=self.kwargs['pk']))
+        trainee = models.Trainee.objects.get(pk=self.kwargs['pk'])
+        context['page_title'] = f"Add Qualification for {trainee}"
         return context
 
     def get_success_url(self):
-        return self.get_close_url('trainee_detail', 'trainee_detail')
+        return self.get_close_url('add_qualification', 'trainee_detail')
 
     def get_form_kwargs(self):
-        kwargs = super(AddQualification, self).get_form_kwargs()
+        kwargs = super().get_form_kwargs()
         kwargs['pk'] = self.kwargs['pk']
         return kwargs
 
@@ -153,7 +153,8 @@ class EditQualification(generic.UpdateView):
     def get_context_data(self, **kwargs):
         context = super().get_context_data(**kwargs)
         context["depths"] = models.TrainingItemQualification.CHOICES
-        context['page_title'] = "Edit Qualification {} for {}".format(self.object, models.Trainee.objects.get(pk=self.kwargs['pk']))
+        trainee = models.Trainee.objects.get(pk=self.kwargs['pk'])
+        context['page_title'] = f"Edit Qualification {self.object} for {trainee}"
         return context
 
     def get_form_kwargs(self):
@@ -175,7 +176,8 @@ class AddLevelRequirement(generic.CreateView, ModalURLMixin):
 
     def get_context_data(self, **kwargs):
         context = super().get_context_data(**kwargs)
-        context["page_title"] = "Add Requirements to Training Level {}".format(models.TrainingLevel.objects.get(pk=self.kwargs['pk']))
+        level = models.TrainingLevel.objects.get(pk=self.kwargs['pk'])
+        context["page_title"] = f"Add Requirements to Training Level {level}"
         return context
 
     def get_form_kwargs(self):

versioning/templates/partials/activity_table_body.html

@@ -14,8 +14,8 @@
         {% for version in object_list %}
             <tr>
                 <th scope="row">{{ version.revision.date_created }}</th>
-                <td><a href="{{ version.changes.new.get_absolute_url }}">{{ version.changes.new.display_id|default:version.changes.new.pk }} | {{version.changes.new|to_class_name}}</a></td>
+                <td><a href="{{ version.changes.new.get_absolute_url }}">{{ version.display_name }}</a></td>
-                <td>{{ version.pk }}|{{ version.revision.pk }}</td>
+                <td>{{ version.display_id }}</td>
                 <td>{% include 'partials/linked_name.html' with profile=version.revision.user %}</td>
                 <td>
                     {% if version.changes.old == None %}

versioning/templates/partials/version_history_table.html

@@ -13,7 +13,7 @@
         {% for version in object_list %}
             <tr>
                 <th scope="row">{{ version.revision.date_created }}</th>
-                <td>{{ version.pk }}|{{ version.revision.pk }}</td>
+                <td>{{ version.display_id }}</td>
                 <td>{{ version.revision.user.name }}</td>
                 <td>
                     {% if version.changes.old is None %}

versioning/versioning.py

@@ -36,7 +36,7 @@ class RevisionMixin:
         version = self.current_version
         if version is None:
             return None
-        return f"V{version.pk} | R{version.revision.pk}"
+        return version.display_id
 
     @property
     def date_created(self):
@@ -148,9 +148,11 @@ class ModelComparison:
     @cached_property
     def item_changes(self):
         from RIGS.models import EventAuthorisation
+        from training.models import TrainingLevelQualification, TrainingItemQualification
         if self.follow and self.version.object is not None:
             item_type = ContentType.objects.get_for_model(self.version.object)
-            old_item_versions = self.version.parent.revision.version_set.exclude(content_type=item_type)
+            old_item_versions = self.version.parent.revision.version_set.exclude(content_type=item_type).exclude(content_type=ContentType.objects.get_for_model(TrainingItemQualification)) \
+                .exclude(content_type=ContentType.objects.get_for_model(TrainingLevelQualification))
             new_item_versions = self.version.revision.version_set.exclude(content_type=item_type).exclude(content_type=ContentType.objects.get_for_model(EventAuthorisation))
 
             comparisonParams = {'excluded_keys': ['id', 'event', 'order', 'checklist', 'level', '_order', 'date_joined']}
@@ -234,3 +236,16 @@ class RIGSVersion(Version):
             old=self.parent._object_version.object if self.parent else None,
             follow=True
         )
+
+    @property
+    def display_id(self):
+        return f"V{self.pk} | R{self.revision.pk}"
+
+    @property
+    def display_name(self):
+        if hasattr(self.changes.new, 'display_id'):
+            id = self.changes.new.display_id
+        else:
+            id = self.changes.new.pk
+
+        return f"{id} | {self.changes.new.__class__.__name__}"