From a8bce1a48f967faeae26f66a53af5b452ef765cb Mon Sep 17 00:00:00 2001 From: Tobias Bengfort Date: Mon, 19 Apr 2021 16:44:01 +0200 Subject: [PATCH 01/17] fix subject_filter fixture --- tests/conftest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/conftest.py b/tests/conftest.py index 7a25665f4..8aac23632 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -204,6 +204,6 @@ def subject_filter(subject_filter_group, attribute_description): return SubjectFilter.objects.create( description=attribute_description, operator='exact', - value='left', + value=1, group=subject_filter_group, ) -- GitLab From 466a77761f76a7bcd12a8fbf737f0e067cad8b75 Mon Sep 17 00:00:00 2001 From: Tobias Bengfort Date: Tue, 20 Apr 2021 12:11:08 +0200 Subject: [PATCH 02/17] do not include study in tag str --- castellum/studies/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/castellum/studies/models.py b/castellum/studies/models.py index f98fe4a1a..d2d8a07f8 100644 --- a/castellum/studies/models.py +++ b/castellum/studies/models.py @@ -383,4 +383,4 @@ class StudyTag(models.Model): name = models.CharField(_('Name'), max_length=128) def __str__(self): - return '{} - {}'.format(self.study, self.name) + return self.name -- GitLab From 7e4f3d5845e2ebb38fee3beea3589246c6453e8c Mon Sep 17 00:00:00 2001 From: Tobias Bengfort Date: Tue, 20 Apr 2021 15:41:50 +0200 Subject: [PATCH 03/17] allow to pass plain values to display filter --- castellum/utils/templatetags/utils.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/castellum/utils/templatetags/utils.py b/castellum/utils/templatetags/utils.py index f556d6524..175122fff 100644 --- a/castellum/utils/templatetags/utils.py +++ b/castellum/utils/templatetags/utils.py @@ -49,9 +49,11 @@ def verbose_name(instance, field_name): @register.filter -def display(instance, field_name): - getter = getattr(instance, 'get_{}_display'.format(field_name), None) - value = getter() if getter else getattr(instance, field_name) +def display(value, field_name=None): + if field_name: + getter = getattr(value, 'get_{}_display'.format(field_name), None) + value = getter() if getter else getattr(value, field_name) + if value is True: return _('Yes') elif value is False: -- GitLab From 58160545ea43403c22359db27a485aca043e70ef Mon Sep 17 00:00:00 2001 From: Tobias Bengfort Date: Mon, 19 Apr 2021 14:48:32 +0200 Subject: [PATCH 04/17] add study.snapshot model field --- .../studies/migrations/0031_study_snapshot.py | 18 ++++++++++++++++++ castellum/studies/models.py | 1 + 2 files changed, 19 insertions(+) create mode 100644 castellum/studies/migrations/0031_study_snapshot.py diff --git a/castellum/studies/migrations/0031_study_snapshot.py b/castellum/studies/migrations/0031_study_snapshot.py new file mode 100644 index 000000000..4fbf493b4 --- /dev/null +++ b/castellum/studies/migrations/0031_study_snapshot.py @@ -0,0 +1,18 @@ +# Generated by Django 3.2 on 2021-04-19 13:38 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('studies', '0030_domain_context'), + ] + + operations = [ + migrations.AddField( + model_name='study', + name='snapshot', + field=models.TextField(blank=True, editable=False, verbose_name='Snapshot'), + ), + ] diff --git a/castellum/studies/models.py b/castellum/studies/models.py index d2d8a07f8..cbda835f8 100644 --- a/castellum/studies/models.py +++ b/castellum/studies/models.py @@ -126,6 +126,7 @@ class Study(models.Model): blank=True, ) announce_status_changes = models.BooleanField(_('Announce status changes'), default=False) + snapshot = models.TextField(_('Snapshot'), blank=True, editable=False) members = models.ManyToManyField(User, through='StudyMembership') domains = GenericRelation(Domain) -- GitLab From 4071c320f234ba33afb073d0acea9353e813c67c Mon Sep 17 00:00:00 2001 From: Tobias Bengfort Date: Mon, 19 Apr 2021 15:14:11 +0200 Subject: [PATCH 05/17] store study snapshot on start --- castellum/studies/views/studies.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/castellum/studies/views/studies.py b/castellum/studies/views/studies.py index a59cc84a4..1f9d23d2f 100644 --- a/castellum/studies/views/studies.py +++ b/castellum/studies/views/studies.py @@ -24,6 +24,7 @@ import logging from django.conf import settings from django.contrib import messages from django.contrib.auth.mixins import LoginRequiredMixin +from django.core import serializers from django.core.exceptions import PermissionDenied from django.db import models from django.forms import modelform_factory @@ -49,6 +50,7 @@ from castellum.castellum_auth.mixins import PermissionRequiredMixin from castellum.pseudonyms.models import Domain from castellum.recruitment import filter_queries from castellum.recruitment.models import Participation +from castellum.recruitment.models import SubjectFilter from castellum.subjects.models import Subject from castellum.utils.mail import MailContext from castellum.utils.views import get_next_url @@ -319,6 +321,18 @@ class StudyStartRecruitmentView(StudyMixin, PermissionRequiredMixin, View): ) else: self.study.set_status(self.study.EXECUTION) + + self.study.snapshot = serializers.serialize('json', [ + self.study, + *self.study.studymembership_set.all(), + *self.study.domains.all(), + *self.study.studytag_set.all(), + *self.study.studysession_set.all(), + *self.study.subjectfiltergroup_set.all(), + *SubjectFilter.objects.filter(group__study=self.study), + ]) + self.study.save() + monitoring_logger.info( 'Study {} started by {}'.format(self.study.name, self.request.user.pk) ) -- GitLab From ad27b3f904edaca6941a34f6488a66246f41a9e2 Mon Sep 17 00:00:00 2001 From: Tobias Bengfort Date: Tue, 20 Apr 2021 11:00:59 +0200 Subject: [PATCH 06/17] exclude snapshot itself in snapshot --- castellum/studies/views/studies.py | 3 ++- castellum/utils/__init__.py | 8 ++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/castellum/studies/views/studies.py b/castellum/studies/views/studies.py index 1f9d23d2f..6913707d1 100644 --- a/castellum/studies/views/studies.py +++ b/castellum/studies/views/studies.py @@ -52,6 +52,7 @@ from castellum.recruitment import filter_queries from castellum.recruitment.models import Participation from castellum.recruitment.models import SubjectFilter from castellum.subjects.models import Subject +from castellum.utils import Exclusion from castellum.utils.mail import MailContext from castellum.utils.views import get_next_url @@ -330,7 +331,7 @@ class StudyStartRecruitmentView(StudyMixin, PermissionRequiredMixin, View): *self.study.studysession_set.all(), *self.study.subjectfiltergroup_set.all(), *SubjectFilter.objects.filter(group__study=self.study), - ]) + ], fields=Exclusion(['snapshot'])) self.study.save() monitoring_logger.info( diff --git a/castellum/utils/__init__.py b/castellum/utils/__init__.py index efca33b16..8f828e1d9 100644 --- a/castellum/utils/__init__.py +++ b/castellum/utils/__init__.py @@ -70,3 +70,11 @@ def get_parler_languages(site_id=None): names = dict(settings.LANGUAGES) codes = [lang['code'] for lang in settings.PARLER_LANGUAGES.get(site_id, [])] return [(code, names[code]) for code in codes] + + +class Exclusion: + def __init__(self, values): + self.values = values + + def __contains__(self, value): + return not (value in self.values) -- GitLab From 0bc72eb1a55cd57ad562dd33d029d667fcd74a5b Mon Sep 17 00:00:00 2001 From: Tobias Bengfort Date: Mon, 19 Apr 2021 15:28:46 +0200 Subject: [PATCH 07/17] add basic study diff view --- castellum/studies/templates/studies/study_base.html | 6 ++++++ castellum/studies/templates/studies/study_diff.html | 7 +++++++ castellum/studies/urls/__init__.py | 2 ++ castellum/studies/views/studies.py | 7 +++++++ 4 files changed, 22 insertions(+) create mode 100644 castellum/studies/templates/studies/study_diff.html diff --git a/castellum/studies/templates/studies/study_base.html b/castellum/studies/templates/studies/study_base.html index be2af5b1d..bb783520a 100644 --- a/castellum/studies/templates/studies/study_base.html +++ b/castellum/studies/templates/studies/study_base.html @@ -110,6 +110,12 @@ {% translate 'Overview' %} + {% if can_access_study and can_approve_study %} +
  • + {% translate 'Changes' %} +
    • + {% endif %} + {% if can_access_study and can_change_study %}
  • {% translate 'General' %} diff --git a/castellum/studies/templates/studies/study_diff.html b/castellum/studies/templates/studies/study_diff.html new file mode 100644 index 000000000..52c0f7c3a --- /dev/null +++ b/castellum/studies/templates/studies/study_diff.html @@ -0,0 +1,7 @@ +{% extends "studies/study_base.html" %} +{% load static i18n %} + +{% block title %}{% translate "Changes" %} · {{ block.super }}{% endblock %} + +{% block content %} +{% endblock %} diff --git a/castellum/studies/urls/__init__.py b/castellum/studies/urls/__init__.py index 79198b8a5..e6ce01f01 100644 --- a/castellum/studies/urls/__init__.py +++ b/castellum/studies/urls/__init__.py @@ -27,6 +27,7 @@ from ..views.recruitment import OneTimeInvitationMailRecruitmentView from ..views.studies import StudyCreateView from ..views.studies import StudyDeleteView from ..views.studies import StudyDetailView +from ..views.studies import StudyDiffView from ..views.studies import StudyExportView from ..views.studies import StudyFinishRecruitmentView from ..views.studies import StudyImportView @@ -49,6 +50,7 @@ urlpatterns = [ path('/start/', StudyStartRecruitmentView.as_view(), name='start'), path('/finish/', StudyFinishRecruitmentView.as_view(), name='finish'), path('/mail/', OneTimeInvitationMailRecruitmentView.as_view(), name='mail'), + path('/diff/', StudyDiffView.as_view(), name='diff'), path('/members/', include(members_urls)), path('/recruitmentsettings/', include(recruitment_urls)), path('/sessions/', include(sessions_urls)), diff --git a/castellum/studies/views/studies.py b/castellum/studies/views/studies.py index 6913707d1..358e88cc9 100644 --- a/castellum/studies/views/studies.py +++ b/castellum/studies/views/studies.py @@ -439,3 +439,10 @@ class StudyExportView(StudyMixin, PermissionRequiredMixin, View): filename = 'castellum_{}.json'.format(slugify(self.study.name)) response['Content-Disposition'] = 'attachment; filename="{}"'.format(filename) return response + + +class StudyDiffView(StudyMixin, PermissionRequiredMixin, DetailView): + model = Study + permission_required = 'studies.approve_study' + template_name = 'studies/study_diff.html' + tab = 'diff' -- GitLab From a683f29faec4fc24f24b4a8a270dd8e281778c41 Mon Sep 17 00:00:00 2001 From: Tobias Bengfort Date: Mon, 19 Apr 2021 16:12:07 +0200 Subject: [PATCH 08/17] diff view content --- .../studies/templates/studies/study_diff.html | 142 +++++++++++++++++- 1 file changed, 141 insertions(+), 1 deletion(-) diff --git a/castellum/studies/templates/studies/study_diff.html b/castellum/studies/templates/studies/study_diff.html index 52c0f7c3a..ccd9d25f7 100644 --- a/castellum/studies/templates/studies/study_diff.html +++ b/castellum/studies/templates/studies/study_diff.html @@ -1,7 +1,147 @@ {% extends "studies/study_base.html" %} -{% load static i18n %} +{% load static i18n utils %} {% block title %}{% translate "Changes" %} · {{ block.super }}{% endblock %} {% block content %} +
    +
    {{ study|verbose_name:'name' }}
    +
    {{ study|display:'name' }}
    + +
    {{ study|verbose_name:'contact_person' }}
    +
    {{ study|display:'contact_person' }}
    + +
    {{ study|verbose_name:'email' }}
    +
    {{ study|display:'email' }}
    + +
    {{ study|verbose_name:'phone' }}
    +
    {{ study|display:'phone' }}
    + +
    {{ study|verbose_name:'principal_investigator' }}
    +
    {{ study|display:'principal_investigator' }}
    + +
    {{ study|verbose_name:'affiliated_scientists' }}
    +
    {{ study|display:'affiliated_scientists' }}
    + +
    {{ study|verbose_name:'affiliated_research_assistants' }}
    +
    {{ study|display:'affiliated_research_assistants' }}
    + +
    {{ study|verbose_name:'description' }}
    +
    {{ study|display:'description' }}
    + +
    {{ study|verbose_name:'data_sensitivity' }}
    +
    {{ study|display:'data_sensitivity' }}
    + +
    {{ study|verbose_name:'min_subject_count' }}
    +
    {{ study|display:'min_subject_count' }}
    + +
    {{ study|verbose_name:'session_instructions' }}
    +
    {{ study|display:'session_instructions' }}
    + +
    {{ study|verbose_name:'sessions_start' }}
    +
    {{ study|display:'sessions_start' }}
    + +
    {{ study|verbose_name:'sessions_end' }}
    +
    {{ study|display:'sessions_end' }}
    + +
    {{ study|verbose_name:'excluded_studies' }}
    +
    {{ study|display:'excluded_studies' }}
    + +
    {% translate 'Tags' %}
    +
    {{ study|display:'studytag_set' }}
    + +
    {% translate 'Domains' %}
    +
    {{ study|display:'domains' }}
    + +
    {{ study|verbose_name:'exportable_attributes' }}
    +
    {{ study|display:'exportable_attributes' }}
    + +
    {{ study|verbose_name:'exclusion_criteria' }}
    +
    {{ study|display:'exclusion_criteria' }}
    + +
    {{ study|verbose_name:'recruitment_text' }}
    +
    {{ study|display:'recruitment_text' }}
    + +
    {{ study|verbose_name:'announce_status_changes' }}
    +
    {{ study|display:'announce_status_changes' }}
    + +
    {{ study|verbose_name:'advanced_filtering' }}
    +
    {{ study|display:'advanced_filtering' }}
    + +
    {{ study|verbose_name:'custom_filter' }}
    +
    {{ study|display:'custom_filter' }}
    + +
    {{ study|verbose_name:'geo_filter' }}
    +
    {{ study|display:'geo_filter' }}
    + +
    {{ study|verbose_name:'is_exclusive' }}
    +
    {{ study|display:'is_exclusive' }}
    + +
    {{ study|verbose_name:'complete_matches_only' }}
    +
    {{ study|display:'complete_matches_only' }}
    + +
    {{ study|verbose_name:'is_onetime_invitation' }}
    +
    {{ study|display:'is_onetime_invitation' }}
    + +
    {{ study|verbose_name:'consent' }}
    +
    {{ study|display:'consent' }}
    + +
    {{ study|verbose_name:'mail_subject' }}
    +
    {{ study|display:'mail_subject' }}
    + +
    {{ study|verbose_name:'mail_body' }}
    +
    {{ study|display:'mail_body' }}
    +
    + + {% with study.studymembership_set.all|list as memberships %} + {% if memberships %} +

    {% translate 'Memberships' %}

    +
    + {% for membership in memberships %} +
    {{ membership|display:'user' }}
    +
    {{ membership|display:'groups' }}
    + {% endfor %} +
    + {% endif %} + {% endwith %} + + {% with study.studysession_set.all|list as sessions %} + {% if sessions %} +

    {% translate 'Sessions' %}

    + {% for session in sessions %} +
    +
    {{ session|verbose_name:'name' }}
    +
    {{ session|display:'name' }}
    + +
    {{ session|verbose_name:'duration' }}
    +
    {{ session|display:'duration' }}
    + +
    {{ session|verbose_name:'type' }}
    +
    {{ session|display:'type' }}
    + +
    {{ session|verbose_name:'resource' }}
    +
    {{ session|display:'resource' }}
    + +
    {{ session|verbose_name:'reminder_text' }}
    +
    {{ session|display:'reminder_text' }}
    + +
    {{ session|verbose_name:'schedule_id' }}
    +
    {{ session|display:'schedule_id' }}
    +
    + {% endfor %} + {% endif %} + {% endwith %} + + {% with study.subjectfiltergroup_set.all|list as filtergroups %} + {% if filtergroups %} +

    {% translate 'Filters' %}

    + {% for filtergroup in filtergroups %} +
      + {% for f in filtergroup.subjectfilter_set.all %} +
    • {{ f }}
      • + {% endfor %} +
    + {% endfor %} + {% endif %} + {% endwith %} {% endblock %} -- GitLab From 9daf1c1f4968443b70b67fc8d2d82a6b933a7a6c Mon Sep 17 00:00:00 2001 From: Tobias Bengfort Date: Tue, 20 Apr 2021 18:04:29 +0200 Subject: [PATCH 09/17] test diff view for completeness --- .../studies/templates/studies/study_diff.html | 82 +++++++++---------- tests/studies/views/test_diff_view.py | 81 ++++++++++++++++++ 2 files changed, 122 insertions(+), 41 deletions(-) create mode 100644 tests/studies/views/test_diff_view.py diff --git a/castellum/studies/templates/studies/study_diff.html b/castellum/studies/templates/studies/study_diff.html index ccd9d25f7..64d446063 100644 --- a/castellum/studies/templates/studies/study_diff.html +++ b/castellum/studies/templates/studies/study_diff.html @@ -5,101 +5,101 @@ {% block content %}
    -
    {{ study|verbose_name:'name' }}
    +
    {{ study|verbose_name:'name' }}
    {{ study|display:'name' }}
    -
    {{ study|verbose_name:'contact_person' }}
    +
    {{ study|verbose_name:'contact_person' }}
    {{ study|display:'contact_person' }}
    -
    {{ study|verbose_name:'email' }}
    +
    {{ study|verbose_name:'email' }}
    {{ study|display:'email' }}
    -
    {{ study|verbose_name:'phone' }}
    +
    {{ study|verbose_name:'phone' }}
    {{ study|display:'phone' }}
    -
    {{ study|verbose_name:'principal_investigator' }}
    +
    {{ study|verbose_name:'principal_investigator' }}
    {{ study|display:'principal_investigator' }}
    -
    {{ study|verbose_name:'affiliated_scientists' }}
    +
    {{ study|verbose_name:'affiliated_scientists' }}
    {{ study|display:'affiliated_scientists' }}
    -
    {{ study|verbose_name:'affiliated_research_assistants' }}
    +
    {{ study|verbose_name:'affiliated_research_assistants' }}
    {{ study|display:'affiliated_research_assistants' }}
    -
    {{ study|verbose_name:'description' }}
    +
    {{ study|verbose_name:'description' }}
    {{ study|display:'description' }}
    -
    {{ study|verbose_name:'data_sensitivity' }}
    +
    {{ study|verbose_name:'data_sensitivity' }}
    {{ study|display:'data_sensitivity' }}
    -
    {{ study|verbose_name:'min_subject_count' }}
    +
    {{ study|verbose_name:'min_subject_count' }}
    {{ study|display:'min_subject_count' }}
    -
    {{ study|verbose_name:'session_instructions' }}
    +
    {{ study|verbose_name:'session_instructions' }}
    {{ study|display:'session_instructions' }}
    -
    {{ study|verbose_name:'sessions_start' }}
    +
    {{ study|verbose_name:'sessions_start' }}
    {{ study|display:'sessions_start' }}
    -
    {{ study|verbose_name:'sessions_end' }}
    +
    {{ study|verbose_name:'sessions_end' }}
    {{ study|display:'sessions_end' }}
    -
    {{ study|verbose_name:'excluded_studies' }}
    +
    {{ study|verbose_name:'excluded_studies' }}
    {{ study|display:'excluded_studies' }}
    -
    {% translate 'Tags' %}
    +
    {% translate 'Tags' %}
    {{ study|display:'studytag_set' }}
    -
    {% translate 'Domains' %}
    +
    {% translate 'Domains' %}
    {{ study|display:'domains' }}
    -
    {{ study|verbose_name:'exportable_attributes' }}
    +
    {{ study|verbose_name:'exportable_attributes' }}
    {{ study|display:'exportable_attributes' }}
    -
    {{ study|verbose_name:'exclusion_criteria' }}
    +
    {{ study|verbose_name:'exclusion_criteria' }}
    {{ study|display:'exclusion_criteria' }}
    -
    {{ study|verbose_name:'recruitment_text' }}
    +
    {{ study|verbose_name:'recruitment_text' }}
    {{ study|display:'recruitment_text' }}
    -
    {{ study|verbose_name:'announce_status_changes' }}
    +
    {{ study|verbose_name:'announce_status_changes' }}
    {{ study|display:'announce_status_changes' }}
    -
    {{ study|verbose_name:'advanced_filtering' }}
    +
    {{ study|verbose_name:'advanced_filtering' }}
    {{ study|display:'advanced_filtering' }}
    -
    {{ study|verbose_name:'custom_filter' }}
    +
    {{ study|verbose_name:'custom_filter' }}
    {{ study|display:'custom_filter' }}
    -
    {{ study|verbose_name:'geo_filter' }}
    +
    {{ study|verbose_name:'geo_filter' }}
    {{ study|display:'geo_filter' }}
    -
    {{ study|verbose_name:'is_exclusive' }}
    +
    {{ study|verbose_name:'is_exclusive' }}
    {{ study|display:'is_exclusive' }}
    -
    {{ study|verbose_name:'complete_matches_only' }}
    +
    {{ study|verbose_name:'complete_matches_only' }}
    {{ study|display:'complete_matches_only' }}
    -
    {{ study|verbose_name:'is_onetime_invitation' }}
    +
    {{ study|verbose_name:'is_onetime_invitation' }}
    {{ study|display:'is_onetime_invitation' }}
    -
    {{ study|verbose_name:'consent' }}
    +
    {{ study|verbose_name:'consent' }}
    {{ study|display:'consent' }}
    -
    {{ study|verbose_name:'mail_subject' }}
    +
    {{ study|verbose_name:'mail_subject' }}
    {{ study|display:'mail_subject' }}
    -
    {{ study|verbose_name:'mail_body' }}
    +
    {{ study|verbose_name:'mail_body' }}
    {{ study|display:'mail_body' }}
    {% with study.studymembership_set.all|list as memberships %} {% if memberships %} -

    {% translate 'Memberships' %}

    +

    {% translate 'Memberships' %}

    {% for membership in memberships %} -
    {{ membership|display:'user' }}
    -
    {{ membership|display:'groups' }}
    +
    {{ membership|display:'user' }}
    +
    {{ membership|display:'groups' }}
    {% endfor %}
    {% endif %} @@ -107,25 +107,25 @@ {% with study.studysession_set.all|list as sessions %} {% if sessions %} -

    {% translate 'Sessions' %}

    +

    {% translate 'Sessions' %}

    {% for session in sessions %}
    -
    {{ session|verbose_name:'name' }}
    +
    {{ session|verbose_name:'name' }}
    {{ session|display:'name' }}
    -
    {{ session|verbose_name:'duration' }}
    +
    {{ session|verbose_name:'duration' }}
    {{ session|display:'duration' }}
    -
    {{ session|verbose_name:'type' }}
    +
    {{ session|verbose_name:'type' }}
    {{ session|display:'type' }}
    -
    {{ session|verbose_name:'resource' }}
    +
    {{ session|verbose_name:'resource' }}
    {{ session|display:'resource' }}
    -
    {{ session|verbose_name:'reminder_text' }}
    +
    {{ session|verbose_name:'reminder_text' }}
    {{ session|display:'reminder_text' }}
    -
    {{ session|verbose_name:'schedule_id' }}
    +
    {{ session|verbose_name:'schedule_id' }}
    {{ session|display:'schedule_id' }}
    {% endfor %} @@ -134,11 +134,11 @@ {% with study.subjectfiltergroup_set.all|list as filtergroups %} {% if filtergroups %} -

    {% translate 'Filters' %}

    +

    {% translate 'Filters' %}

    {% for filtergroup in filtergroups %}
      {% for f in filtergroup.subjectfilter_set.all %} -
    • {{ f }}
      • +
    • {{ f }}
      • {% endfor %}
    {% endfor %} diff --git a/tests/studies/views/test_diff_view.py b/tests/studies/views/test_diff_view.py new file mode 100644 index 000000000..d601499aa --- /dev/null +++ b/tests/studies/views/test_diff_view.py @@ -0,0 +1,81 @@ +import re + +from django.apps import apps + +from model_bakery import baker + +from castellum.studies.models import StudySession +from castellum.studies.models import StudyTag + +EXCLUDED = [ + 'admin.', + 'appointments.Appointment.', + 'auth.', + 'castellum_auth.User.', + 'contacts.', + 'contenttypes.', + 'geofilters.Geolocation.', + 'pseudonyms.Pseudonym.', + 'recruitment.AttributeCategory.', + 'recruitment.AttributeCategoryTranslation.', + 'recruitment.AttributeChoice.', + 'recruitment.AttributeChoiceTranslation.', + 'recruitment.AttributeDescription.', + 'recruitment.AttributeDescriptionTranslation.', + 'recruitment.MailBatch.', + 'recruitment.NewsMailBatch.', + 'recruitment.Participation.', + 'recruitment.SubjectFilterGroup.id', + 'recruitment.SubjectFilterGroup.study', + 'sessions.', + 'studies.Resource.', + 'studies.StudyGroup.', + 'studies.Study.id', + 'studies.Study.mailbatch', + 'studies.Study.newsmailbatch', + 'studies.Study.participation', + 'studies.StudyMembership.id', + 'studies.StudyMembership.study', + 'studies.StudySession.appointment', + 'studies.StudySession.id', + 'studies.StudySession.study', + 'studies.StudyType.', + 'studies.StudyTypeEventFeed.', + 'studies.StudyTypeTranslation.', + 'subjects.', + + # already covered by other fields (e.g. backrefs) + 'pseudonyms.Domain.', # 'studies.Study.domains' + 'recruitment.SubjectFilter.', # recruitment.SubjectFilterGroup.subjectfilter + 'studies.Study.members', # studies.Study.studymembership_set + 'studies.StudyTag.', # studies.Study.studytag + + # intentionally left out + 'studies.Study.previous_status', + 'studies.Study.snapshot', + 'studies.Study.status', + 'studies.StudySession.domains', +] + + +def iter_fields(): + for app in apps.get_app_configs(): + for model in app.get_models(): + for field in model._meta.get_fields(): + s = '{}.{}.{}'.format(app.label, model._meta.object_name, field.name) + if all(not s.startswith(pattern) for pattern in EXCLUDED): + yield s + + +def test_study_diff_complete(client, member, study, subject_filter): + client.force_login(member) + + baker.make(StudySession, study=study) + baker.make(StudyTag, study=study) + + response = client.get('/studies/{}/diff/'.format(study.pk)) + + expected = set(iter_fields()) + actual = {s.decode('utf-8') for s in re.findall(b'data-fieldname="([^"]*)"', response.content)} + + assert actual == expected -- GitLab From 3ae8b9c1f04bda2fc5b1a4e2ffe4ea4887fe3a5a Mon Sep 17 00:00:00 2001 From: Tobias Bengfort Date: Tue, 20 Apr 2021 17:59:52 +0200 Subject: [PATCH 10/17] highlight changed fields --- castellum/static/style.css | 10 + .../studies/templates/studies/study_diff.html | 132 +++++++------ castellum/studies/views/studies.py | 10 + castellum/utils/templatetags/diff.py | 186 ++++++++++++++++++ 4 files changed, 273 insertions(+), 65 deletions(-) create mode 100644 castellum/utils/templatetags/diff.py diff --git a/castellum/static/style.css b/castellum/static/style.css index 091a17d5a..65abc6f81 100644 --- a/castellum/static/style.css +++ b/castellum/static/style.css @@ -159,3 +159,13 @@ .dl-inline dd { margin: 0 1em 0 0; } + +.diff-changed { + color: var(--orange); +} +.diff-added { + color: var(--green); +} +.diff-removed { + color: var(--red); +} diff --git a/castellum/studies/templates/studies/study_diff.html b/castellum/studies/templates/studies/study_diff.html index 64d446063..1efa01c9b 100644 --- a/castellum/studies/templates/studies/study_diff.html +++ b/castellum/studies/templates/studies/study_diff.html @@ -1,147 +1,149 @@ {% extends "studies/study_base.html" %} -{% load static i18n utils %} +{% load static i18n utils diff %} {% block title %}{% translate "Changes" %} · {{ block.super }}{% endblock %} {% block content %}
    {{ study|verbose_name:'name' }}
    -
    {{ study|display:'name' }}
    +
    {% diff_get study 'name' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {{ study|verbose_name:'contact_person' }}
    -
    {{ study|display:'contact_person' }}
    +
    {% diff_get study 'contact_person' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {{ study|verbose_name:'email' }}
    -
    {{ study|display:'email' }}
    +
    {% diff_get study 'email' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {{ study|verbose_name:'phone' }}
    -
    {{ study|display:'phone' }}
    +
    {% diff_get study 'phone' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {{ study|verbose_name:'principal_investigator' }}
    -
    {{ study|display:'principal_investigator' }}
    +
    {% diff_get study 'principal_investigator' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {{ study|verbose_name:'affiliated_scientists' }}
    -
    {{ study|display:'affiliated_scientists' }}
    +
    {% diff_get study 'affiliated_scientists' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {{ study|verbose_name:'affiliated_research_assistants' }}
    -
    {{ study|display:'affiliated_research_assistants' }}
    +
    {% diff_get study 'affiliated_research_assistants' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {{ study|verbose_name:'description' }}
    -
    {{ study|display:'description' }}
    +
    {% diff_get study 'description' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {{ study|verbose_name:'data_sensitivity' }}
    -
    {{ study|display:'data_sensitivity' }}
    +
    {% diff_get study 'data_sensitivity' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {{ study|verbose_name:'min_subject_count' }}
    -
    {{ study|display:'min_subject_count' }}
    +
    {% diff_get study 'min_subject_count' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {{ study|verbose_name:'session_instructions' }}
    -
    {{ study|display:'session_instructions' }}
    +
    {% diff_get study 'session_instructions' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {{ study|verbose_name:'sessions_start' }}
    -
    {{ study|display:'sessions_start' }}
    +
    {% diff_get study 'sessions_start' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {{ study|verbose_name:'sessions_end' }}
    -
    {{ study|display:'sessions_end' }}
    +
    {% diff_get study 'sessions_end' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {{ study|verbose_name:'excluded_studies' }}
    -
    {{ study|display:'excluded_studies' }}
    +
    {% diff_get study 'excluded_studies' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {% translate 'Tags' %}
    -
    {{ study|display:'studytag_set' }}
    +
    {% diff_get study 'studytag_set' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {% translate 'Domains' %}
    -
    {{ study|display:'domains' }}
    +
    {% diff_get study 'domains' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {{ study|verbose_name:'exportable_attributes' }}
    -
    {{ study|display:'exportable_attributes' }}
    +
    {% diff_get study 'exportable_attributes' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {{ study|verbose_name:'exclusion_criteria' }}
    -
    {{ study|display:'exclusion_criteria' }}
    +
    {% diff_get study 'exclusion_criteria' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {{ study|verbose_name:'recruitment_text' }}
    -
    {{ study|display:'recruitment_text' }}
    +
    {% diff_get study 'recruitment_text' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {{ study|verbose_name:'announce_status_changes' }}
    -
    {{ study|display:'announce_status_changes' }}
    +
    {% diff_get study 'announce_status_changes' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {{ study|verbose_name:'advanced_filtering' }}
    -
    {{ study|display:'advanced_filtering' }}
    +
    {% diff_get study 'advanced_filtering' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {{ study|verbose_name:'custom_filter' }}
    -
    {{ study|display:'custom_filter' }}
    +
    {% diff_get study 'custom_filter' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {{ study|verbose_name:'geo_filter' }}
    -
    {{ study|display:'geo_filter' }}
    +
    {% diff_get study 'geo_filter' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {{ study|verbose_name:'is_exclusive' }}
    -
    {{ study|display:'is_exclusive' }}
    +
    {% diff_get study 'is_exclusive' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {{ study|verbose_name:'complete_matches_only' }}
    -
    {{ study|display:'complete_matches_only' }}
    +
    {% diff_get study 'complete_matches_only' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {{ study|verbose_name:'is_onetime_invitation' }}
    -
    {{ study|display:'is_onetime_invitation' }}
    +
    {% diff_get study 'is_onetime_invitation' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {{ study|verbose_name:'consent' }}
    -
    {{ study|display:'consent' }}
    +
    {% diff_get study 'consent' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {{ study|verbose_name:'mail_subject' }}
    -
    {{ study|display:'mail_subject' }}
    +
    {% diff_get study 'mail_subject' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {{ study|verbose_name:'mail_body' }}
    -
    {{ study|display:'mail_body' }}
    +
    {% diff_get study 'mail_body' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    - {% with study.studymembership_set.all|list as memberships %} - {% if memberships %} -

    {% translate 'Memberships' %}

    -
    - {% for membership in memberships %} -
    {{ membership|display:'user' }}
    -
    {{ membership|display:'groups' }}
    - {% endfor %} -
    - {% endif %} - {% endwith %} - - {% with study.studysession_set.all|list as sessions %} - {% if sessions %} -

    {% translate 'Sessions' %}

    - {% for session in sessions %} + {% diff_get study 'studymembership_set' as memberships %} + {% if memberships %} +

    {% translate 'Memberships' %}

    +
    + {% for membership, class in memberships %} +
    {% diff_wrap class %}{{ membership|display:'user' }}{% end_diff_wrap %}
    +
    {% diff_get membership 'groups' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    + {% endfor %} +
    + {% endif %} + + {% diff_get study 'studysession_set' as sessions %} + {% if sessions %} +

    {% translate 'Sessions' %}

    + {% for session, class in sessions %} + {% diff_wrap class %}
    {{ session|verbose_name:'name' }}
    -
    {{ session|display:'name' }}
    +
    {% diff_get session 'name' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {{ session|verbose_name:'duration' }}
    -
    {{ session|display:'duration' }}
    +
    {% diff_get session 'duration' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {{ session|verbose_name:'type' }}
    -
    {{ session|display:'type' }}
    +
    {% diff_get session 'type' as type %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {{ session|verbose_name:'resource' }}
    -
    {{ session|display:'resource' }}
    +
    {% diff_get session 'resource' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {{ session|verbose_name:'reminder_text' }}
    -
    {{ session|display:'reminder_text' }}
    +
    {% diff_get session 'reminder_text' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {{ session|verbose_name:'schedule_id' }}
    -
    {{ session|display:'schedule_id' }}
    +
    {% diff_get session 'schedule_id' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    - {% endfor %} - {% endif %} - {% endwith %} - - {% with study.subjectfiltergroup_set.all|list as filtergroups %} - {% if filtergroups %} -

    {% translate 'Filters' %}

    - {% for filtergroup in filtergroups %} + {% end_diff_wrap %} + {% endfor %} + {% endif %} + + {% diff_get study 'subjectfiltergroup_set' as filtergroups %} + {% if filtergroups %} +

    {% translate 'Filters' %}

    + {% for filtergroup, class in filtergroups %} + {% diff_wrap class %}
      - {% for f in filtergroup.subjectfilter_set.all %} -
    • {{ f }}
      • + {% diff_get filtergroup 'subjectfilter_set' as filters %} + {% for f, class in filters %} +
    • {% diff_wrap class %}{{ f }}{% end_diff_wrap %}
      • {% endfor %}
    - {% endfor %} - {% endif %} - {% endwith %} + {% end_diff_wrap %} + {% endfor %} + {% endif %} {% endblock %} diff --git a/castellum/studies/views/studies.py b/castellum/studies/views/studies.py index 358e88cc9..905a227da 100644 --- a/castellum/studies/views/studies.py +++ b/castellum/studies/views/studies.py @@ -446,3 +446,13 @@ class StudyDiffView(StudyMixin, PermissionRequiredMixin, DetailView): permission_required = 'studies.approve_study' template_name = 'studies/study_diff.html' tab = 'diff' + + def get_context_data(self, **kwargs): + context = super().get_context_data(**kwargs) + try: + context['snapshot'] = list(serializers.deserialize( + 'json', self.study.snapshot, ignorenonexistent=True + )) + except serializers.base.DeserializationError: + context['snapshot'] = [] + return context diff --git a/castellum/utils/templatetags/diff.py b/castellum/utils/templatetags/diff.py new file mode 100644 index 000000000..3ee18d737 --- /dev/null +++ b/castellum/utils/templatetags/diff.py @@ -0,0 +1,186 @@ +# (c) 2018-2021 +# MPIB , +# MPI-CBS , +# MPIP +# +# This file is part of Castellum. +# +# Castellum is free software; you can redistribute it and/or modify it +# under the terms of the GNU Affero General Public License as published +# by the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# Castellum is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Affero General Public License for more details. +# +# You should have received a copy of the GNU Affero General Public +# License along with Castellum. If not, see +# . + +from django import template +from django.utils import html + +register = template.Library() + + +def register_container_tag(name=None, nargs=0): + def wrapped(node_class): + def do(parser, token): + bits = token.split_contents() + args = [parser.compile_filter(x) for x in bits[1:]] + if len(args) != nargs: + raise template.TemplateSyntaxError( + '{tag} tag requires exactly {nargs} arguments'.format( + tag=bits[0], nargs=nargs + ) + ) + + nodelist = parser.parse(['end_' + name]) + parser.delete_first_token() + + return node_class(nodelist, *args) + + register.tag(name=name)(do) + return node_class + + return wrapped + + +def get_old(snapshot, new_obj): + # equality on django models checks only for pk, not actual values + for old in snapshot: + if old.object == new_obj: + return old + raise KeyError + + +@register.simple_tag(takes_context=True) +def diff_class(context, new_obj, fieldname): + snapshot = context['snapshot'] + new_value = getattr(new_obj, fieldname) + + try: + old = get_old(snapshot, new_obj) + old_value = getattr(old.object, fieldname) + except KeyError: + return 'diff-added' + + if new_value == old_value: + return 'diff-unchanged' + elif not new_value: + return 'diff-removed' + elif not old_value: + return 'diff-added' + else: + return 'diff-changed' + + +def diff_get_m2m(snapshot, new_obj, fieldname): + field = getattr(new_obj, fieldname) + model = field.model + + try: + old = get_old(snapshot, new_obj) + old_values = set(old.m2m_data[fieldname]) + except KeyError: + old_values = set() + + for obj in field.all(): + if obj.pk in old_values: + old_values.remove(obj.pk) + yield obj, 'diff-unchanged' + else: + yield obj, 'diff-added' + for pk in old_values: + try: + obj = model.objects.get(pk=pk) + yield obj, 'diff-removed' + except model.DoesNotExist: + # Deletion of related models is not relevant for the diff + pass + + +def diff_get_o2m(snapshot, new_obj, fieldname): + field = getattr(new_obj, fieldname) + model = field.model + + old_objects = set() + for old in snapshot: + if isinstance(old.object, model) and getattr(old.object, field.field.attname) == new_obj.pk: + old_objects.add(old.object) + + for obj in field.all(): + try: + old = get_old(snapshot, obj) + old_objects.remove(obj) + if str(obj) == str(old.object): + yield obj, 'diff-unchanged' + else: + yield obj, 'diff-changed' + except KeyError: + yield obj, 'diff-added' + for obj in old_objects: + # Diffing the contents of obj recursively will result in + # 'diff-unchanged' in this case because this already is the old + # object. + yield obj, 'diff-removed' + + +@register.simple_tag(takes_context=True) +def diff_get(context, new_obj, fieldname): + snapshot = context['snapshot'] + try: + rel = getattr(new_obj.__class__, fieldname).rel + if rel.many_to_many: + return list(diff_get_m2m(snapshot, new_obj, fieldname)) + else: + return list(diff_get_o2m(snapshot, new_obj, fieldname)) + except AttributeError: + cls = diff_class(context, new_obj, fieldname) + value = getattr(new_obj, fieldname) + return [(value, cls)] + + +def diff_wrap(content, cls): + TAGS = { + 'diff-added': 'ins', + 'diff-removed': 'del', + } + tag = TAGS.get(cls, 'span') + if cls == 'diff-unchanged': + return content + return html.format_html('<{0} class="{1}">{2}', tag, cls, content) + + +@register_container_tag(name='diff_wrap', nargs=1) +class DiffWrapNode(template.Node): + def __init__(self, nodelist, cls): + self.nodelist = nodelist + self.cls = cls + + def render(self, context): + cls = self.cls.resolve(context) + content = self.nodelist.render(context) + return diff_wrap(content, cls) + + +@register_container_tag(name='diff_list', nargs=1) +class DiffListNode(template.Node): + def __init__(self, nodelist, items): + self.nodelist = nodelist + self.items = items + + def render(self, context): + items = self.items.resolve(context) + + def render_item(value, cls): + with context.push(): + context['value'] = value + context['class'] = cls + return self.nodelist.render(context) + + return html.mark_safe(', '.join( + diff_wrap(render_item(value, cls), cls) for value, cls in items + ) or '—') -- GitLab From 75e3774b3e44f1f7313532b7d5355d37a4a914dc Mon Sep 17 00:00:00 2001 From: Tobias Bengfort Date: Mon, 26 Apr 2021 15:00:51 +0200 Subject: [PATCH 11/17] simplify diff_wrap - can be implemented as simple tag - does not validate existance end_diff_wrap --- castellum/utils/templatetags/diff.py | 40 +++++++++++++++++----------- 1 file changed, 25 insertions(+), 15 deletions(-) diff --git a/castellum/utils/templatetags/diff.py b/castellum/utils/templatetags/diff.py index 3ee18d737..9f23817f9 100644 --- a/castellum/utils/templatetags/diff.py +++ b/castellum/utils/templatetags/diff.py @@ -24,6 +24,11 @@ from django.utils import html register = template.Library() +TAGS = { + 'diff-added': 'ins', + 'diff-removed': 'del', +} + def register_container_tag(name=None, nargs=0): def wrapped(node_class): @@ -143,27 +148,32 @@ def diff_get(context, new_obj, fieldname): return [(value, cls)] -def diff_wrap(content, cls): - TAGS = { - 'diff-added': 'ins', - 'diff-removed': 'del', - } +def _diff_wrap(content, cls): tag = TAGS.get(cls, 'span') if cls == 'diff-unchanged': return content return html.format_html('<{0} class="{1}">{2}', tag, cls, content) -@register_container_tag(name='diff_wrap', nargs=1) -class DiffWrapNode(template.Node): - def __init__(self, nodelist, cls): - self.nodelist = nodelist - self.cls = cls +@register.simple_tag(takes_context=True) +def diff_wrap(context, cls): + context.setdefault('_diff_wrap_stack', []) + context['_diff_wrap_stack'].append(cls) - def render(self, context): - cls = self.cls.resolve(context) - content = self.nodelist.render(context) - return diff_wrap(content, cls) + if cls == 'diff-unchanged': + return '' + tag = TAGS.get(cls, 'span') + return html.format_html('<{0} class="{1}">', tag, cls) + + +@register.simple_tag(takes_context=True) +def end_diff_wrap(context): + cls = context['_diff_wrap_stack'].pop() + + if cls == 'diff-unchanged': + return '' + tag = TAGS.get(cls, 'span') + return html.format_html('', tag) @register_container_tag(name='diff_list', nargs=1) @@ -182,5 +192,5 @@ class DiffListNode(template.Node): return self.nodelist.render(context) return html.mark_safe(', '.join( - diff_wrap(render_item(value, cls), cls) for value, cls in items + _diff_wrap(render_item(value, cls), cls) for value, cls in items ) or '—') -- GitLab From 1187385ee4344c63c34164b63de745f67bceb7ca Mon Sep 17 00:00:00 2001 From: Tobias Bengfort Date: Mon, 26 Apr 2021 15:07:33 +0200 Subject: [PATCH 12/17] further simplify diff_wrap --- .../studies/templates/studies/study_diff.html | 16 +++++++++----- castellum/utils/templatetags/diff.py | 22 +++---------------- 2 files changed, 13 insertions(+), 25 deletions(-) diff --git a/castellum/studies/templates/studies/study_diff.html b/castellum/studies/templates/studies/study_diff.html index 1efa01c9b..631250a8e 100644 --- a/castellum/studies/templates/studies/study_diff.html +++ b/castellum/studies/templates/studies/study_diff.html @@ -98,7 +98,9 @@

    {% translate 'Memberships' %}

    {% for membership, class in memberships %} -
    {% diff_wrap class %}{{ membership|display:'user' }}{% end_diff_wrap %}
    +
    + <{{ class|diff_tag }} class="{{ class }}">{{ membership|display:'user' }} +
    {% diff_get membership 'groups' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    {% endfor %}
    @@ -108,7 +110,7 @@ {% if sessions %}

    {% translate 'Sessions' %}

    {% for session, class in sessions %} - {% diff_wrap class %} + <{{ class|diff_tag }} class="{{ class }}">
    {{ session|verbose_name:'name' }}
    {% diff_get session 'name' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    @@ -128,7 +130,7 @@
    {{ session|verbose_name:'schedule_id' }}
    {% diff_get session 'schedule_id' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    - {% end_diff_wrap %} + {% endfor %} {% endif %} @@ -136,14 +138,16 @@ {% if filtergroups %}

    {% translate 'Filters' %}

    {% for filtergroup, class in filtergroups %} - {% diff_wrap class %} + <{{ class|diff_tag }} class="{{ class }}">
      {% diff_get filtergroup 'subjectfilter_set' as filters %} {% for f, class in filters %} -
    • {% diff_wrap class %}{{ f }}{% end_diff_wrap %}
      • +
    • + <{{ class|diff_tag }} class="{{ class }}">{{ f }} +
      • {% endfor %}
    - {% end_diff_wrap %} + {% endfor %} {% endif %} {% endblock %} diff --git a/castellum/utils/templatetags/diff.py b/castellum/utils/templatetags/diff.py index 9f23817f9..2d2271b4d 100644 --- a/castellum/utils/templatetags/diff.py +++ b/castellum/utils/templatetags/diff.py @@ -155,25 +155,9 @@ def _diff_wrap(content, cls): return html.format_html('<{0} class="{1}">{2}', tag, cls, content) -@register.simple_tag(takes_context=True) -def diff_wrap(context, cls): - context.setdefault('_diff_wrap_stack', []) - context['_diff_wrap_stack'].append(cls) - - if cls == 'diff-unchanged': - return '' - tag = TAGS.get(cls, 'span') - return html.format_html('<{0} class="{1}">', tag, cls) - - -@register.simple_tag(takes_context=True) -def end_diff_wrap(context): - cls = context['_diff_wrap_stack'].pop() - - if cls == 'diff-unchanged': - return '' - tag = TAGS.get(cls, 'span') - return html.format_html('', tag) +@register.filter +def diff_tag(cls): + return TAGS.get(cls, 'span') @register_container_tag(name='diff_list', nargs=1) -- GitLab From a89b1bc759208fbc82bd423b3f64bd7fff586f29 Mon Sep 17 00:00:00 2001 From: Tobias Bengfort Date: Mon, 26 Apr 2021 15:14:31 +0200 Subject: [PATCH 13/17] simplify diff_list hardcodes {{ value|display }} for content. But that is OK because this is just a shortcut anyway. --- .../studies/templates/studies/study_diff.html | 72 +++++++++---------- .../utils/templates/utils/__diff_list.html | 7 ++ castellum/utils/templatetags/diff.py | 52 +------------- 3 files changed, 46 insertions(+), 85 deletions(-) create mode 100644 castellum/utils/templates/utils/__diff_list.html diff --git a/castellum/studies/templates/studies/study_diff.html b/castellum/studies/templates/studies/study_diff.html index 631250a8e..fe53f7a25 100644 --- a/castellum/studies/templates/studies/study_diff.html +++ b/castellum/studies/templates/studies/study_diff.html @@ -6,91 +6,91 @@ {% block content %}
    {{ study|verbose_name:'name' }}
    -
    {% diff_get study 'name' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get study 'name' as items %}{% diff_list items %}
    {{ study|verbose_name:'contact_person' }}
    -
    {% diff_get study 'contact_person' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get study 'contact_person' as items %}{% diff_list items %}
    {{ study|verbose_name:'email' }}
    -
    {% diff_get study 'email' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get study 'email' as items %}{% diff_list items %}
    {{ study|verbose_name:'phone' }}
    -
    {% diff_get study 'phone' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get study 'phone' as items %}{% diff_list items %}
    {{ study|verbose_name:'principal_investigator' }}
    -
    {% diff_get study 'principal_investigator' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get study 'principal_investigator' as items %}{% diff_list items %}
    {{ study|verbose_name:'affiliated_scientists' }}
    -
    {% diff_get study 'affiliated_scientists' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get study 'affiliated_scientists' as items %}{% diff_list items %}
    {{ study|verbose_name:'affiliated_research_assistants' }}
    -
    {% diff_get study 'affiliated_research_assistants' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get study 'affiliated_research_assistants' as items %}{% diff_list items %}
    {{ study|verbose_name:'description' }}
    -
    {% diff_get study 'description' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get study 'description' as items %}{% diff_list items %}
    {{ study|verbose_name:'data_sensitivity' }}
    -
    {% diff_get study 'data_sensitivity' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get study 'data_sensitivity' as items %}{% diff_list items %}
    {{ study|verbose_name:'min_subject_count' }}
    -
    {% diff_get study 'min_subject_count' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get study 'min_subject_count' as items %}{% diff_list items %}
    {{ study|verbose_name:'session_instructions' }}
    -
    {% diff_get study 'session_instructions' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get study 'session_instructions' as items %}{% diff_list items %}
    {{ study|verbose_name:'sessions_start' }}
    -
    {% diff_get study 'sessions_start' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get study 'sessions_start' as items %}{% diff_list items %}
    {{ study|verbose_name:'sessions_end' }}
    -
    {% diff_get study 'sessions_end' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get study 'sessions_end' as items %}{% diff_list items %}
    {{ study|verbose_name:'excluded_studies' }}
    -
    {% diff_get study 'excluded_studies' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get study 'excluded_studies' as items %}{% diff_list items %}
    {% translate 'Tags' %}
    -
    {% diff_get study 'studytag_set' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get study 'studytag_set' as items %}{% diff_list items %}
    {% translate 'Domains' %}
    -
    {% diff_get study 'domains' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get study 'domains' as items %}{% diff_list items %}
    {{ study|verbose_name:'exportable_attributes' }}
    -
    {% diff_get study 'exportable_attributes' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get study 'exportable_attributes' as items %}{% diff_list items %}
    {{ study|verbose_name:'exclusion_criteria' }}
    -
    {% diff_get study 'exclusion_criteria' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get study 'exclusion_criteria' as items %}{% diff_list items %}
    {{ study|verbose_name:'recruitment_text' }}
    -
    {% diff_get study 'recruitment_text' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get study 'recruitment_text' as items %}{% diff_list items %}
    {{ study|verbose_name:'announce_status_changes' }}
    -
    {% diff_get study 'announce_status_changes' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get study 'announce_status_changes' as items %}{% diff_list items %}
    {{ study|verbose_name:'advanced_filtering' }}
    -
    {% diff_get study 'advanced_filtering' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get study 'advanced_filtering' as items %}{% diff_list items %}
    {{ study|verbose_name:'custom_filter' }}
    -
    {% diff_get study 'custom_filter' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get study 'custom_filter' as items %}{% diff_list items %}
    {{ study|verbose_name:'geo_filter' }}
    -
    {% diff_get study 'geo_filter' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get study 'geo_filter' as items %}{% diff_list items %}
    {{ study|verbose_name:'is_exclusive' }}
    -
    {% diff_get study 'is_exclusive' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get study 'is_exclusive' as items %}{% diff_list items %}
    {{ study|verbose_name:'complete_matches_only' }}
    -
    {% diff_get study 'complete_matches_only' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get study 'complete_matches_only' as items %}{% diff_list items %}
    {{ study|verbose_name:'is_onetime_invitation' }}
    -
    {% diff_get study 'is_onetime_invitation' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get study 'is_onetime_invitation' as items %}{% diff_list items %}
    {{ study|verbose_name:'consent' }}
    -
    {% diff_get study 'consent' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get study 'consent' as items %}{% diff_list items %}
    {{ study|verbose_name:'mail_subject' }}
    -
    {% diff_get study 'mail_subject' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get study 'mail_subject' as items %}{% diff_list items %}
    {{ study|verbose_name:'mail_body' }}
    -
    {% diff_get study 'mail_body' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get study 'mail_body' as items %}{% diff_list items %}
    {% diff_get study 'studymembership_set' as memberships %} @@ -101,7 +101,7 @@
    <{{ class|diff_tag }} class="{{ class }}">{{ membership|display:'user' }}
    -
    {% diff_get membership 'groups' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get membership 'groups' as items %}{% diff_list items %}
    {% endfor %} {% endif %} @@ -113,22 +113,22 @@ <{{ class|diff_tag }} class="{{ class }}">
    {{ session|verbose_name:'name' }}
    -
    {% diff_get session 'name' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get session 'name' as items %}{% diff_list items %}
    {{ session|verbose_name:'duration' }}
    -
    {% diff_get session 'duration' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get session 'duration' as items %}{% diff_list items %}
    {{ session|verbose_name:'type' }}
    -
    {% diff_get session 'type' as type %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get session 'type' as type %}{% diff_list items %}
    {{ session|verbose_name:'resource' }}
    -
    {% diff_get session 'resource' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get session 'resource' as items %}{% diff_list items %}
    {{ session|verbose_name:'reminder_text' }}
    -
    {% diff_get session 'reminder_text' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get session 'reminder_text' as items %}{% diff_list items %}
    {{ session|verbose_name:'schedule_id' }}
    -
    {% diff_get session 'schedule_id' as items %}{% diff_list items %}{{ value|display }}{% end_diff_list %}
    +
    {% diff_get session 'schedule_id' as items %}{% diff_list items %}
    {% endfor %} diff --git a/castellum/utils/templates/utils/__diff_list.html b/castellum/utils/templates/utils/__diff_list.html new file mode 100644 index 000000000..1069d5f7d --- /dev/null +++ b/castellum/utils/templates/utils/__diff_list.html @@ -0,0 +1,7 @@ +{% load utils diff %} + +{% for value, class in items %} + <{{ class|diff_tag }} class="{{ class }}">{{ value|display }}{% if not forloop.last %},{% endif %} +{% empty %} + — +{% endfor %} diff --git a/castellum/utils/templatetags/diff.py b/castellum/utils/templatetags/diff.py index 2d2271b4d..0771bf086 100644 --- a/castellum/utils/templatetags/diff.py +++ b/castellum/utils/templatetags/diff.py @@ -20,7 +20,6 @@ # . from django import template -from django.utils import html register = template.Library() @@ -30,29 +29,6 @@ TAGS = { } -def register_container_tag(name=None, nargs=0): - def wrapped(node_class): - def do(parser, token): - bits = token.split_contents() - args = [parser.compile_filter(x) for x in bits[1:]] - if len(args) != nargs: - raise template.TemplateSyntaxError( - '{tag} tag requires exactly {nargs} arguments'.format( - tag=bits[0], nargs=nargs - ) - ) - - nodelist = parser.parse(['end_' + name]) - parser.delete_first_token() - - return node_class(nodelist, *args) - - register.tag(name=name)(do) - return node_class - - return wrapped - - def get_old(snapshot, new_obj): # equality on django models checks only for pk, not actual values for old in snapshot: @@ -148,33 +124,11 @@ def diff_get(context, new_obj, fieldname): return [(value, cls)] -def _diff_wrap(content, cls): - tag = TAGS.get(cls, 'span') - if cls == 'diff-unchanged': - return content - return html.format_html('<{0} class="{1}">{2}', tag, cls, content) - - @register.filter def diff_tag(cls): return TAGS.get(cls, 'span') -@register_container_tag(name='diff_list', nargs=1) -class DiffListNode(template.Node): - def __init__(self, nodelist, items): - self.nodelist = nodelist - self.items = items - - def render(self, context): - items = self.items.resolve(context) - - def render_item(value, cls): - with context.push(): - context['value'] = value - context['class'] = cls - return self.nodelist.render(context) - - return html.mark_safe(', '.join( - _diff_wrap(render_item(value, cls), cls) for value, cls in items - ) or '—') +@register.inclusion_tag('utils/__diff_list.html') +def diff_list(items): + return {'items': items} -- GitLab From 8ced249c80ada2a67c97ed5c7c619e537853c341 Mon Sep 17 00:00:00 2001 From: Tobias Bengfort Date: Mon, 26 Apr 2021 15:18:40 +0200 Subject: [PATCH 14/17] rm unused diff_class templatetag --- castellum/utils/templatetags/diff.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/castellum/utils/templatetags/diff.py b/castellum/utils/templatetags/diff.py index 0771bf086..24cce878d 100644 --- a/castellum/utils/templatetags/diff.py +++ b/castellum/utils/templatetags/diff.py @@ -37,9 +37,7 @@ def get_old(snapshot, new_obj): raise KeyError -@register.simple_tag(takes_context=True) -def diff_class(context, new_obj, fieldname): - snapshot = context['snapshot'] +def diff_class(snapshot, new_obj, fieldname): new_value = getattr(new_obj, fieldname) try: @@ -119,7 +117,7 @@ def diff_get(context, new_obj, fieldname): else: return list(diff_get_o2m(snapshot, new_obj, fieldname)) except AttributeError: - cls = diff_class(context, new_obj, fieldname) + cls = diff_class(snapshot, new_obj, fieldname) value = getattr(new_obj, fieldname) return [(value, cls)] -- GitLab From 0f9652b715cd01211135dcdf9569105293bb76cd Mon Sep 17 00:00:00 2001 From: Tobias Bengfort Date: Mon, 26 Apr 2021 17:36:07 +0200 Subject: [PATCH 15/17] add some tests --- castellum/studies/views/studies.py | 25 ++++---- tests/utils/templatetags/test_diff_tags.py | 67 ++++++++++++++++++++++ 2 files changed, 81 insertions(+), 11 deletions(-) create mode 100644 tests/utils/templatetags/test_diff_tags.py diff --git a/castellum/studies/views/studies.py b/castellum/studies/views/studies.py index 905a227da..819c3af22 100644 --- a/castellum/studies/views/studies.py +++ b/castellum/studies/views/studies.py @@ -103,6 +103,19 @@ def send_pr_mail(study, user_email): ) +def take_snapshot(study): + study.snapshot = serializers.serialize('json', [ + study, + *study.studymembership_set.all(), + *study.domains.all(), + *study.studytag_set.all(), + *study.studysession_set.all(), + *study.subjectfiltergroup_set.all(), + *SubjectFilter.objects.filter(group__study=study), + ], fields=Exclusion(['snapshot'])) + study.save() + + class StudyIndexView(LoginRequiredMixin, ListView): model = Study ordering = 'name' @@ -322,17 +335,7 @@ class StudyStartRecruitmentView(StudyMixin, PermissionRequiredMixin, View): ) else: self.study.set_status(self.study.EXECUTION) - - self.study.snapshot = serializers.serialize('json', [ - self.study, - *self.study.studymembership_set.all(), - *self.study.domains.all(), - *self.study.studytag_set.all(), - *self.study.studysession_set.all(), - *self.study.subjectfiltergroup_set.all(), - *SubjectFilter.objects.filter(group__study=self.study), - ], fields=Exclusion(['snapshot'])) - self.study.save() + take_snapshot(self.study) monitoring_logger.info( 'Study {} started by {}'.format(self.study.name, self.request.user.pk) diff --git a/tests/utils/templatetags/test_diff_tags.py b/tests/utils/templatetags/test_diff_tags.py new file mode 100644 index 000000000..be897fc96 --- /dev/null +++ b/tests/utils/templatetags/test_diff_tags.py @@ -0,0 +1,67 @@ +from django.core import serializers + +from model_bakery import baker + +from castellum.studies.models import Study +from castellum.studies.views.studies import take_snapshot +from castellum.utils.templatetags import diff + + +def get_snapshot(study): + take_snapshot(study) + return list(serializers.deserialize('json', study.snapshot, ignorenonexistent=True)) + + +def test_diff_class(study): + snapshot = get_snapshot(study) + assert diff.diff_class(snapshot, study, 'name') == 'diff-unchanged' + study.name = 'test' + assert diff.diff_class(snapshot, study, 'name') == 'diff-changed' + study.name = '' + assert diff.diff_class(snapshot, study, 'name') == 'diff-removed' + + snapshot = get_snapshot(study) + study.name = 'test' + assert diff.diff_class(snapshot, study, 'name') == 'diff-added' + + +def test_diff_get_m2m(study): + excluded1 = baker.make(Study, name='excluded1') + excluded2 = baker.make(Study, name='excluded2') + + study.excluded_studies.add(excluded1) + snapshot = get_snapshot(study) + + study.excluded_studies.add(excluded2) + items = diff.diff_get_m2m(snapshot, study, 'excluded_studies') + items = [(value.name, cls) for value, cls in items] + assert items == [('excluded1', 'diff-unchanged'), ('excluded2', 'diff-added')] + + study.excluded_studies.clear() + items = diff.diff_get_m2m(snapshot, study, 'excluded_studies') + items = [(value.name, cls) for value, cls in items] + assert items == [('excluded1', 'diff-removed')] + + excluded1.delete() + items = diff.diff_get_m2m(snapshot, study, 'excluded_studies') + items = [(value.name, cls) for value, cls in items] + assert items == [] + + +def test_diff_get_o2m(study): + study.studytag_set.create(name='tag1') + study.studytag_set.create(name='tag2') + study.studytag_set.create(name='tag3') + snapshot = get_snapshot(study) + + study.studytag_set.filter(name='tag2').update(name='tag2-edited') + study.studytag_set.filter(name='tag3').delete() + study.studytag_set.create(name='tag4') + items = diff.diff_get_o2m(snapshot, study, 'studytag_set') + items = [(value.name, cls) for value, cls in items] + assert items == [ + ('tag1', 'diff-unchanged'), + ('tag2-edited', 'diff-changed'), + ('tag4', 'diff-added'), + ('tag3', 'diff-removed'), + ] -- GitLab From 670150532cecfa3193eb729c872c65e1a73af64b Mon Sep 17 00:00:00 2001 From: Tobias Bengfort Date: Wed, 28 Apr 2021 16:38:42 +0200 Subject: [PATCH 16/17] exclude additional fields in snapshot this way comparing snapshot strings gives an easy way to check for relevant changes --- castellum/studies/views/studies.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/castellum/studies/views/studies.py b/castellum/studies/views/studies.py index 819c3af22..5974aa533 100644 --- a/castellum/studies/views/studies.py +++ b/castellum/studies/views/studies.py @@ -112,7 +112,7 @@ def take_snapshot(study): *study.studysession_set.all(), *study.subjectfiltergroup_set.all(), *SubjectFilter.objects.filter(group__study=study), - ], fields=Exclusion(['snapshot'])) + ], fields=Exclusion(['snapshot', 'status', 'previous_status'])) study.save() -- GitLab From bfa4e85de3df2e98e03ab2dab0c77a58818aa095 Mon Sep 17 00:00:00 2001 From: Tobias Bengfort Date: Tue, 11 May 2021 14:32:50 +0200 Subject: [PATCH 17/17] add info text --- castellum/studies/templates/studies/study_diff.html | 2 ++ 1 file changed, 2 insertions(+) diff --git a/castellum/studies/templates/studies/study_diff.html b/castellum/studies/templates/studies/study_diff.html index fe53f7a25..e64884fc5 100644 --- a/castellum/studies/templates/studies/study_diff.html +++ b/castellum/studies/templates/studies/study_diff.html @@ -4,6 +4,8 @@ {% block title %}{% translate "Changes" %} · {{ block.super }}{% endblock %} {% block content %} +

    {% translate 'This page provides an overview of all the settings for this study. Any changes that were made since the study was started are highlighted.' %}

    +
    {{ study|verbose_name:'name' }}
    {% diff_get study 'name' as items %}{% diff_list items %}
    -- GitLab