From 3726063655cc2e3a91dbe0e2d01df97582adbf37 Mon Sep 17 00:00:00 2001 From: Dylan Hillerbrand Date: Thu, 14 Nov 2024 14:54:57 -0500 Subject: [PATCH 1/3] fix(feast selector options): show folio ranges in feast selector options Modify options in the feast dropdown to include the feast and the range of folios on which the feast appears, rather than a single option for each feast-folio pair. Remove duplicate tests of feast selector options on various views. Add single test for feast dropdown options. Refs: 1707 --- .../main_app/templates/browse_chants.html | 4 +- .../main_app/templates/chant_edit.html | 4 +- .../main_app/templates/source_detail.html | 4 +- .../main_app/tests/test_views/test_chant.py | 93 +++++++++++++ .../main_app/tests/test_views/test_source.py | 43 ------ .../cantusdb_project/main_app/views/chant.py | 125 ++++++++++++++++-- 6 files changed, 210 insertions(+), 63 deletions(-) diff --git a/django/cantusdb_project/main_app/templates/browse_chants.html b/django/cantusdb_project/main_app/templates/browse_chants.html index 771aceac0..c06f3125a 100644 --- a/django/cantusdb_project/main_app/templates/browse_chants.html +++ b/django/cantusdb_project/main_app/templates/browse_chants.html @@ -166,8 +166,8 @@

{
diff --git a/django/cantusdb_project/main_app/templates/chant_edit.html b/django/cantusdb_project/main_app/templates/chant_edit.html index 987e398e6..373ae4d84 100644 --- a/django/cantusdb_project/main_app/templates/chant_edit.html +++ b/django/cantusdb_project/main_app/templates/chant_edit.html @@ -410,8 +410,8 @@

{
diff --git a/django/cantusdb_project/main_app/templates/source_detail.html b/django/cantusdb_project/main_app/templates/source_detail.html index 981bc290d..c0e939c3b 100644 --- a/django/cantusdb_project/main_app/templates/source_detail.html +++ b/django/cantusdb_project/main_app/templates/source_detail.html @@ -212,8 +212,8 @@

{{ source.short_heading }}

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 09b995cb3..4fa32e5d2 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 @@ -3134,3 +3134,96 @@ def test_non_existing_chant(self): chant = make_fake_chant() response = self.client.post(reverse("chant-delete", args=[chant.id + 100])) self.assertEqual(response.status_code, 404) + + +class ChantViewHelpersTest(TestCase): + """ + Tests for the helper functions defined in views.chant + """ + + @classmethod + def setUpTestData(cls): + cls.feasts = [make_fake_feast() for _ in range(4)] + + def test_get_feast_selector_options(self) -> None: + with self.subTest("r/v foliation"): + source = make_fake_source() + feasts = self.feasts + # Create chants for feasts[0] for range 001v, A001v + for folio in ["A001v", "001v"]: + make_fake_chant(source=source, folio=folio, feast=feasts[0]) + # Create chants for feasts[1] for range 001r, 002r + for folio in ["001r", "002r"]: + make_fake_chant(source=source, folio=folio, feast=feasts[1]) + # Create chants for feasts[2] for range 002r-002v, 003v + for folio in ["002r", "002v", "003v"]: + make_fake_chant(source=source, folio=folio, feast=feasts[2]) + # Create a chant on 003r with no feast that should show up in no ranges + make_fake_chant(source=source, folio="003r", feast=None) + feast_selector_options = get_feast_selector_options(source) + expected_result = [ + (feasts[1].id, feasts[1].name, "001r, 002r"), + (feasts[0].id, feasts[0].name, "001v, A001v"), + (feasts[2].id, feasts[2].name, "002r-002v, 003v"), + ] + self.assertEqual(feast_selector_options, expected_result) + with self.subTest("Foliation with numbers only"): + source = make_fake_source() + feasts = self.feasts + # Create chants for feasts[0] for range 002-004 + for folio in ["002", "003", "004"]: + make_fake_chant(source=source, folio=folio, feast=feasts[0]) + # Create chants for feasts[1] for range 001, 003 + for folio in ["001", "003"]: + make_fake_chant(source=source, folio=folio, feast=feasts[1]) + feast_selector_options = get_feast_selector_options(source) + expected_result = [ + (feasts[1].id, feasts[1].name, "001, 003"), + (feasts[0].id, feasts[0].name, "002-004"), + ] + self.assertEqual(feast_selector_options, expected_result) + with self.subTest("Unnumbered folios"): + source = make_fake_source() + feasts = self.feasts + # Create chants for feasts[0] for folios 003v-003w, 004v + for folio in ["003v", "003w", "004v"]: + make_fake_chant(source=source, folio=folio, feast=feasts[0]) + # Create chants for feasts[1] for folios 002r-002x + for folio in ["002r", "002x"]: + make_fake_chant(source=source, folio=folio, feast=feasts[1]) + # Create chants for feasts[2] for folios 003w-004r + for folio in ["003w", "004r"]: + make_fake_chant(source=source, folio=folio, feast=feasts[2]) + # Create chants for feasts[3] for folios 003w-003x + for folio in ["003x", "003w"]: + make_fake_chant(source=source, folio=folio, feast=feasts[3]) + feast_selector_options = get_feast_selector_options(source) + expected_result = [ + (feasts[1].id, feasts[1].name, "002r-002x"), + (feasts[0].id, feasts[0].name, "003v-003w, 004v"), + (feasts[3].id, feasts[3].name, "003w-003x"), + (feasts[2].id, feasts[2].name, "003w-004r"), + ] + self.assertEqual(feast_selector_options, expected_result) + with self.subTest("Unexpected folio numbers"): + # This subTest ensures that unexpected folio numbers (say, + # a something like "00q1r") are added correctly to ranges. + source = make_fake_source() + feasts = self.feasts + # Create chants for feasts[0] with a normal range (001r-002r) + for folio in ["001r", "001v", "002r"]: + make_fake_chant(source=source, folio=folio, feast=feasts[0]) + # Create chants for feasts[1] with an unexpected folio number (00q1r) + # and expected folio numbers + for folio in ["00q1r", "002v", "003r"]: + make_fake_chant(source=source, folio=folio, feast=feasts[1]) + # Create chants for feasts[2] with only unexpected folio numbers + for folio in ["00q2r", "00q3", "X00q3"]: + make_fake_chant(source=source, folio=folio, feast=feasts[2]) + feast_selector_options = get_feast_selector_options(source) + expected_result = [ + (feasts[0].id, feasts[0].name, "001r-002r"), + (feasts[1].id, feasts[1].name, "002v-003r, 00q1r"), + (feasts[2].id, feasts[2].name, "00q2r, 00q3, X00q3"), + ] + self.assertEqual(feast_selector_options, expected_result) diff --git a/django/cantusdb_project/main_app/tests/test_views/test_source.py b/django/cantusdb_project/main_app/tests/test_views/test_source.py index 11e7565cf..8220dff6e 100644 --- a/django/cantusdb_project/main_app/tests/test_views/test_source.py +++ b/django/cantusdb_project/main_app/tests/test_views/test_source.py @@ -164,28 +164,6 @@ def test_context_sequence_folios(self): # the folios should be ordered by the "folio" field self.assertEqual(folios.query.order_by, ("folio",)) - def test_context_feasts_with_folios(self): - # create a source and several chants (associated with feasts) in it - source = make_fake_source() - feast_1 = make_fake_feast() - feast_2 = make_fake_feast() - make_fake_chant(source=source, folio="001r", feast=feast_1) - make_fake_chant(source=source, folio="001r", feast=feast_1) - make_fake_chant(source=source, folio="001v", feast=feast_2) - make_fake_chant(source=source, folio="001v", feast=None) - make_fake_chant(source=source, folio="001v", feast=feast_2) - make_fake_chant(source=source, folio="002r", feast=feast_1) - # request the page - response = self.client.get(reverse("source-detail", args=[source.id])) - # context "feasts_with_folios" is a list of tuples - # it records the folios where the feast changes - expected_result = [ - ("001r", feast_1.id, feast_1.name), - ("001v", feast_2.id, feast_2.name), - ("002r", feast_1.id, feast_1.name), - ] - self.assertEqual(response.context["feasts_with_folios"], expected_result) - def test_context_sequences(self): # create a sequence source and several sequences in it source = make_fake_source( @@ -622,27 +600,6 @@ def test_context_folios(self): folios = response.context["folios"] self.assertEqual(list(folios), ["001r", "001v", "002r", "002v"]) - def test_context_feasts_with_folios(self): - cantus_segment = make_fake_segment(id=4063) - source = make_fake_source(segment=cantus_segment) - feast_1 = make_fake_feast() - feast_2 = make_fake_feast() - make_fake_chant(source=source, folio="001r", feast=feast_1) - make_fake_chant(source=source, folio="001r", feast=feast_1) - make_fake_chant(source=source, folio="001v", feast=feast_2) - make_fake_chant(source=source, folio="001v", feast=None) - make_fake_chant(source=source, folio="001v", feast=feast_2) - make_fake_chant(source=source, folio="002r", feast=feast_1) - response = self.client.get(reverse("browse-chants", args=[source.id])) - # context "feasts_with_folios" is a list of tuples - # it records the folios where the feast changes - expected_result = [ - ("001r", feast_1.id, feast_1.name), - ("001v", feast_2.id, feast_2.name), - ("002r", feast_1.id, feast_1.name), - ] - self.assertEqual(response.context["feasts_with_folios"], expected_result) - def test_redirect_with_source_parameter(self): cantus_segment = make_fake_segment(id=4063) source = make_fake_source(segment=cantus_segment) diff --git a/django/cantusdb_project/main_app/views/chant.py b/django/cantusdb_project/main_app/views/chant.py index 7b6e6706e..7b69a0df9 100644 --- a/django/cantusdb_project/main_app/views/chant.py +++ b/django/cantusdb_project/main_app/views/chant.py @@ -1,10 +1,12 @@ import urllib.parse from collections import Counter, defaultdict -from typing import Optional, Iterator, Any +from typing import Optional, Any, Iterator +import string from django.contrib import messages from django.contrib.auth.mixins import LoginRequiredMixin from django.contrib.auth.mixins import UserPassesTestMixin +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 @@ -106,27 +108,122 @@ ) -def get_feast_selector_options(source: Source) -> list[tuple[str, int, str]]: - """Generate folio-feast pairs as options for the feast selector +def split_folio_name(folio: str) -> tuple[str, str, str]: + """ + Splits a folio name into its parts: prefix, number, and suffix. + + If + """ + prefix = folio[0] if folio[0].isalpha() else "" + number = folio.strip(string.ascii_letters) + suffix = folio[-1] if folio[-1].isalpha() else "" + return prefix, number, suffix + + +def create_folio_ranges(folios: list[str]) -> str: + """ + Combines a list of folios (in ascending order) + into a single string with ranges. + + Example: + combine_folio_names(['001r', '001v', '002r', '003r', '003v','005v']) + returns '001r-002r, 003r-003v, 005v' + + Note: The resulting ranges may include folios that do not contain + chants with the given feast *if* the feast is on unnumbered folios or + adjacent to unnumbered folios. For example, if a feast appears on folios + 001v and 002r, but there is an unnumbered folio (per CantusDB convention, + called 001w and 001x) between them, the resulting feast range will appear + as '001v-002r'. Similarly, if the feast appears on 001v and 001x, the range + will appear as '001v-001x'. + """ + folio_ranges: list[dict[str, str]] = [{"start": folios[0]}] + most_recent_folio = folios[0] + most_recent_folio_split = split_folio_name(most_recent_folio) + for curr_folio in folios[1:]: + curr_folio_split = split_folio_name(curr_folio) + # Check if the folio number has incremented by one. + # We use this in the second conditional block below but + # check it here to catch if the folio number is not coercible + # to an integer. + try: + folio_num_inc_1 = ( + int(curr_folio_split[1]) == int(most_recent_folio_split[1]) + 1 + ) + except ValueError: + folio_num_inc_1 = False + # If the folio prefix has changed, start a new range + if curr_folio_split[0] != most_recent_folio_split[0]: + folio_ranges[-1]["end"] = most_recent_folio + folio_ranges.append({"start": curr_folio}) + # Next, we add to the current range if one of the following conditions + # is met: + # 1. The folio number does not change. + # 2. The folio number increases by one and either (a) there is no suffix + # or (b) the suffix of the current folio is "r" and the suffix of the previous + # folio was not "r". + elif (curr_folio_split[1] == most_recent_folio_split[1]) or ( + folio_num_inc_1 + and ( + curr_folio_split[2] == "" + or (curr_folio_split[2] == "r" and most_recent_folio_split[2] != "r") + ) + ): + folio_ranges[-1]["end"] = curr_folio + else: + folio_ranges[-1]["end"] = most_recent_folio + folio_ranges.append({"start": curr_folio}) + most_recent_folio = curr_folio + most_recent_folio_split = curr_folio_split + folio_ranges[-1]["end"] = most_recent_folio + # Create strings in the format "start-end" for each range. + # If a folio range only contains one folio, we don't need to display the end folio. + folio_range_strs = [ + ( + f"{folio_range['start']}-{folio_range['end']}" + if folio_range["start"] != folio_range["end"] + else f"{folio_range['start']}" + ) + for folio_range in folio_ranges + ] + return ", ".join(folio_range_strs) - Going through all chants in the source, folio by folio, - a new entry (in the form of folio-feast) is added when the feast changes. + +def get_feast_selector_options(source: Source) -> list[tuple[int, str, str]]: + """ + Generate a list of feasts in the source to be used in the feast selector + dropdown. Returns a list of tuples in the following format + [ + (feast_id, feast_name, folios), + (feast_id, feast_name, folios), + ... + ] + where folios is a list of folio ranges associated with the feast. Args: - source (Source object): The source that the user is browsing in. + source (Source object): The source for which the dropdown is created. Returns: - list of tuples: A list of folios and Feast objects, to be unpacked in template. + list of tuples: A list of feasts and their associated folios. """ - folios_feasts_iter: Iterator[tuple[Optional[str], int, str]] = ( - source.chant_set.exclude(feast=None) - .select_related("feast", "genre", "service") - .values_list("folio", "feast_id", "feast__name") - .order_by("folio", "c_sequence") + chant_set_w_feasts: QuerySet[Chant, tuple[int, str]] = source.chant_set.exclude( + feast=None + ).values_list("feast_id", "feast__name") + feasts_agg_folios: Iterator[tuple[int, str, list[str]]] = ( + chant_set_w_feasts.annotate(folios=ArrayAgg("folio", distinct=True)) + .order_by("folios") .iterator() ) - deduped_folios_feasts_lists = list(dict.fromkeys(folios_feasts_iter)) - return deduped_folios_feasts_lists + feasts_with_folio_range = [] + for feast_with_folio in feasts_agg_folios: + feasts_with_folio_range.append( + ( + feast_with_folio[0], + feast_with_folio[1], + create_folio_ranges(feast_with_folio[2]), + ) + ) + return feasts_with_folio_range def get_chants_with_feasts( From 8d4d6e24cf8e81dd82e3d177e19344f3b385b773 Mon Sep 17 00:00:00 2001 From: Dylan Hillerbrand Date: Thu, 14 Nov 2024 15:03:17 -0500 Subject: [PATCH 2/3] 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 @@ {% endif %} - {% if pk_specified %} - {% if user.is_authenticated %} + {% if chant %}

View | Edit

- {% endif %}
{% csrf_token %} +
@@ -352,28 +351,28 @@
{% else %} -

Full text & Volpiano edit form

+

Chant Edit Form


-
-
1) Select a folio or a feast (in the right block)
-
2) Click "EDIT" next to any chant
-
3) The fulltext and Volpiano fields should appear in this area
-
4) Edit the fields according to the manuscript, following the fulltext guidelines created by Cantus
-
5) Click "SAVE"
-
-
+
    +
  1. Select a folio or a feast (in the right block).
    2. +
  2. Click "EDIT" next to any chant.
    3. +
  3. A form will appear in this area that allows you to edit the chant data.
    4. +
  4. Edit these fields according to the Cantus Database protocols.
    5. +
  5. Click "SAVE".
    6. +
+
{{ source.short_heading }}
-
+
- + Add new chant + + Add new chant
-
+
- + Add new source + + Add new source
{% endif %} @@ -385,13 +384,13 @@

Full text & Volpiano edit form

{{ source.short_heading }}

- {% if source.chant_set.exists %} + {% if source_has_chants %}