From d64b8cac9a25bd42592c2fc0790dc58d410255dc Mon Sep 17 00:00:00 2001 From: Virginia Dooley Date: Sun, 26 May 2024 15:13:38 +0100 Subject: [PATCH 1/4] Add the front end option to split --- .../uk/templates/candidates/person-view.html | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/ynr/apps/elections/uk/templates/candidates/person-view.html b/ynr/apps/elections/uk/templates/candidates/person-view.html index aaff94341..1a854f0bc 100644 --- a/ynr/apps/elections/uk/templates/candidates/person-view.html +++ b/ynr/apps/elections/uk/templates/candidates/person-view.html @@ -256,6 +256,27 @@

Is this a duplicate person?

+ +
+

Split this person

+ {% if user_can_edit and person_edits_allowed %} +

Has this person been merged incorrectly?

+ {% if user.is_authenticated %} + Split person + {% if person.queued_image %} + {% if user_can_review_photos %} +

{{ person.name }} has a photo that needs to be reviewed

+ Review image + {% endif %} + {% endif %} + {% else %} + Log in to split + {% endif %} + {% else %} +

Edits disabled

+ {% include 'candidates/_edits_disallowed_message.html' %} + {% endif %} +

Use this data!

From a38a5cbf5da7f999104bf42ed1ab92ec25b8e806 Mon Sep 17 00:00:00 2001 From: Virginia Dooley Date: Sun, 26 May 2024 15:23:22 +0100 Subject: [PATCH 2/4] Present the user with options for splitting a Person --- ynr/apps/candidates/views/people.py | 97 ++++++++++++++++++- .../uk/templates/candidates/person-view.html | 6 +- ynr/apps/people/forms/forms.py | 25 ++++- ynr/apps/people/splitting.py | 65 +++++++++++++ .../people/templates/people/split_person.html | 51 ++++++++++ ynr/apps/people/urls.py | 5 + 6 files changed, 241 insertions(+), 8 deletions(-) create mode 100644 ynr/apps/people/splitting.py create mode 100644 ynr/apps/people/templates/people/split_person.html diff --git a/ynr/apps/candidates/views/people.py b/ynr/apps/candidates/views/people.py index 02f117569..d5c9a800f 100644 --- a/ynr/apps/candidates/views/people.py +++ b/ynr/apps/candidates/views/people.py @@ -24,7 +24,12 @@ from elections.mixins import ElectionMixin from elections.models import Election from elections.uk.forms import SelectBallotForm -from people.forms.forms import NewPersonForm, UpdatePersonForm +from people.forms.forms import ( + NewPersonForm, + PersonSplitForm, + PersonSplitFormSet, + UpdatePersonForm, +) from people.forms.formsets import ( PersonIdentifierFormsetFactory, PersonMembershipFormsetFactory, @@ -490,6 +495,96 @@ def form_valid(self, all_forms): ) +class PersonSplitView(FormView): + template_name = "people/split_person.html" + form_class = PersonSplitForm + + def get_review_url(self): + person_id = self.kwargs.get("person_id") + return reverse_lazy( + "review_split_person", kwargs={"person_id": person_id} + ) + + def get_context_data(self, **kwargs): + context = super().get_context_data(**kwargs) + context["person"] = self.get_person() + context["person_id"] = self.kwargs["person_id"] + context["formset"] = self.get_form() + return context + + def get_initial_data(self, form_class=None): + person = self.get_person() + return [ + {"attribute_name": "name", "attribute_value": person.name}, + # {"attribute_name": "image", "attribute_value": person.image.image if person.image else None}, + {"attribute_name": "gender", "attribute_value": person.gender}, + # { + # "attribute_name": "birth_date", + # "attribute_value": person.birth_date if person.birth_date else None, + # }, + # { + # "attribute_name": "death_date", + # "attribute_value": person.death_date if person.death_date else None, + # }, + # {"attribute_name": "summary", "attribute_value": person.summary if person.summary else None }, + # { + # "attribute_name": "biography", + # "attribute_value": person.biography if person.biography else None, + # }, + # { + # "attribute_name": "other_names", + # "attribute_value": person.other_names.all if person.other_names.all else None, + # }, + # { + # "attribute_name": "memberships", + # "attribute_value": person.memberships.all if person.memberships.all else None, + # }, + ] + + def get_form(self, form_class=None): + initial_data = self.get_initial_data() + print("Initial Data:", initial_data) + return PersonSplitFormSet( + initial=self.get_initial_data(form_class), + ) + + def get_person(self): + person_id = self.kwargs.get("person_id") + return get_object_or_404(Person, pk=person_id) + + def form_valid(self, formset): + choices = { + "keep": [], + "add": [], + "both": [], + } + for form in formset: + attribute_name = form.cleaned_data["attribute_name"] + attribute_value = form.cleaned_data["attribute_value"] + choice = form.cleaned_data["choice"] + person_id = self.kwargs.get("person_id") + if choice == "keep": + choices["keep"].append({attribute_name: attribute_value}) + elif choice == "move": + choices["move"].append({attribute_name: attribute_value}) + elif choice == "both": + choices["both"].append({attribute_name: attribute_value}) + if choices: + self.request.session["choices"] = choices + self.request.session["person_id"] = person_id + return redirect(self.get_review_url()) + return self.form_invalid(formset) + + def post(self, request, *args, **kwargs): + formset = PersonSplitFormSet(request.POST) + print("POST Data:", request.POST) # Print POST data to inspect + if formset.is_valid(): + return self.form_valid(formset) + + print("Formset Errors:", formset.errors) + return self.form_invalid(formset) + + class NewPersonSelectElectionView(LoginRequiredMixin, FormView): """ For when we know new person's name, but not the election they are standing diff --git a/ynr/apps/elections/uk/templates/candidates/person-view.html b/ynr/apps/elections/uk/templates/candidates/person-view.html index 1a854f0bc..8069ba42c 100644 --- a/ynr/apps/elections/uk/templates/candidates/person-view.html +++ b/ynr/apps/elections/uk/templates/candidates/person-view.html @@ -19,7 +19,7 @@ - + {% comment %} {% endcomment %} @@ -30,7 +30,7 @@ {% if settings.TWITTER_USERNAME %} {% endif %} - + {% comment %} {% endcomment %} {% endblock %} @@ -48,7 +48,7 @@
- + {% comment %} {% endcomment %} {% if not person.person_image and user.is_authenticated %} Upload photo diff --git a/ynr/apps/people/forms/forms.py b/ynr/apps/people/forms/forms.py index 20a63e6e8..9f7851534 100644 --- a/ynr/apps/people/forms/forms.py +++ b/ynr/apps/people/forms/forms.py @@ -4,6 +4,7 @@ from django.conf import settings from django.core.exceptions import ValidationError from django.core.validators import URLValidator, validate_email +from django.forms import formset_factory from django.utils import timezone from django.utils.functional import cached_property from facebook_data.tasks import extract_fb_page_id @@ -14,10 +15,7 @@ PreviousPartyAffiliationsField, ) from parties.models import Party -from people.forms.fields import ( - CurrentUnlockedBallotsField, - StrippedCharField, -) +from people.forms.fields import CurrentUnlockedBallotsField, StrippedCharField from people.helpers import ( clean_mastodon_username, clean_twitter_username, @@ -407,6 +405,25 @@ class UpdatePersonForm(BasePersonForm): pass +class PersonSplitForm(forms.Form): + attribute_name = forms.CharField(widget=forms.HiddenInput()) + attribute_value = forms.CharField( + widget=forms.TextInput(attrs={"readonly": "readonly"}) + ) + + choice = forms.ChoiceField( + choices=[ + ("keep", "Keep"), + ("add", "Add"), + ("both", "Both"), + ], + widget=forms.RadioSelect, + ) + + +PersonSplitFormSet = formset_factory(PersonSplitForm, extra=0) + + class OtherNameForm(forms.ModelForm): class Meta: model = OtherName diff --git a/ynr/apps/people/splitting.py b/ynr/apps/people/splitting.py new file mode 100644 index 000000000..c4f40eac6 --- /dev/null +++ b/ynr/apps/people/splitting.py @@ -0,0 +1,65 @@ +from collections import defaultdict + +from candidates.models.versions import ( + is_a_merge, + version_timestamp_key, +) + + +class PersonSplitter: + """This class is responsible for splitting incorrectly merged candidates. + Inverse of ynr/apps/people/merging.py + """ + + def __init__(self, person): + self.person = person + + def merged_version(self, person): + """Search through the person versions for the person merge history. + and return the last merge version. + """ + versions_data = self.person.versions + version_id_to_parent_ids = {} + if not self.person.versions: + return version_id_to_parent_ids + ordered_versions = sorted(versions_data, key=version_timestamp_key) + person_id_to_ordered_versions = defaultdict(list) + # Divide all the version with the same ID into separate ordered + # lists, and record the parent of each version that we get from + # doing that: + merged_versions = [] + for version in ordered_versions: + version_id = version["version_id"] + person_id = version["data"]["id"] + versions_for_person_id = person_id_to_ordered_versions[person_id] + if versions_for_person_id: + last_version_id = versions_for_person_id[-1]["version_id"] + version_id_to_parent_ids[version_id] = [last_version_id] + else: + version_id_to_parent_ids[version_id] = [] + versions_for_person_id.append(version) + # Now go through looking for versions that represent merges. Note + # that it's *possible* for someone to create a new version that + # doesn't represent a merge but which has a information_source + # message that makes it look like one. We try to raise an + # exception if this might have happened, by checking that (a) the + # person ID in the message also has history in this versions array + # and (b) the number of unique person IDs in the versions is one + # more than the number of versions that look like merges. We raise + # an exception in either of these situations. + + number_of_person_ids = len(person_id_to_ordered_versions.keys()) + number_of_merges = 0 + for version in ordered_versions: + version_id = version["version_id"] + merged_from = is_a_merge(version) + if merged_from: + number_of_merges += 1 + # TO DO: Do we want the most recent merged version or should we present the user with options? + merged_versions.append(version) + if number_of_person_ids != number_of_merges + 1: + raise ValueError( + "The number of unique person IDs in the versions is not one more than the number of versions that look like merges." + ) + + return merged_versions[0]["data"] diff --git a/ynr/apps/people/templates/people/split_person.html b/ynr/apps/people/templates/people/split_person.html new file mode 100644 index 000000000..1e72fd63d --- /dev/null +++ b/ynr/apps/people/templates/people/split_person.html @@ -0,0 +1,51 @@ +{% extends 'base.html' %} +{% load thumbnail %} +{% load static %} +{% load pipeline %} + +{% block content %} +

Split {{ person.name }} ({{person_id}}) into two people

+

Choose which properties to keep on {{ person.name }}, which to move to a new person, and which to keep on both.

+ +
+ {% csrf_token %} + {{ formset.management_form }} + + + + + + + + + + + + + {% for form in formset %} + {% for error in form.errors %} +
{{ error }}
+ {% endfor %} + + + + {% if form.attribute_name.value == "image" and form.attribute_value.value %} + {% thumbnail form.attribute_value.value "100x100" as im %} + + {% endthumbnail %} + {% elif form.attribute_name.value == "image" and not form.attribute_value.value %} + + {% else %} + + {% endif %} + + + + + {% endfor %} + +
FieldValueKeep on originalAdd to newUse for Both
{{ form.attribute_name.value }}No image available{{ form.attribute_value.value }}{{ form.choice.0 }}{{ form.choice.1 }}{{ form.choice.2 }}
+ + Cancel +
+{% endblock %} diff --git a/ynr/apps/people/urls.py b/ynr/apps/people/urls.py index ef4e0b23b..de6321b1a 100644 --- a/ynr/apps/people/urls.py +++ b/ynr/apps/people/urls.py @@ -47,6 +47,11 @@ views.UpdatePersonView.as_view(), name="person-update", ), + re_path( + r"^person/(?P\d+)/split/?$", + views.PersonSplitView.as_view(), + name="person-split", + ), re_path( r"^person/create/select_election$", views.NewPersonSelectElectionView.as_view(), From bcf5801087a612f8b77979e53db6fd3c04fb903a Mon Sep 17 00:00:00 2001 From: Virginia Dooley Date: Wed, 29 May 2024 20:37:58 +0100 Subject: [PATCH 3/4] Review split person choices --- ynr/apps/candidates/views/people.py | 26 +++++++---- ynr/apps/people/forms/forms.py | 19 +++----- .../templates/people/review_split_person.html | 43 +++++++++++++++++++ .../people/templates/people/split_person.html | 21 +++++---- ynr/apps/people/urls.py | 10 +++++ 5 files changed, 89 insertions(+), 30 deletions(-) create mode 100644 ynr/apps/people/templates/people/review_split_person.html diff --git a/ynr/apps/candidates/views/people.py b/ynr/apps/candidates/views/people.py index d5c9a800f..c62f3a84f 100644 --- a/ynr/apps/candidates/views/people.py +++ b/ynr/apps/candidates/views/people.py @@ -13,9 +13,9 @@ HttpResponsePermanentRedirect, HttpResponseRedirect, ) -from django.shortcuts import get_object_or_404 +from django.shortcuts import get_object_or_404, redirect from django.template.loader import render_to_string -from django.urls import reverse +from django.urls import reverse, reverse_lazy from django.utils.decorators import method_decorator from django.views.decorators.cache import cache_control from django.views.generic import FormView, TemplateView, UpdateView, View @@ -514,6 +514,7 @@ def get_context_data(self, **kwargs): def get_initial_data(self, form_class=None): person = self.get_person() + #: TO DO: Figure out how to return attribute names and values only for those that are not empty return [ {"attribute_name": "name", "attribute_value": person.name}, # {"attribute_name": "image", "attribute_value": person.image.image if person.image else None}, @@ -542,10 +543,8 @@ def get_initial_data(self, form_class=None): ] def get_form(self, form_class=None): - initial_data = self.get_initial_data() - print("Initial Data:", initial_data) return PersonSplitFormSet( - initial=self.get_initial_data(form_class), + initial=self.get_initial_data(), ) def get_person(self): @@ -555,7 +554,7 @@ def get_person(self): def form_valid(self, formset): choices = { "keep": [], - "add": [], + "move": [], "both": [], } for form in formset: @@ -577,14 +576,23 @@ def form_valid(self, formset): def post(self, request, *args, **kwargs): formset = PersonSplitFormSet(request.POST) - print("POST Data:", request.POST) # Print POST data to inspect if formset.is_valid(): return self.form_valid(formset) - - print("Formset Errors:", formset.errors) return self.form_invalid(formset) +class ReviewPersonSplitView(TemplateView): + template_name = "people/review_split_person.html" + + def get_context_data(self, **kwargs): + context = super().get_context_data(**kwargs) + context["choices"] = self.request.session.get("choices", {}) + context["person"] = get_object_or_404( + Person, pk=self.request.session.get("person_id") + ) + return context + + class NewPersonSelectElectionView(LoginRequiredMixin, FormView): """ For when we know new person's name, but not the election they are standing diff --git a/ynr/apps/people/forms/forms.py b/ynr/apps/people/forms/forms.py index 9f7851534..912300bc8 100644 --- a/ynr/apps/people/forms/forms.py +++ b/ynr/apps/people/forms/forms.py @@ -406,19 +406,14 @@ class UpdatePersonForm(BasePersonForm): class PersonSplitForm(forms.Form): + CHOICES = [ + ("keep", "Keep for original person"), + ("move", "Move to a new person"), + ("both", "Do both"), + ] attribute_name = forms.CharField(widget=forms.HiddenInput()) - attribute_value = forms.CharField( - widget=forms.TextInput(attrs={"readonly": "readonly"}) - ) - - choice = forms.ChoiceField( - choices=[ - ("keep", "Keep"), - ("add", "Add"), - ("both", "Both"), - ], - widget=forms.RadioSelect, - ) + attribute_value = forms.CharField(widget=forms.HiddenInput()) + choice = forms.ChoiceField(choices=CHOICES, widget=forms.RadioSelect) PersonSplitFormSet = formset_factory(PersonSplitForm, extra=0) diff --git a/ynr/apps/people/templates/people/review_split_person.html b/ynr/apps/people/templates/people/review_split_person.html new file mode 100644 index 000000000..05e579b38 --- /dev/null +++ b/ynr/apps/people/templates/people/review_split_person.html @@ -0,0 +1,43 @@ +{% extends 'base.html' %} +{% load thumbnail %} +{% load static %} +{% load pipeline %} + +{% block content %} +

Review split person choices

+ + {% if choices.keep %} +

Keep these attributes on Person:# {{person_id}}

+
    + {% for item in choices.keep %} +
  • {{ item }}
    • + {% endfor %} +
+

Submitting this form will create a new person with the above attributes.

+ {% endif %} + + {% if choices.move %} +

Move these attributes to a new Person

+
    + {% for item in choices.move %} +
  • {{ item }}
    • + {% endfor %} +
+

Submitting this form will create a new person with the above attributes and remove the above attributes from the original person.

+ {% endif %} + + {% if choices.both %} +

Do both

+
    + {% for item in choices.both %} +
  • {{ item }}
    • + {% endfor %} +
+

Submitting this form will create a new person with the above attributes and keep the above attributes on the original person.

+ {% endif %} +
+ {% csrf_token %} + +
+ Go back +{% endblock %} diff --git a/ynr/apps/people/templates/people/split_person.html b/ynr/apps/people/templates/people/split_person.html index 1e72fd63d..26e3c3095 100644 --- a/ynr/apps/people/templates/people/split_person.html +++ b/ynr/apps/people/templates/people/split_person.html @@ -29,15 +29,18 @@

Split {{ person.name }} ({{perso {{ form.attribute_name.value }} - {% if form.attribute_name.value == "image" and form.attribute_value.value %} - {% thumbnail form.attribute_value.value "100x100" as im %} - - {% endthumbnail %} - {% elif form.attribute_name.value == "image" and not form.attribute_value.value %} - No image available - {% else %} - {{ form.attribute_value.value }} - {% endif %} + + {% if form.attribute_name.value == "image" %} + {% thumbnail form.attribute_value.value "100x100" as im %} + + {% endthumbnail %} + {% else %} + {{ form.attribute_value.value }} + {% endif %} + + {{ form.attribute_name }} + {{ form.attribute_value }} + {{ form.choice.0 }} {{ form.choice.1 }} {{ form.choice.2 }} diff --git a/ynr/apps/people/urls.py b/ynr/apps/people/urls.py index de6321b1a..18026e95a 100644 --- a/ynr/apps/people/urls.py +++ b/ynr/apps/people/urls.py @@ -52,6 +52,16 @@ views.PersonSplitView.as_view(), name="person-split", ), + re_path( + r"^person/(?P\d+)/review/?$", + views.ReviewPersonSplitView.as_view(), + name="review_split_person", + ), + re_path( + r"^person/(?P\d+)/confirm_split/?$", + views.ConfirmPersonSplitView.as_view(), + name="confirm_split_person", + ), re_path( r"^person/create/select_election$", views.NewPersonSelectElectionView.as_view(), From f2b93e649b49e275e2450ef7f851f7866f5b05f7 Mon Sep 17 00:00:00 2001 From: Virginia Dooley Date: Thu, 30 May 2024 11:05:23 +0100 Subject: [PATCH 4/4] Confirm split person changes --- ynr/apps/candidates/views/people.py | 63 +++++++++++++++++++ .../people/confirm_split_person.html | 12 ++++ ynr/apps/people/urls.py | 2 +- 3 files changed, 76 insertions(+), 1 deletion(-) create mode 100644 ynr/apps/people/templates/people/confirm_split_person.html diff --git a/ynr/apps/candidates/views/people.py b/ynr/apps/candidates/views/people.py index c62f3a84f..a2ccb0df4 100644 --- a/ynr/apps/candidates/views/people.py +++ b/ynr/apps/candidates/views/people.py @@ -593,6 +593,69 @@ def get_context_data(self, **kwargs): return context +class ConfirmPersonSplitView(TemplateView): + template_name = "people/confirm_split_person.html" + + def get_success_url(self, person_id, new_person_id=None): + person_id = self.kwargs.get("person_id") + if new_person_id: + return reverse( + "confirm_split_person", + kwargs={"person_id": person_id, "new_person_id": new_person_id}, + ) + return reverse("confirm_split_person", kwargs={"person_id": person_id}) + + def get_context_data(self, **kwargs): + context = super().get_context_data(**kwargs) + person_id = kwargs.get("person_id") + context["person"] = get_object_or_404(Person, pk=person_id) + return context + + def post(self, request, *args, **kwargs): + person_id = self.request.session.get("person_id") + person = get_object_or_404(Person, pk=person_id) + choices = request.session.get("choices", {}) + new_person_data = self.split(person, choices) + new_person_id = new_person_data.id if new_person_data else None + return redirect( + self.get_success_url( + person_id=person_id, new_person_id=new_person_id + ) + ) + + def split(self, person, choices): + person_id = person.id + new_person = None + if not choices: + # TODO: ADD A MESSAGE TO SAY THAT NO CHOICES WERE MADE? + # TODO: Do we even need to do this? + return redirect("person-view", person_id=person_id) + if choices["both"] or choices["move"]: + new_person = Person.objects.create() + for choice in choices["keep"]: + for key, value in choice.items(): + setattr(person, key, value) + person.save() + for choice in choices["move"]: + # create a new person with the chosen attribute values + new_person = Person.objects.create() + for key, value in choice.items(): + setattr(new_person, key, value) + # remove this attribute from the original person + # TODO: however if this is a required person field, such as name, how can we handle it? + # perhaps we need to add a text input to the form in the previous step + # to allow the user to enter a new value + for key, value in choice.items(): + setattr(person, key, None) + for choice in choices["both"]: + # create a new person with the chosen attribute values + for key, value in choice.items(): + setattr(new_person, key, value) + person.save() + new_person.save() + return new_person + + class NewPersonSelectElectionView(LoginRequiredMixin, FormView): """ For when we know new person's name, but not the election they are standing diff --git a/ynr/apps/people/templates/people/confirm_split_person.html b/ynr/apps/people/templates/people/confirm_split_person.html new file mode 100644 index 000000000..9ebed9e59 --- /dev/null +++ b/ynr/apps/people/templates/people/confirm_split_person.html @@ -0,0 +1,12 @@ +{% extends 'base.html' %} +{% load thumbnail %} +{% load static %} +{% load pipeline %} + +{% block content %} +

Confirm Split

+

Original Person: {{ person_id }}

+ +

New Person: {{ new_person_id}}

+ +{% endblock %} diff --git a/ynr/apps/people/urls.py b/ynr/apps/people/urls.py index 18026e95a..16d698c1c 100644 --- a/ynr/apps/people/urls.py +++ b/ynr/apps/people/urls.py @@ -58,7 +58,7 @@ name="review_split_person", ), re_path( - r"^person/(?P\d+)/confirm_split/?$", + r"^person/(?P\d+)/confirm_split_person/(?P\d+)?/?$", views.ConfirmPersonSplitView.as_view(), name="confirm_split_person", ),