From 8d4d6e24cf8e81dd82e3d177e19344f3b385b773 Mon Sep 17 00:00:00 2001
From: Dylan Hillerbrand <dhillerbrand@gmail.com>
Date: Thu, 14 Nov 2024 15:03:17 -0500
Subject: [PATCH] fix(chant edit): fix feast/folio dropdowns; specify chant pk
 on POST request

Fix behaviour of feast and folio dropdown selectors on Chant Edit page to show chants on a specific folio or for a specific feast so that the selector remains on the selected option. Fix blank headers for filtered lists of chants on Chant Edit page.

Modify instructions on the Chant Edit page when no chant is selected (these has previously suggested that only full texts and volpiano was editable via that page; this has been corrected to note that all chant fields are editable).

Modify SourceEditChantsView `get_object` method so that the edited chant is always set by pk rather than recency of creation. Previously, this had been effectively true when changes were made through the form, as Django parsed the URL parameter for chant pk into the request.GET queryset; however, since URL parameters cannot currently be set with the Django test client, tests of this views were relying on the recency of chant feature. Now tests and the live form rely on the same logic (the presence of a `pk` option in the form data itself).

Reduce extraneous querysets in SourceEditChantsView.

Refs: #1707, #1708
---
 .../main_app/templates/chant_edit.html        |  39 +++--
 .../main_app/tests/test_views/test_chant.py   |   5 +-
 .../cantusdb_project/main_app/views/chant.py  | 156 ++++++------------
 3 files changed, 73 insertions(+), 127 deletions(-)

diff --git a/django/cantusdb_project/main_app/templates/chant_edit.html b/django/cantusdb_project/main_app/templates/chant_edit.html
index 373ae4d84..93b006111 100644
--- a/django/cantusdb_project/main_app/templates/chant_edit.html
+++ b/django/cantusdb_project/main_app/templates/chant_edit.html
@@ -43,14 +43,13 @@
         </div>
     {% endif %}
 
-    {% if pk_specified %}
-        {% if user.is_authenticated %}
+    {% if chant %}
             <p>
                 <a href="{% url 'chant-detail' chant.id %}">View</a> | Edit
             </p>
-        {% endif %}
         <form method="post" style="line-height: normal">{% csrf_token %}
             <input type="hidden" name="referrer" value="{{ request.META.HTTP_REFERER }}">
+            <input type="hidden" name="pk" value="{{ chant.id }}">
             
             <div class="form-row">
                 <div class="form-group m-1 col-lg-2">
@@ -352,28 +351,28 @@
             </div>
         </form> 
     {% else %}
-        <h3>Full text &amp; Volpiano edit form</h3>
+        <h3>Chant Edit Form</h3>
         <br>
-        <dl>
-            <dd><small>1) Select a <b>folio</b> or a <b>feast</b> (in the right block)</small></dd>
-            <dd><small>2) Click <b>"EDIT"</b> next to any chant</small></dd>
-            <dd><small>3) The <b>fulltext</b> and <b>Volpiano</b> fields should appear in this area</small></dd>
-            <dd><small>4) Edit the fields <b>according to the manuscript, following the fulltext guidelines created by Cantus</b></small></dd>
-            <dd><small>5) Click <b>"SAVE"</b></small></dd>
-        </dl>
-        <div style="margin-top:5px;">
+        <ol>
+            <li class="small">Select a <b>folio</b> or a <b>feast</b> (in the right block).</li>
+            <li class="small">Click <b>"EDIT"</b> next to any chant.</li>
+            <li class="small">A form will appear in this area that allows you to edit the chant data.</li>
+            <li class="small">Edit these fields according to the Cantus Database protocols.</li>
+            <li class="small">Click <b>"SAVE"</b>.</li>
+        </ol>
+        <div class="mt-1">
             <a href="{% url 'source-detail' source.id %}" title="{{ source.heading }}">
                 {{ source.short_heading }}
             </a>
         </div>
-        <div style="margin-top:5px;">
+        <div class="mt-1">
             <a href="{% url "chant-create" source.pk %}">
-                <small><b>&plus; Add new chant</b></small>
+                <small><b>+ Add new chant</b></small>
             </a>
         </div>
-        <div style="margin-top:5px;">
+        <div class="mt-1">
             <a href="{% url "source-create" %}">
-                <small><b>&plus; Add new source</b></small>
+                <small><b>+ Add new source</b></small>
             </a>
         </div>
     {% endif %}
@@ -385,13 +384,13 @@ <h3>Full text &amp; Volpiano edit form</h3>
             <h4><a href="{% url 'source-detail' source.id %}" title="{{ source.heading }}">{{ source.short_heading }}</a></h4>
         </div>
         <div class="card-body">
-            {% if source.chant_set.exists %}
+            {% if source_has_chants %}
                 <small>
                     <!--a small selector of all folios of this source-->
                     <select id="folioSelect" class="w-30">
                         <option value="">Select a folio:</option>
                         {% for folio in folios %}
-                            {% if folio == initial_GET_folio %}
+                            {% if folio == folio_query %}
                                 <option value="{{ folio }}" selected>{{ folio }}</option>
                             {% else %}
                                 <option value="{{ folio }}">{{ folio }}</option>
@@ -422,7 +421,7 @@ <h4><a href="{% url 'source-detail' source.id %}" title="{{ source.heading }}">{
             {% comment %} render if the user has selected a specific folio {% endcomment %}
             {% if feasts_current_folio %}
                 {% for feast, chants in feasts_current_folio %}
-                    <small>Folio: <b>{{ chant.folio }}</b> - Feast: <b title="{{ chant.feast.description }}">{{ feast.name }}</b></small>
+                    <small>Folio: <b>{{ folio_query }}</b> - Feast: <b title="{{ chant.feast.description }}">{{ feast.name }}</b></small>
                     <table class="table table-sm small table-bordered">     
                         {% for chant in chants %}
                             <tr>
@@ -463,7 +462,7 @@ <h4><a href="{% url 'source-detail' source.id %}" title="{{ source.heading }}">{
             {% comment %} render if the user has selected a specific feast {% endcomment %}
             {% elif folios_current_feast %}
                 {% for folio, chants in folios_current_feast %}
-                    <small>Folio: <b>{{ folio }}</b> - Feast: <b title="{{ chant.feast.description }}">{{ chant.feast }}</b></small>
+                    <small>Folio: <b>{{ folio }}</b> - Feast: <b title="{{ feast.description }}">{{ feast.name }}</b></small>
                     <table class="table table-sm small table-bordered">     
                         {% for chant in chants %}
                             <tr>
diff --git a/django/cantusdb_project/main_app/tests/test_views/test_chant.py b/django/cantusdb_project/main_app/tests/test_views/test_chant.py
index 4fa32e5d2..b51213778 100644
--- a/django/cantusdb_project/main_app/tests/test_views/test_chant.py
+++ b/django/cantusdb_project/main_app/tests/test_views/test_chant.py
@@ -236,6 +236,7 @@ def test_volpiano_signal(self):
             reverse("source-edit-chants", args=[source.id]),
             {
                 "manuscript_full_text_std_spelling": "ut queant lactose",
+                "pk": chant_1.id,
                 "folio": "001r",
                 "c_sequence": "1",
                 # liquescents, to be converted to lowercase
@@ -251,7 +252,7 @@ def test_volpiano_signal(self):
         self.assertEqual(chant_1.volpiano, "9abcdefg)A-B1C2D3E4F5G67?. yiz")
         self.assertEqual(chant_1.volpiano_notes, "9abcdefg9abcdefg")
 
-        make_fake_chant(
+        chant_2 = make_fake_chant(
             manuscript_full_text_std_spelling="resonare foobaz",
             source=source,
             folio="001r",
@@ -266,6 +267,7 @@ def test_volpiano_signal(self):
                 "folio": "001r",
                 "c_sequence": "2",
                 "volpiano": "abacadaeafagahaja",
+                "pk": chant_2.id,
             },
         )
         with patch("requests.get", mock_requests_get):
@@ -319,6 +321,7 @@ def test_proofread_chant(self):
                 "folio": folio,
                 "c_sequence": c_sequence,
                 "manuscript_full_text_std_spelling": ms_std,
+                "pk": chant.id,
             },
         )
         self.assertEqual(response.status_code, 302)  # 302 Found
diff --git a/django/cantusdb_project/main_app/views/chant.py b/django/cantusdb_project/main_app/views/chant.py
index 7b69a0df9..4d7fb3de8 100644
--- a/django/cantusdb_project/main_app/views/chant.py
+++ b/django/cantusdb_project/main_app/views/chant.py
@@ -9,7 +9,6 @@
 from django.contrib.postgres.aggregates import ArrayAgg
 from django.core.exceptions import PermissionDenied
 from django.db.models import Q, QuerySet
-from django.forms import BaseModelForm
 from django.http import Http404, HttpResponse
 from django.shortcuts import get_object_or_404
 from django.urls import reverse
@@ -1074,129 +1073,85 @@ class SourceEditChantsView(LoginRequiredMixin, UserPassesTestMixin, UpdateView):
     model = Chant
     form_class = ChantEditForm
     pk_url_kwarg = "source_id"
+    source: Source
+    source_has_chants: bool
 
-    def test_func(self):
+    def test_func(self) -> bool:
         user = self.request.user
         source_id = self.kwargs.get(self.pk_url_kwarg)
-        source = get_object_or_404(Source, id=source_id)
+        self.source = get_object_or_404(Source, id=source_id)
 
-        return user_can_edit_chants_in_source(user, source)
+        return user_can_edit_chants_in_source(user, self.source)
 
-    def get_queryset(self):
+    def get_queryset(self) -> QuerySet[Chant]:
         """
-        When a user visits the edit-chant page for a certain Source,
-        there are 2 dropdowns on the right side of the page: one for folio, and the other for feast.
-
-        When either a folio or a feast is selected, a list of Chants in the selected folio/feast will
-        be rendered.
-
         Returns:
-            a QuerySet of Chants in the Source, filtered by the optional search parameters.
-
-        Note: the first folio is selected by default.
+            a QuerySet of Chants in the Source, with associated feast,
+            genre, and service objects selected.
         """
-
-        # when arriving at this page, the url must have a source specified
-        source_id = self.kwargs.get(self.pk_url_kwarg)
-        source = Source.objects.get(id=source_id)
-
-        # optional search params
-        feast_id = self.request.GET.get("feast")
-        folio = self.request.GET.get("folio")
+        source = self.source
 
         # get all chants in the specified source
-        chants = source.chant_set.select_related(
-            "feast", "service", "genre", "source__holding_institution"
-        )
-        if not source.chant_set.exists():
-            # return empty queryset
-            return chants.all()
-        # filter the chants with optional search params
-        if feast_id:
-            chants = chants.filter(feast__id=feast_id)
-        elif folio:
-            chants = chants.filter(folio=folio)
-        # if none of the optional search params are specified, the first folio in the
-        # source is selected by default
-        else:
-            folios = chants.values_list("folio", flat=True).distinct().order_by("folio")
-            if not folios:
-                # if the source has no chants (conceivable), or if it has chants but
-                # none of them have folios specified (we don't really expect this to happen)
-                raise Http404
-            initial_folio = folios[0]
-            chants = chants.filter(folio=initial_folio)
+        chants = source.chant_set.select_related("feast", "service", "genre")
         self.queryset = chants
         return self.queryset
 
-    def get_object(self, **kwargs):
+    def get_object(self, queryset=None) -> Optional[Chant]:
         """
-        If the Source has no Chant, an Http404 is raised.
-        This is because there would be no Chant for the UpdateView to handle.
-
         Returns:
-            the Chant that we wish to edit (specified by the Chant's pk)
+            the Chant that we wish to edit (specified by the Chant's pk).
+            If no pk is specified or if no chants exist in the source,
+            None is returned.
         """
         queryset = self.get_queryset()
-        if queryset.count() == 0:
-            return None
 
-        pk = self.request.GET.get("pk")
-        # if a pk is not specified, this means that the user has not yet selected a Chant to edit
-        # thus, we will not render the update form
-        # instead, we will render the instructions page
+        if self.request.method == "GET":
+            pk = self.request.GET.get("pk")
+        elif self.request.method == "POST":
+            pk = self.request.POST.get("pk")
+        else:
+            pk = None
+
+        self.source_has_chants = queryset.exists()
         if not pk:
-            pk = queryset.latest("date_created").pk
-        queryset = queryset.filter(pk=pk)
-        return queryset.get()
+            return None
+        if not self.source_has_chants:
+            return None
+        return queryset.get(pk=pk)
 
     def get_context_data(self, **kwargs):
         context = super().get_context_data(**kwargs)
-        source_id = self.kwargs.get(self.pk_url_kwarg)
-        source = Source.objects.get(id=source_id)
-        context["source"] = source
-
-        chants_in_source = source.chant_set.select_related(
-            "feast", "genre", "service", "source__holding_institution"
-        )
+        context["source"] = self.source
 
-        # the following code block is sort of obsolete because if there is no Chant
-        # in the Source, a 404 will be raised
-        if not chants_in_source.exists():
-            # these are needed in the selectors and hyperlinks on the right side of the page
-            # if there's no chant in the source, there should be no options in those selectors
-            context["folios"] = None
-            context["feasts_with_folios"] = None
-            context["previous_folio"] = None
-            context["next_folio"] = None
+        chants_in_source = self.queryset
+        context["source_has_chants"] = self.source_has_chants
+        if not context["source_has_chants"]:
             return context
 
-        # generate options for the folio selector on the right side of the page
+        # generate options for the selectors on the right side of the page
         folios = (
             chants_in_source.values_list("folio", flat=True)
             .distinct()
             .order_by("folio")
         )
         context["folios"] = folios
-        # the options for the feast selector on the right, same as the source detail page
-        context["feasts_with_folios"] = get_feast_selector_options(source)
+        context["feast_selector_options"] = get_feast_selector_options(self.source)
 
-        if self.request.GET.get("feast"):
+        # generate the list of chants to display in the lower right sidebar card
+        # this card displays chants in the source filtered by the folio or feast selector
+        # if no feast or folio is selected, defaults to the chants on the first folio
+        if feast_param := self.request.GET.get("feast"):
             # if there is a "feast" query parameter, it means the user has chosen a specific feast
             # need to render a list of chants, grouped and ordered by folio and within each group,
-            # ordered by c_sequence
-            context["folios_current_feast"] = get_chants_with_folios(self.queryset)
+            # ordered by c_sequence. We get a Feast object in order to display some additional
+            # feast information in that list of chants.
+            context["feast"] = Feast.objects.get(id=feast_param)
+            context["folios_current_feast"] = get_chants_with_folios(
+                self.queryset.filter(feast_id=feast_param)
+            )
         else:
-            # the user has selected a folio, or,
-            # they have just navigated to the edit-chant page (where the first folio gets
-            # selected by default)
-            if self.request.GET.get("folio"):
-                # if browsing chants on a specific folio
-                folio = self.request.GET.get("folio")
-            else:
-                folio = folios[0]
-                # will be used in the template to pre-select the first folio in the drop-down
-                context["initial_GET_folio"] = folio
+            folio = self.request.GET.get("folio") or folios[0]
+            context["folio_query"] = folio
             try:
                 index = list(folios).index(folio)
             except ValueError:
@@ -1209,19 +1164,12 @@ def get_context_data(self, **kwargs):
             # if there is a "folio" query parameter, it means the user has chosen a specific folio
             # need to render a list of chants, ordered by c_sequence and grouped by feast
             context["feasts_current_folio"] = get_chants_with_feasts(
-                self.queryset.select_related(
-                    "feast", "genre", "service", "source__holding_institution"
-                ).order_by("c_sequence")
+                self.queryset.filter(folio=folio).order_by("c_sequence")
             )
 
-        # this boolean lets us decide whether to show the user the instructions or the editing form
-        # if the pk hasn't been specified, a user hasn't selected a specific chant they want to edit
-        # if so, we should display the instructions
-        pk = self.request.GET.get("pk")
-        pk_specified = bool(pk)
-        context["pk_specified"] = pk_specified
-
-        chant = self.get_object()
+        chant = self.object
+        if not chant:
+            return context
         if chant.volpiano:
             has_syl_text = bool(chant.manuscript_syllabized_full_text)
             # Note: the second value returned is a flag indicating whether the alignment process
@@ -1245,8 +1193,6 @@ def get_context_data(self, **kwargs):
         # in case the chant has no manuscript_full_text_std_spelling, we check Cantus Index
         # for the expected text for chants with the same Cantus ID, and pass it to the context
         # to suggest it to the user
-        if not chant:
-            return context
         cantus_id = chant.cantus_id
         if not cantus_id:
             return context
@@ -1261,7 +1207,6 @@ def form_valid(self, form):
 
         user: User = self.request.user
         chant: Chant = form.instance
-        proofreaders = []
 
         if not user_can_proofread_chant(user, chant):
             # Preserve the original values for proofreader-specific fields
@@ -1300,9 +1245,8 @@ def get_success_url(self):
         next_url = self.request.GET.get("ref")
         if next_url:
             return self.request.POST.get("referrer")
-        else:
-            # ref not found, stay on the same page after save
-            return self.request.get_full_path()
+        # ref not found, stay on the same page after save
+        return self.request.get_full_path()
 
 
 class ChantEditSyllabificationView(LoginRequiredMixin, UserPassesTestMixin, UpdateView):