From 81598ea624afd2881a18245cc3d786f3438c7f43 Mon Sep 17 00:00:00 2001 From: James Graham Date: Fri, 19 Mar 2021 15:36:09 +0000 Subject: [PATCH] refactor: allow admin config of static questions Text and visibility set in admin panel are now respected everywhere --- people/forms.py | 5 +- .../0051_refactor_hardcoded_questions.py | 81 +++++++++++++++++++ people/models/organisation.py | 3 + people/models/person.py | 2 + people/models/question.py | 59 +++++++++++--- people/models/relationship.py | 8 +- .../{person => }/includes/answer_set.html | 0 .../organisation-relationship/detail.html | 27 +------ .../templates/people/organisation/detail.html | 44 +--------- .../templates/people/person/detail_full.html | 2 +- .../people/person/detail_partial.html | 2 +- .../templates/people/relationship/detail.html | 27 +------ people/views/organisation.py | 37 +++------ people/views/person.py | 36 ++------- people/views/relationship.py | 20 ++++- 15 files changed, 184 insertions(+), 169 deletions(-) create mode 100644 people/migrations/0051_refactor_hardcoded_questions.py rename people/templates/people/{person => }/includes/answer_set.html (100%) diff --git a/people/forms.py b/people/forms.py index 0423431..f315a3f 100644 --- a/people/forms.py +++ b/people/forms.py @@ -52,8 +52,9 @@ class DynamicAnswerSetBase(forms.Form): continue # Placeholder question for sorting hardcoded questions - if question.is_hardcoded and (question.text in self.Meta.fields): - field_order.append(question.text) + if question.is_hardcoded and (question.hardcoded_field + in self.Meta.fields): + field_order.append(question.hardcoded_field) continue field_class = self.field_class diff --git a/people/migrations/0051_refactor_hardcoded_questions.py b/people/migrations/0051_refactor_hardcoded_questions.py new file mode 100644 index 0000000..7ccafb1 --- /dev/null +++ b/people/migrations/0051_refactor_hardcoded_questions.py @@ -0,0 +1,81 @@ +# Generated by Django 2.2.10 on 2021-03-19 14:37 + +from django.db import migrations, models +from django.db.models import F + + +def forward(apps, schema_editor): + """Move `text` field to `hardcoded_field`.""" + models = [ + 'OrganisationQuestion', + 'OrganisationRelationshipQuestion', + 'PersonQuestion', + 'RelationshipQuestion', + ] + models = map(lambda m: apps.get_model('people', m), models) + + for model in models: + model.objects.filter(is_hardcoded=True).update( + hardcoded_field=F('text')) + + +def backward(apps, schema_editor): + """Move `hardcoded_field` to `text` field.""" + models = [ + 'OrganisationQuestion', + 'OrganisationRelationshipQuestion', + 'PersonQuestion', + 'RelationshipQuestion', + ] + models = map(lambda m: apps.get_model('people', m), models) + + for model in models: + model.objects.exclude(hardcoded_field='').update( + text=F('hardcoded_field'), is_hardcoded=True) + + +class Migration(migrations.Migration): + + dependencies = [ + ('people', '0050_relationship_remove_timestamps'), + ] + + operations = [ + migrations.AddField( + model_name='organisationquestion', + name='hardcoded_field', + field=models.CharField(blank=True, help_text='Which hardcoded field does this question represent?', max_length=255), + ), + migrations.AddField( + model_name='organisationrelationshipquestion', + name='hardcoded_field', + field=models.CharField(blank=True, help_text='Which hardcoded field does this question represent?', max_length=255), + ), + migrations.AddField( + model_name='personquestion', + name='hardcoded_field', + field=models.CharField(blank=True, help_text='Which hardcoded field does this question represent?', max_length=255), + ), + migrations.AddField( + model_name='relationshipquestion', + name='hardcoded_field', + field=models.CharField(blank=True, help_text='Which hardcoded field does this question represent?', max_length=255), + ), + migrations.RunPython(forward, backward), + migrations.RemoveField( + model_name='organisationquestion', + name='is_hardcoded', + ), + migrations.RemoveField( + model_name='organisationrelationshipquestion', + name='is_hardcoded', + ), + migrations.RemoveField( + model_name='personquestion', + name='is_hardcoded', + ), + migrations.RemoveField( + model_name='relationshipquestion', + name='is_hardcoded', + ), + ] diff --git a/people/models/organisation.py b/people/models/organisation.py index d43fc1b..3f9c76c 100644 --- a/people/models/organisation.py +++ b/people/models/organisation.py @@ -61,6 +61,9 @@ class Organisation(models.Model): class OrganisationAnswerSet(AnswerSet): """The answers to the organisation questions at a particular point in time.""" + + question_model = OrganisationQuestion + #: Organisation to which this answer set belongs organisation = models.ForeignKey(Organisation, on_delete=models.CASCADE, diff --git a/people/models/person.py b/people/models/person.py index b1bf9c7..fab2333 100644 --- a/people/models/person.py +++ b/people/models/person.py @@ -132,6 +132,8 @@ class Person(models.Model): class PersonAnswerSet(AnswerSet): """The answers to the person questions at a particular point in time.""" + question_model = PersonQuestion + #: Person to which this answer set belongs person = models.ForeignKey(Person, on_delete=models.CASCADE, diff --git a/people/models/question.py b/people/models/question.py index 4a5fe1a..e7a50d6 100644 --- a/people/models/question.py +++ b/people/models/question.py @@ -1,4 +1,5 @@ """Base models for configurable questions and response sets.""" +import abc import typing from django.db import models @@ -53,12 +54,16 @@ class Question(models.Model): blank=False, null=False) - #: Is this question hardcoded in an AnswerSet? - is_hardcoded = models.BooleanField( - help_text='Only the order field has any effect for a hardcoded question.', - default=False, - blank=False, - null=False) + @property + def is_hardcoded(self) -> bool: + return bool(self.hardcoded_field) + + hardcoded_field = models.CharField( + help_text='Which hardcoded field does this question represent?', + max_length=255, + blank=True, + null=False + ) #: Should people be able to add their own answers? allow_free_text = models.BooleanField(default=False, @@ -126,6 +131,12 @@ class AnswerSet(models.Model): ] get_latest_by = 'timestamp' + @classmethod + @abc.abstractproperty + def question_model(cls) -> models.Model: + """Model representing questions to be answered in this AnswerSet.""" + raise NotImplementedError + #: Entity to which this answer set belongs #: This foreign key must be added to each concrete subclass # person = models.ForeignKey(Person, @@ -134,9 +145,14 @@ class AnswerSet(models.Model): # blank=False, # null=False) - #: Answers to :class:`Question`s - #: This many to many relation must be added to each concrete subclass - # question_answers = models.ManyToManyField(QuestionChoice) + @abc.abstractproperty + def question_answers(self) -> models.QuerySet: + """Answers to :class:`Question`s. + + This many to many relation must be added to each concrete subclass + question_answers = models.ManyToManyField(QuestionChoice) + """ + raise NotImplementedError #: When were these answers collected? timestamp = models.DateTimeField(auto_now_add=True, editable=False) @@ -150,6 +166,31 @@ class AnswerSet(models.Model): def is_current(self) -> bool: return self.replaced_timestamp is None + def build_question_answers(self, show_all: bool = False) -> typing.Dict[str, str]: + """Collect answers to dynamic questions and join with commas.""" + questions = self.question_model.objects.all() + if not show_all: + questions = questions.filter(answer_is_public=True) + + question_answers = {} + try: + for question in questions: + if question.hardcoded_field: + question_answers[question.text] = getattr( + self, question.hardcoded_field) + + else: + answers = self.question_answers.filter( + question=question) + question_answers[question.text] = ', '.join( + map(str, answers)) + + except AttributeError: + # No AnswerSet yet + pass + + return question_answers + def as_dict(self, answers: typing.Optional[typing.Dict[str, typing.Any]] = None): """Get the answers from this set as a dictionary for use in Form.initial.""" if answers is None: diff --git a/people/models/relationship.py b/people/models/relationship.py index ea68d0e..3fd703d 100644 --- a/people/models/relationship.py +++ b/people/models/relationship.py @@ -36,9 +36,7 @@ class RelationshipQuestionChoice(QuestionChoice): class Relationship(models.Model): - """ - A directional relationship between two people allowing linked questions. - """ + """A directional relationship between two people allowing linked questions.""" class Meta: constraints = [ models.UniqueConstraint(fields=['source', 'target'], @@ -95,6 +93,8 @@ class Relationship(models.Model): class RelationshipAnswerSet(AnswerSet): """The answers to the relationship questions at a particular point in time.""" + question_model = RelationshipQuestion + #: Relationship to which this answer set belongs relationship = models.ForeignKey(Relationship, on_delete=models.CASCADE, @@ -176,6 +176,8 @@ class OrganisationRelationship(models.Model): class OrganisationRelationshipAnswerSet(AnswerSet): """The answers to the organisation relationship questions at a particular point in time.""" + question_model = OrganisationRelationshipQuestion + #: OrganisationRelationship to which this answer set belongs relationship = models.ForeignKey(OrganisationRelationship, on_delete=models.CASCADE, diff --git a/people/templates/people/person/includes/answer_set.html b/people/templates/people/includes/answer_set.html similarity index 100% rename from people/templates/people/person/includes/answer_set.html rename to people/templates/people/includes/answer_set.html diff --git a/people/templates/people/organisation-relationship/detail.html b/people/templates/people/organisation-relationship/detail.html index 0d10870..7fb3374 100644 --- a/people/templates/people/organisation-relationship/detail.html +++ b/people/templates/people/organisation-relationship/detail.html @@ -64,32 +64,7 @@ {% else %} - - - - - - - - - - {% for answer in answer_set.question_answers.all %} - - - - - - {% empty %} - - - - {% endfor %} - -
QuestionAnswer
{{ answer.question }}{{ answer }}
No records
- -

- Last updated: {{ answer_set.timestamp }} -

+ {% include 'people/includes/answer_set.html' %} {% endif %} {% endwith %} diff --git a/people/templates/people/organisation/detail.html b/people/templates/people/organisation/detail.html index 1dd9065..599ec56 100644 --- a/people/templates/people/organisation/detail.html +++ b/people/templates/people/organisation/detail.html @@ -54,49 +54,7 @@
- - - - - - - - - - {% if answer_set.website %} - - {% endif %} - - {% if answer_set.countries %} - - {% endif %} - - {% if answer_set.hq_country %} - - {% endif %} - - {% for question, answers in question_answers.items %} - - - - - {% endfor %} - - {% if answer_set is None %} - - - - {% endif %} - -
QuestionAnswer
Website - {{ answer_set.website }} -
Countries - {% for country in answer_set.countries %} - {{ country.name }}{% if not forloop.last %},{% endif %} - {% endfor %} -
HQ Country{{ answer_set.hq_country.name }}
{{ question }}{{ answers }}
No answers
- -

Last updated: {{ answer_set.timestamp }}

+ {% include 'people/includes/answer_set.html' %}
diff --git a/people/templates/people/person/detail_full.html b/people/templates/people/person/detail_full.html index 494e610..f65f92a 100644 --- a/people/templates/people/person/detail_full.html +++ b/people/templates/people/person/detail_full.html @@ -57,7 +57,7 @@ {% endif %} - {% include 'people/person/includes/answer_set.html' %} + {% include 'people/includes/answer_set.html' %} Update diff --git a/people/templates/people/person/detail_partial.html b/people/templates/people/person/detail_partial.html index ff0b785..3d5e146 100644 --- a/people/templates/people/person/detail_partial.html +++ b/people/templates/people/person/detail_partial.html @@ -49,7 +49,7 @@
- {% include 'people/person/includes/answer_set.html' %} + {% include 'people/includes/answer_set.html' %}
diff --git a/people/templates/people/relationship/detail.html b/people/templates/people/relationship/detail.html index 70ff1a0..2d4c0ee 100644 --- a/people/templates/people/relationship/detail.html +++ b/people/templates/people/relationship/detail.html @@ -70,32 +70,7 @@ {% else %} - - - - - - - - - - {% for answer in answer_set.question_answers.all %} - - - - - - {% empty %} - - - - {% endfor %} - -
QuestionAnswer
{{ answer.question }}{{ answer }}
No records
- -

- Last updated: {{ answer_set.timestamp }} -

+ {% include 'people/includes/answer_set.html' %} {% endif %} {% endwith %} diff --git a/people/views/organisation.py b/people/views/organisation.py index 9a2df8e..b678b32 100644 --- a/people/views/organisation.py +++ b/people/views/organisation.py @@ -117,42 +117,25 @@ class OrganisationDetailView(LoginRequiredMixin, DetailView): context_object_name = 'organisation' template_name = 'people/organisation/detail.html' - def build_question_answers( - self, - answer_set: models.OrganisationAnswerSet) -> typing.Dict[str, str]: - """Collect answers to dynamic questions and join with commas.""" - show_all = self.request.user.is_superuser - questions = models.OrganisationQuestion.objects.filter( - is_hardcoded=False) - if not show_all: - questions = questions.filter(answer_is_public=True) - - question_answers = {} - try: - for question in questions: - answers = answer_set.question_answers.filter(question=question) - question_answers[str(question)] = ', '.join(map(str, answers)) - - except AttributeError: - # No AnswerSet yet - pass - - return question_answers - def get_context_data(self, **kwargs: typing.Any) -> typing.Dict[str, typing.Any]: """Add map marker to context.""" context = super().get_context_data(**kwargs) - answerset = self.object.current_answers - context['answer_set'] = answerset - context['question_answers'] = self.build_question_answers(answerset) + answer_set = self.object.current_answers + context['answer_set'] = answer_set context['map_markers'] = [{ 'name': self.object.name, - 'lat': getattr(answerset, 'latitude', None), - 'lng': getattr(answerset, 'longitude', None), + 'lat': getattr(answer_set, 'latitude', None), + 'lng': getattr(answer_set, 'longitude', None), }] + context['question_answers'] = {} + if answer_set is not None: + show_all = self.request.user.is_superuser + context['question_answers'] = answer_set.build_question_answers( + show_all) + context['relationship'] = None try: relationship = models.OrganisationRelationship.objects.get( diff --git a/people/views/person.py b/people/views/person.py index 8735f73..5271319 100644 --- a/people/views/person.py +++ b/people/views/person.py @@ -94,34 +94,6 @@ class ProfileView(LoginRequiredMixin, DetailView): # pk was not provided in URL return self.request.user.person - def build_question_answers( - self, answer_set: models.PersonAnswerSet) -> typing.Dict[str, str]: - """Collect answers to dynamic questions and join with commas.""" - show_all = ((self.object.user == self.request.user) - or self.request.user.is_superuser) - questions = models.PersonQuestion.objects.all() - if not show_all: - questions = questions.filter(answer_is_public=True) - - question_answers = {} - try: - for question in questions: - if question.is_hardcoded: - question_answers[str(question)] = getattr( - answer_set, question.text) - - else: - answers = answer_set.question_answers.filter( - question=question) - question_answers[str(question)] = ', '.join( - map(str, answers)) - - except AttributeError: - # No AnswerSet yet - pass - - return question_answers - def get_context_data(self, **kwargs: typing.Any) -> typing.Dict[str, typing.Any]: """Add current :class:`PersonAnswerSet` to context.""" @@ -129,9 +101,15 @@ class ProfileView(LoginRequiredMixin, DetailView): answer_set = self.object.current_answers context['answer_set'] = answer_set - context['question_answers'] = self.build_question_answers(answer_set) context['map_markers'] = [get_map_data(self.object)] + context['question_answers'] = {} + if answer_set is not None: + show_all = ((self.object.user == self.request.user) + or self.request.user.is_superuser) + context['question_answers'] = answer_set.build_question_answers( + show_all) + context['relationship'] = None try: relationship = models.Relationship.objects.get( diff --git a/people/views/relationship.py b/people/views/relationship.py index fb6a5ee..66589c6 100644 --- a/people/views/relationship.py +++ b/people/views/relationship.py @@ -19,6 +19,23 @@ class RelationshipDetailView(permissions.UserIsLinkedPersonMixin, DetailView): template_name = 'people/relationship/detail.html' related_person_field = 'source' + def get_context_data(self, + **kwargs: typing.Any) -> typing.Dict[str, typing.Any]: + """Add current :class:`RelationshipAnswerSet` to context.""" + context = super().get_context_data(**kwargs) + + answer_set = self.object.current_answers + context['answer_set'] = answer_set + + context['question_answers'] = {} + if answer_set is not None: + show_all = ((self.object.source == self.request.user) + or self.request.user.is_superuser) + context['question_answers'] = answer_set.build_question_answers( + show_all) + + return context + class RelationshipCreateView(LoginRequiredMixin, RedirectView): """View for creating a :class:`Relationship`. @@ -124,8 +141,7 @@ class OrganisationRelationshipEndView(RelationshipEndView): model = models.OrganisationRelationship -class OrganisationRelationshipDetailView(permissions.UserIsLinkedPersonMixin, - DetailView): +class OrganisationRelationshipDetailView(RelationshipDetailView): """View displaying details of an :class:`OrganisationRelationship`.""" model = models.OrganisationRelationship template_name = 'people/organisation-relationship/detail.html'