From b8214a2df4d7939f9795adaa8ab21d91261e712d Mon Sep 17 00:00:00 2001
From: Ben Sturmfels <ben@sturm.com.au>
Date: Fri, 25 Oct 2024 11:30:22 +1100
Subject: [PATCH] supporters: Simplify and extend docs

---
 conservancy/supporters/forms.py               | 39 ++++++++++++-------
 .../supporters/sustainers_stripe.html         |  2 +-
 2 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/conservancy/supporters/forms.py b/conservancy/supporters/forms.py
index 80f6e8a5..c6a26017 100644
--- a/conservancy/supporters/forms.py
+++ b/conservancy/supporters/forms.py
@@ -1,6 +1,4 @@
 from django import forms
-from django.utils.safestring import mark_safe
-from django.urls import reverse
 from .models import SustainerOrder
 
 
@@ -10,7 +8,7 @@ class SustainerFormRenderer(forms.renderers.DjangoTemplates):
 
 
 class ButtonRadioSelect(forms.widgets.RadioSelect):
-    """Radio button styled like a button. BYO CSS."""
+    """Radio button styled like a button."""
 
     # Extra <span> wrappers to support CSS
     option_template_name = 'supporters/buttonradio_option.html'
@@ -27,7 +25,25 @@ class ButtonRadioSelect(forms.widgets.RadioSelect):
 
 
 class SustainerForm(forms.ModelForm):
-    # Used to pre-fill the price selector
+    """Sustainer sign-up
+
+    The logic for this form is somewhat spread between this Django form and the Django
+    template and Alpine JS code in the template.
+
+    Having to define some of the the Alpine JS attributes here in the form and some in
+    the template feels awkward, and I wish there was a better way. Django Crispy Forms
+    is typically a good option, but I really wanted to see if the new Django 5 form
+    improvements could beat that (eg. ".as_field_group"). They certainly help, but put
+    several levels of abstraction between you and the HTML (eg. renderers) and spread
+    your HTML across various template and code files. While I appreciate not having to
+    write code to render checked and unchecked boxes, designing attractive interactive
+    forms shouldn't be this complicated.
+
+    Alpine JS has its own trade-offs here. There's nearly no JavaScript as such, but the
+    "x-.." attributes are meaningless until you read the Alpine docs.
+    """
+
+    # To pre-fill the price option buttons in the case of server-side validation errors.
     amount_option = forms.CharField(required=False)
 
     template_name = 'supporters/sustainer_form.html'
@@ -62,7 +78,7 @@ class SustainerForm(forms.ModelForm):
                 }
             ),
             'amount': forms.widgets.NumberInput(
-                # Keeping default widget, just neater to add many attrs here.
+                # Retaining default widget, just neater to add many attrs here.
                 attrs={
                     # So we can update the amount field from the amount_option selected.
                     'x-model': 'amount',
@@ -87,20 +103,15 @@ class SustainerForm(forms.ModelForm):
         recurring = self.cleaned_data.get('recurring', '')
         amount = self.cleaned_data.get('amount', 0)
         minimum = self.MONTH_MINIMUM if recurring == 'month' else self.YEAR_MINIMUM
-        donate_url = reverse('donate')
         if amount < minimum:
-            self.add_error(
-                '',
-                mark_safe(
-                    f'${minimum:d} is a minimum for Conservancy Sustainers. <a href="{donate_url}">Donate smaller amounts here</a>.'
-                ),
-            )
+            self.add_error('', f'${minimum:d} is a minimum for Conservancy Sustainers.')
         tshirt_size = self.cleaned_data.get('tshirt_size')
-        if tshirt_size and not all(
+        address_provided = all(
             [
                 self.cleaned_data.get('street'),
                 self.cleaned_data.get('city'),
                 self.cleaned_data.get('country'),
             ]
-        ):
+        )
+        if tshirt_size and not address_provided:
             self.add_error('street', 'No address provided')
diff --git a/conservancy/supporters/templates/supporters/sustainers_stripe.html b/conservancy/supporters/templates/supporters/sustainers_stripe.html
index 67c550e6..68a52ac8 100644
--- a/conservancy/supporters/templates/supporters/sustainers_stripe.html
+++ b/conservancy/supporters/templates/supporters/sustainers_stripe.html
@@ -50,7 +50,7 @@
 
       {# Alpine JS is used to show different payments amounts for monthly/annual, write the selected payment amount into the "amount" field, reset the seleted amount when you change monthly/annual and pop out the address when you select a T-shirt. #}
       <form method="post" action="."
-        {# Pre-fill field defaults in case of server-side validation error. Otherwise Alpine JS will override them. #}
+        {# Pre-fill field defaults in case of server-side validation error. Otherwise Alpine JS will override them. Could alternatively use the `json_script` tag here. #}
         x-data="{
                  recurring: '{{ form.recurring.value|escapejs }}',
                  amount: parseInt('{{ form.amount.value|escapejs }}'),