Improve AKSubmission

Add docstrings
Remove code-duplications for dispatching user selection on submission overview
Remove code smells
Remove unused admin.py
Remove unused view in views

AKSubmission/admin.py

-# Register your models here.

AKSubmission/api.py

@@ -25,9 +25,13 @@ def ak_interest_indication_active(event, current_timestamp):
 def increment_interest_counter(request, event_slug, pk, **kwargs):
     """
     Increment interest counter for AK
+
+    This view either returns a HTTP 200 if the counter was incremented,
+    an HTTP 403 if indicating interest is currently not allowed,
+    or an HTTP 404 if there is no matching AK for the given primary key and event slug.
     """
     try:
-        ak = AK.objects.get(pk=pk)
+        ak = AK.objects.get(pk=pk, event__slug=event_slug)
         # Check whether interest indication is currently allowed
         current_timestamp = datetime.now().astimezone(ak.event.timezone)
         if ak_interest_indication_active(ak.event, current_timestamp):

AKSubmission/apps.py

@@ -2,4 +2,7 @@ from django.apps import AppConfig
 
 
 class AksubmissionConfig(AppConfig):
+    """
+    App configuration (default, only specifies name of the app)
+    """
     name = 'AKSubmission'

AKSubmission/forms.py

+"""
+Submission-specific forms
+"""
+
 import itertools
 import re
 
@@ -7,10 +11,21 @@ from django.utils.translation import gettext_lazy as _
 
 from AKModel.availability.forms import AvailabilitiesFormMixin
 from AKModel.availability.models import Availability
-from AKModel.models import AK, AKOwner, AKCategory, AKRequirement, AKSlot, AKOrgaMessage, Event
+from AKModel.models import AK, AKOwner, AKCategory, AKRequirement, AKSlot, AKOrgaMessage
 
 
 class AKForm(AvailabilitiesFormMixin, forms.ModelForm):
+    """
+    Base form to add and edit AKs
+
+    Contains suitable widgets for the different data types, restricts querysets (e.g., of requirements) to entries
+    belonging to the event this AK belongs to.
+    Prepares initial slot creation (by accepting multiple input formats and a list of slots to generate),
+    automatically generate short names and wiki links if necessary
+
+    Will be modified/used by :class:`AKSubmissionForm` (that allows to add slots and excludes links)
+    and :class:`AKWishForm`
+    """
     required_css_class = 'required'
     split_string = re.compile('[,;]')
 
@@ -57,7 +72,14 @@ class AKForm(AvailabilitiesFormMixin, forms.ModelForm):
 
     @staticmethod
     def _clean_duration(duration):
-        # Handle different duration formats (h:mm and decimal comma instead of point)
+        """
+        Clean/convert input format for the duration(s) of the slot(s)
+
+        Handle different duration formats (h:mm and decimal comma instead of point)
+
+        :param duration: raw input, either with ":", "," or "."
+        :return: normalized duration (point-separated hour float)
+        """
         if ":" in duration:
             h, m = duration.split(":")
             duration = int(h) + int(m) / 60
@@ -66,31 +88,44 @@ class AKForm(AvailabilitiesFormMixin, forms.ModelForm):
 
         try:
             float(duration)
-        except ValueError:
+        except ValueError as exc:
             raise ValidationError(
                 _('"%(duration)s" is not a valid duration'),
                 code='invalid',
                 params={'duration': duration},
-            )
+            ) from exc
 
         return duration
 
     def clean(self):
+        """
+        Normalize/clean inputs
+
+        Generate a (not yet used) short name if field was left blank, generate a wiki link,
+        create a list of normalized slot durations
+
+        :return: cleaned inputs
+        """
         cleaned_data = super().clean()
 
         # Generate short name if not given
         short_name = self.cleaned_data["short_name"]
         if len(short_name) == 0:
             short_name = self.cleaned_data['name']
+            # First try to split AK name at positions with semantic value (e.g., where the full name is separated
+            # by a ':'), if not possible, do a hard cut at the maximum specified length
             short_name = short_name.partition(':')[0]
             short_name = short_name.partition(' - ')[0]
             short_name = short_name.partition(' (')[0]
             short_name = short_name[:AK._meta.get_field('short_name').max_length]
+            # Check whether this short name already exists...
             for i in itertools.count(1):
+                # ...and either use it...
                 if not AK.objects.filter(short_name=short_name, event=self.cleaned_data["event"]).exists():
                     break
+                # ... or postfix a number starting at 1 and growing until an unused short name is found
                 digits = len(str(i))
-                short_name = '{}-{}'.format(short_name[:-(digits + 1)], i)
+                short_name = f'{short_name[:-(digits + 1)]}-{i}'
             cleaned_data["short_name"] = short_name
 
         # Generate wiki link
@@ -106,35 +141,57 @@ class AKForm(AvailabilitiesFormMixin, forms.ModelForm):
 
 
 class AKSubmissionForm(AKForm):
+    """
+    Form for Submitting new AKs
+
+    Is a special variant of :class:`AKForm` that does not allow to manually edit wiki and protocol links and enforces
+    the generation of at least one slot.
+    """
     class Meta(AKForm.Meta):
-        exclude = ['link', 'protocol_link']
+        # Exclude fields again that were previously included in the parent class
+        exclude = ['link', 'protocol_link'] #pylint: disable=modelform-uses-exclude
 
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
-        # Add field for durations
+        # Add field for durations (cleaning will be handled by parent class)
         self.fields["durations"] = forms.CharField(
             widget=forms.Textarea,
             label=_("Duration(s)"),
             help_text=_(
-                "Enter at least one planned duration (in hours). If your AK should have multiple slots, use multiple lines"),
-            initial=
-            self.initial.get('event').default_slot
+                "Enter at least one planned duration (in hours). "
+                "If your AK should have multiple slots, use multiple lines"),
+            initial=self.initial.get('event').default_slot
         )
 
     def clean_availabilities(self):
+        """
+        Automatically improve availabilities entered.
+        If the user did not specify availabilities assume the full event duration is possible
+        :return: cleaned availabilities
+        (either user input or one availability for the full length of the event if user input was empty)
+        """
         availabilities = super().clean_availabilities()
-        # If the user did not specify availabilities assume the full event duration is possible
         if len(availabilities) == 0:
             availabilities.append(Availability.with_event_length(event=self.cleaned_data["event"]))
         return availabilities
 
 
 class AKWishForm(AKForm):
+    """
+    Form for submitting or editing wishes
+
+    Is a special variant of :class:`AKForm` that does not allow to specify owner(s) or
+    manually edit wiki and protocol links
+    """
     class Meta(AKForm.Meta):
-        exclude = ['owners', 'link', 'protocol_link']
+        # Exclude fields again that were previously included in the parent class
+        exclude = ['owners', 'link', 'protocol_link'] #pylint: disable=modelform-uses-exclude
 
 
 class AKOwnerForm(forms.ModelForm):
+    """
+    Form to create/edit AK owners
+    """
     required_css_class = 'required'
 
     class Meta:
@@ -146,6 +203,9 @@ class AKOwnerForm(forms.ModelForm):
 
 
 class AKDurationForm(forms.ModelForm):
+    """
+    Form to add an additional slot to a given AK
+    """
     class Meta:
         model = AKSlot
         fields = ['duration', 'ak', 'event']
@@ -156,6 +216,9 @@ class AKDurationForm(forms.ModelForm):
 
 
 class AKOrgaMessageForm(forms.ModelForm):
+    """
+    Form to create a confidential message to the organizers  belonging to a given AK
+    """
     class Meta:
         model = AKOrgaMessage
         fields = ['ak', 'text', 'event']

AKSubmission/models.py

@@ -3,14 +3,15 @@ from django.conf import settings
 from django.core.mail import EmailMessage
 from django.db.models.signals import post_save
 from django.dispatch import receiver
-from django.urls import reverse_lazy
 
 from AKModel.models import AKOrgaMessage, AKSlot
 
 
 @receiver(post_save, sender=AKOrgaMessage)
-def orga_message_saved_handler(sender, instance: AKOrgaMessage, created, **kwargs):
-    # React to newly created Orga message by sending an email
+def orga_message_saved_handler(sender, instance: AKOrgaMessage, created, **kwargs): # pylint: disable=unused-argument
+    """
+    React to newly created Orga message by sending an email
+    """
 
     if created and settings.SEND_MAILS:
         host = 'https://' + settings.ALLOWED_HOSTS[0] if len(settings.ALLOWED_HOSTS) > 0 else 'http://127.0.0.1:8000'
@@ -26,10 +27,12 @@ def orga_message_saved_handler(sender, instance: AKOrgaMessage, created, **kwarg
 
 
 @receiver(post_save, sender=AKSlot)
-def slot_created_handler(sender, instance: AKSlot, created, **kwargs):
-    # React to slots that are created after the plan was already published by sending an email
-
-    if created and settings.SEND_MAILS and apps.is_installed("AKPlan") and not instance.event.plan_hidden and instance.room is None and instance.start is None:
+def slot_created_handler(sender, instance: AKSlot, created, **kwargs): # pylint: disable=unused-argument
+    """
+    React to slots that are created after the plan was already published by sending an email
+    """
+    if created and settings.SEND_MAILS and apps.is_installed("AKPlan") \
+            and not instance.event.plan_hidden and instance.room is None and instance.start is None: # pylint: disable=too-many-boolean-expressions,line-too-long
         host = 'https://' + settings.ALLOWED_HOSTS[0] if len(settings.ALLOWED_HOSTS) > 0 else 'http://127.0.0.1:8000'
         url = f"{host}{instance.ak.detail_url}"
 

AKSubmission/templatetags/tags_AKSubmission.py

@@ -6,6 +6,11 @@ register = template.Library()
 
 @register.filter
 def bool_symbol(bool_val):
+    """
+    Show a nice icon instead of the string true/false
+    :param bool_val: boolean value to iconify
+    :return: check or times icon depending on the value
+    """
     if bool_val:
         return fa6_icon("check", "fas")
     return fa6_icon("times", "fas")
@@ -13,14 +18,34 @@ def bool_symbol(bool_val):
 
 @register.inclusion_tag("AKSubmission/tracks_list.html")
 def track_list(tracks, event_slug):
+    """
+    Generate a clickable list of tracks (one badge per track) based upon the tracks_list template
+
+    :param tracks: tracks to consider
+    :param event_slug: slug of this event, required for link creation
+    :return: html fragment containing track links
+    """
     return {"tracks": tracks, "event_slug": event_slug}
 
 
 @register.inclusion_tag("AKSubmission/category_list.html")
 def category_list(categories, event_slug):
+    """
+    Generate a clickable list of categories (one badge per category) based upon the category_list template
+
+    :param categories: categories to consider
+    :param event_slug: slug of this event, required for link creation
+    :return: html fragment containing category links
+    """
     return {"categories": categories, "event_slug": event_slug}
 
 
 @register.inclusion_tag("AKSubmission/category_linked_badge.html")
 def category_linked_badge(category, event_slug):
+    """
+    Generate a clickable category badge based upon the category_linked_badge template
+    :param category: category to show/link
+    :param event_slug: slug of this event, required for link creation
+    :return: html fragment containing badge
+    """
     return {"category": category, "event_slug": event_slug}

AKSubmission/tests.py

@@ -9,6 +9,15 @@ from AKModel.tests import BasicViewTests
 
 
 class ModelViewTests(BasicViewTests, TestCase):
+    """
+    Testcases for AKSubmission app.
+
+    This extends :class:`BasicViewTests` for standard view and edit testcases
+    that are specified in this class as VIEWS and EDIT_TESTCASES.
+
+    Additionally several additional testcases, in particular to test the API
+    and the dispatching for owner selection and editing are specified.
+    """
     fixtures = ['model.json']
 
     VIEWS = [
@@ -47,24 +56,27 @@ class ModelViewTests(BasicViewTests, TestCase):
         """
         self.client.logout()
 
-        view_name_with_prefix, url = self._name_and_url(('akslot_edit', {'event_slug': 'kif42', 'pk': 1}))
+        _, url = self._name_and_url(('akslot_edit', {'event_slug': 'kif42', 'pk': 1}))
         response = self.client.get(url)
         self.assertEqual(response.status_code, 302,
                          msg=f"AK Slot editing ({url}) possible even though slot was already scheduled")
         self._assert_message(response, "You cannot edit a slot that has already been scheduled")
 
-        view_name_with_prefix, url = self._name_and_url(('akslot_delete', {'event_slug': 'kif42', 'pk': 1}))
+        _, url = self._name_and_url(('akslot_delete', {'event_slug': 'kif42', 'pk': 1}))
         response = self.client.get(url)
         self.assertEqual(response.status_code, 302,
                          msg=f"AK Slot deletion ({url}) possible even though slot was already scheduled")
         self._assert_message(response, "You cannot delete a slot that has already been scheduled")
 
     def test_slot_creation_deletion(self):
+        """
+        Test creation and deletion of slots in frontend
+        """
         ak_args = {'event_slug': 'kif42', 'pk': 1}
         redirect_url = reverse_lazy(f"{self.APP_NAME}:ak_detail", kwargs=ak_args)
 
+        # Create a valid slot -> Redirect to AK detail page, message to user
         count_slots = AK.objects.get(pk=1).akslot_set.count()
-
         create_url = reverse_lazy(f"{self.APP_NAME}:akslot_add", kwargs=ak_args)
         response = self.client.post(create_url, {'ak': 1, 'event': 2, 'duration': 1.5})
         self.assertRedirects(response, redirect_url, status_code=302, target_status_code=200,
@@ -75,6 +87,8 @@ class ModelViewTests(BasicViewTests, TestCase):
         # Get primary key of newly created Slot
         slot_pk = AK.objects.get(pk=1).akslot_set.order_by('pk').last().pk
 
+        # Edit the recently created slot: Make sure view is accessible, post change
+        # -> redirect to detail page, duration updated
         edit_url = reverse_lazy(f"{self.APP_NAME}:akslot_edit", kwargs={'event_slug': 'kif42', 'pk': slot_pk})
         response = self.client.get(edit_url)
         self.assertEqual(response.status_code, 200, msg=f"Cant open edit view for newly created slot ({edit_url})")
@@ -84,6 +98,8 @@ class ModelViewTests(BasicViewTests, TestCase):
         self.assertEqual(AKSlot.objects.get(pk=slot_pk).duration, 2,
                          msg="Slot was not correctly changed")
 
+        # Delete recently created slot: Make sure view is accessible, post deletion
+        # -> redirect to detail page, slot deleted, message to user
         deletion_url = reverse_lazy(f"{self.APP_NAME}:akslot_delete", kwargs={'event_slug': 'kif42', 'pk': slot_pk})
         response = self.client.get(deletion_url)
         self.assertEqual(response.status_code, 200,
@@ -95,55 +111,77 @@ class ModelViewTests(BasicViewTests, TestCase):
         self.assertEqual(AK.objects.get(pk=1).akslot_set.count(), count_slots, msg="AK still has to many slots")
 
     def test_ak_owner_editing(self):
-        # Test editing of new user
+        """
+        Test dispatch of user editing requests
+        """
         edit_url = reverse_lazy(f"{self.APP_NAME}:akowner_edit_dispatch", kwargs={'event_slug': 'kif42'})
 
         base_url = reverse_lazy(f"{self.APP_NAME}:submission_overview", kwargs={'event_slug': 'kif42'})
+
+        # Empty form/no user selected -> start page
         response = self.client.post(edit_url, {'owner_id': -1})
         self.assertRedirects(response, base_url, status_code=302, target_status_code=200,
                              msg_prefix="Did not redirect to start page even though no user was selected")
         self._assert_message(response, "No user selected")
 
+        # Correct selection -> user edit page for that user
         edit_redirect_url = reverse_lazy(f"{self.APP_NAME}:akowner_edit", kwargs={'event_slug': 'kif42', 'slug': 'a'})
         response = self.client.post(edit_url, {'owner_id': 1})
         self.assertRedirects(response, edit_redirect_url, status_code=302, target_status_code=200,
                              msg_prefix=f"Dispatch redirect failed (should go to {edit_redirect_url})")
 
     def test_ak_owner_selection(self):
+        """
+        Test dispatch of owner selection requests
+        """
         select_url = reverse_lazy(f"{self.APP_NAME}:akowner_select", kwargs={'event_slug': 'kif42'})
-
         create_url = reverse_lazy(f"{self.APP_NAME}:akowner_create", kwargs={'event_slug': 'kif42'})
+
+        # Empty user selection -> create a new user view
         response = self.client.post(select_url, {'owner_id': -1})
         self.assertRedirects(response, create_url, status_code=302, target_status_code=200,
                              msg_prefix="Did not redirect to user create view even though no user was specified")
 
+        # Valid user selected -> redirect to view that allows to add a new AK with this user as owner
         add_redirect_url = reverse_lazy(f"{self.APP_NAME}:submit_ak", kwargs={'event_slug': 'kif42', 'owner_slug': 'a'})
         response = self.client.post(select_url, {'owner_id': 1})
         self.assertRedirects(response, add_redirect_url, status_code=302, target_status_code=200,
                     msg_prefix=f"Dispatch redirect to ak submission page failed (should go to {add_redirect_url})")
 
     def test_orga_message_submission(self):
+        """
+        Test submission and storing of direct confident messages to organizers
+        """
         form_url = reverse_lazy(f"{self.APP_NAME}:akmessage_add", kwargs={'event_slug': 'kif42', 'pk': 1})
         detail_url = reverse_lazy(f"{self.APP_NAME}:ak_detail", kwargs={'event_slug': 'kif42', 'pk': 1})
 
         count_messages = AK.objects.get(pk=1).akorgamessage_set.count()
 
+        # Test that submission view is accessible
         response = self.client.get(form_url)
         self.assertEqual(response.status_code, 200, msg="Could not load message form view")
+
+        # Test submission itself and the following redirect -> AK detail page
         response = self.client.post(form_url, {'ak': 1, 'event': 2, 'text': 'Test message text'})
         self.assertRedirects(response, detail_url, status_code=302, target_status_code=200,
                              msg_prefix=f"Did not trigger redirect to ak detail page ({detail_url})")
+
+        # Make sure message was correctly saved in database and user is notified about that
         self._assert_message(response, "Message to organizers successfully saved")
         self.assertEqual(AK.objects.get(pk=1).akorgamessage_set.count(), count_messages + 1,
                          msg="Message was not correctly saved")
 
     def test_interest_api(self):
+        """
+        Test interest indicating API (access, functionality)
+        """
         interest_api_url = "/kif42/api/ak/1/indicate-interest/"
 
         ak = AK.objects.get(pk=1)
         event = Event.objects.get(slug='kif42')
         ak_interest_counter = ak.interest_counter
 
+        # Check Access method (only POST)
         response = self.client.get(interest_api_url)
         self.assertEqual(response.status_code, 405, "Should not be accessible via GET")
 
@@ -151,6 +189,7 @@ class ModelViewTests(BasicViewTests, TestCase):
         event.interest_end = datetime.now().astimezone(event.timezone) + timedelta(minutes=+10)
         event.save()
 
+        # Test correct indication -> HTTP 200, counter increased
         response = self.client.post(interest_api_url)
         self.assertEqual(response.status_code, 200, f"API end point not working ({interest_api_url})")
         self.assertEqual(AK.objects.get(pk=1).interest_counter, ak_interest_counter + 1, "Counter was not increased")
@@ -158,30 +197,41 @@ class ModelViewTests(BasicViewTests, TestCase):
         event.interest_end = datetime.now().astimezone(event.timezone) + timedelta(minutes=-2)
         event.save()
 
+        # Test indication outside of indication window -> HTTP 403, counter not increased
         response = self.client.post(interest_api_url)
         self.assertEqual(response.status_code, 403,
                     "API end point still reachable even though interest indication window ended ({interest_api_url})")
         self.assertEqual(AK.objects.get(pk=1).interest_counter, ak_interest_counter + 1,
                          "Counter was increased even though interest indication window ended")
 
+        # Test call for non-existing AK -> HTTP 403
         invalid_interest_api_url = "/kif42/api/ak/-1/indicate-interest/"
         response = self.client.post(invalid_interest_api_url)
         self.assertEqual(response.status_code, 404, f"Invalid URL reachable ({interest_api_url})")
 
     def test_adding_of_unknown_user(self):
+        """
+        Test adding of a previously not existing owner to an AK
+        """
+        # Pre-Check: AK detail page existing?
         detail_url = reverse_lazy(f"{self.APP_NAME}:ak_detail", kwargs={'event_slug': 'kif42', 'pk': 1})
         response = self.client.get(detail_url)
         self.assertEqual(response.status_code, 200, msg="Could not load ak detail view")
 
+        # Make sure AK detail page contains a link to add a new owner
         edit_url = reverse_lazy(f"{self.APP_NAME}:ak_edit", kwargs={'event_slug': 'kif42', 'pk': 1})
         response = self.client.get(edit_url)
         self.assertEqual(response.status_code, 200, msg="Could not load ak detail view")
         self.assertContains(response, "Add person not in the list yet",
                             msg_prefix="Link to add unknown user not contained")
 
+        # Check adding of a new owner by posting an according request
+        # -> Redirect to AK detail page, message to user, owners list updated
         self.assertEqual(AK.objects.get(pk=1).owners.count(), 1)
-        add_new_user_to_ak_url = reverse_lazy(f"{self.APP_NAME}:akowner_create", kwargs={'event_slug': 'kif42'}) + f"?add_to_existing_ak=1"
-        response = self.client.post(add_new_user_to_ak_url, {'name': 'New test owner', 'event': Event.get_by_slug('kif42').pk})
+        add_new_user_to_ak_url = reverse_lazy(f"{self.APP_NAME}:akowner_create", kwargs={'event_slug': 'kif42'}) \
+                                 + "?add_to_existing_ak=1"
+        response = self.client.post(add_new_user_to_ak_url,
+                                    {'name': 'New test owner', 'event': Event.get_by_slug('kif42').pk})
         self.assertRedirects(response, detail_url,
                              msg_prefix=f"No correct redirect: {add_new_user_to_ak_url} (POST) -> {detail_url}")
         self._assert_message(response, "Added 'New test owner' as new owner of 'Test AK Inhalt'")