Sponsorship review ux (#1684)

* Separate package benefits in application form

* Flag sponsors benefits added by user

* Split package and added benefits in full sponsorship detail

* Filter by package only benefits

* Enable Sponsorship's state control

* Prevents from rejecting or accepting reviewd applications

* Add buttons to approve/reject sponsorship application

* Implement HTML to reject/approve sponsorship

* Improve admin

* Display fee when rejecting/approving

* Fix breaking tests

* Display detailed sponsor's address information

* Should appennd, not overwrite HTML snippet

* Fix f-string and change postal code order

* Add titles to approve/reject html

* Better display sponsorship

Author: berinhard

sponsors/admin.py

@@ -19,6 +19,7 @@
 )
 from sponsors import use_cases
 from sponsors.forms import SponsorshipReviewAdminForm
+from sponsors.exceptions import SponsorshipInvalidStatusException
 from cms.admin import ContentManageableModelAdmin
 
 
@@ -101,16 +102,19 @@ class SponsorBenefitInline(admin.TabularInline):
 
 @admin.register(Sponsorship)
 class SponsorshipAdmin(admin.ModelAdmin):
+    change_form_template = "sponsors/admin/sponsorship_change_form.html"
     form = SponsorshipReviewAdminForm
     inlines = [SponsorBenefitInline]
     list_display = [
         "sponsor",
+        "status",
         "applied_on",
         "approved_on",
         "start_date",
         "end_date",
         "display_sponsorship_link",
     ]
+    list_filter = ["status"]
     readonly_fields = [
         "for_modified_package",
         "sponsor",
@@ -260,7 +264,19 @@ def get_sponsor_primary_phone(self, obj):
     get_sponsor_primary_phone.short_description = "Primary Phone"
 
     def get_sponsor_mailing_address(self, obj):
-        return obj.sponsor.mailing_address
+        sponsor = obj.sponsor
+        city_row = f'{sponsor.city} - {sponsor.get_country_display()} ({sponsor.country})'
+        if sponsor.state:
+            city_row = f'{sponsor.city} - {sponsor.state} - {sponsor.get_country_display()} ({sponsor.country})'
+
+        mail_row = sponsor.mailing_address_line_1
+        if sponsor.mailing_address_line_2:
+            mail_row += f' - {sponsor.mailing_address_line_2}'
+
+        html = f'<p>{city_row}</p>'
+        html += f'<p>{mail_row}</p>'
+        html += f'<p>{sponsor.postal_code}</p>'
+        return mark_safe(html)
 
     get_sponsor_mailing_address.short_description = "Mailing/Billing Address"
 
@@ -276,7 +292,7 @@ def get_sponsor_contacts(self, obj):
             )
             html += "</ul>"
         if not_primary:
-            html = "<b>Other contacts</b><ul>"
+            html += "<b>Other contacts</b><ul>"
             html += "".join(
                 [f"<li>{c.name}: {c.email} / {c.phone}</li>" for c in not_primary]
             )
@@ -289,12 +305,18 @@ def reject_sponsorship_view(self, request, pk):
         sponsorship = get_object_or_404(self.get_queryset(request), pk=pk)
 
         if request.method.upper() == "POST" and request.POST.get("confirm") == "yes":
-            use_case = use_cases.RejectSponsorshipApplicationUseCase.build()
-            use_case.execute(sponsorship)
+            try:
+                use_case = use_cases.RejectSponsorshipApplicationUseCase.build()
+                use_case.execute(sponsorship)
+                self.message_user(
+                    request, "Sponsorship was rejected!", messages.SUCCESS
+                )
+            except SponsorshipInvalidStatusException as e:
+                self.message_user(request, str(e), messages.ERROR)
+
             redirect_url = reverse(
                 "admin:sponsors_sponsorship_change", args=[sponsorship.pk]
             )
-            self.message_user(request, "Sponsorship was rejected!", messages.SUCCESS)
             return redirect(redirect_url)
 
         context = {"sponsorship": sponsorship}
@@ -306,12 +328,18 @@ def approve_sponsorship_view(self, request, pk):
         sponsorship = get_object_or_404(self.get_queryset(request), pk=pk)
 
         if request.method.upper() == "POST" and request.POST.get("confirm") == "yes":
-            sponsorship.approve()
-            sponsorship.save()
+            try:
+                sponsorship.approve()
+                sponsorship.save()
+                self.message_user(
+                    request, "Sponsorship was approved!", messages.SUCCESS
+                )
+            except SponsorshipInvalidStatusException as e:
+                self.message_user(request, str(e), messages.ERROR)
+
             redirect_url = reverse(
                 "admin:sponsors_sponsorship_change", args=[sponsorship.pk]
             )
-            self.message_user(request, "Sponsorship was approved!", messages.SUCCESS)
             return redirect(redirect_url)
 
         context = {"sponsorship": sponsorship}

sponsors/exceptions.py

@@ -3,3 +3,10 @@ class SponsorWithExistingApplicationException(Exception):
     Raised when user tries to create a new Sponsorship application
     for a Sponsor which already has applications pending to review
     """
+
+
+class SponsorshipInvalidStatusException(Exception):
+    """
+    Raised when user tries to change the Sponsorship's status
+    to a new one but from an invalid current status
+    """

sponsors/models.py

@@ -15,7 +15,10 @@
 from companies.models import Company
 
 from .managers import SponsorshipQuerySet
-from .exceptions import SponsorWithExistingApplicationException
+from .exceptions import (
+    SponsorWithExistingApplicationException,
+    SponsorshipInvalidStatusException,
+)
 
 DEFAULT_MARKUP_TYPE = getattr(settings, "DEFAULT_MARKUP_TYPE", "restructuredtext")
 
@@ -267,6 +270,15 @@ class Sponsorship(models.Model):
     level_name = models.CharField(max_length=64, default="")
     sponsorship_fee = models.PositiveIntegerField(null=True, blank=True)
 
+    def __str__(self):
+        repr = f"{self.level_name} ({self.get_status_display()}) for sponsor {self.sponsor.name}"
+        if self.start_date and self.end_date:
+            fmt = "%m/%d/%Y"
+            start = self.start_date.strftime(fmt)
+            end = self.end_date.strftime(fmt)
+            repr += f" [{start} - {end}]"
+        return repr
+
     @classmethod
     def new(cls, sponsor, benefits, package=None, submited_by=None):
         """
@@ -317,10 +329,16 @@ def estimated_cost(self):
         )
 
     def reject(self):
+        if self.REJECTED not in self.next_status:
+            msg = f"Can't reject a {self.get_status_display()} sponsorship."
+            raise SponsorshipInvalidStatusException(msg)
         self.status = self.REJECTED
         self.rejected_on = timezone.now().date()
 
     def approve(self):
+        if self.APPROVED not in self.next_status:
+            msg = f"Can't approve a {self.get_status_display()} sponsorship."
+            raise SponsorshipInvalidStatusException(msg)
         self.status = self.APPROVED
         self.approved_on = timezone.now().date()
 
@@ -343,6 +361,16 @@ def package_benefits(self):
     def added_benefits(self):
         return self.benefits.filter(added_by_user=True)
 
+    @property
+    def next_status(self):
+        states_map = {
+            self.APPLIED: [self.APPROVED, self.REJECTED],
+            self.APPROVED: [self.FINALIZED],
+            self.REJECTED: [],
+            self.FINALIZED: [],
+        }
+        return states_map[self.status]
+
 
 class SponsorBenefit(models.Model):
     sponsorship = models.ForeignKey(

sponsors/templatetags/sponsors.py

@@ -7,9 +7,12 @@
 
 
 @register.inclusion_tag("sponsors/partials/full_sponsorship.txt")
-def full_sponsorship(sponsorship):
+def full_sponsorship(sponsorship, display_fee=False):
+    if not display_fee:
+        display_fee = not sponsorship.for_modified_package
     return {
         "sponsorship": sponsorship,
         "sponsor": sponsorship.sponsor,
         "benefits": list(sponsorship.benefits.all()),
+        "display_fee": display_fee,
     }

sponsors/tests/test_models.py

@@ -41,6 +41,17 @@ def setUp(self):
         self.sponsor = baker.make("sponsors.Sponsor")
         self.user = baker.make(settings.AUTH_USER_MODEL)
 
+    def test_control_sponsorship_next_status(self):
+        states_map = {
+            Sponsorship.APPLIED: [Sponsorship.APPROVED, Sponsorship.REJECTED],
+            Sponsorship.APPROVED: [Sponsorship.FINALIZED],
+            Sponsorship.REJECTED: [],
+            Sponsorship.FINALIZED: [],
+        }
+        for status, exepcted in states_map.items():
+            sponsorship = baker.prepare(Sponsorship, status=status)
+            self.assertEqual(sponsorship.next_status, exepcted)
+
     def test_create_new_sponsorship(self):
         sponsorship = Sponsorship.new(
             self.sponsor, self.benefits, submited_by=self.user

sponsors/tests/test_notifications.py

@@ -14,7 +14,7 @@ class AppliedSponsorshipNotificationToPSFTests(TestCase):
     def setUp(self):
         self.notification = notifications.AppliedSponsorshipNotificationToPSF()
         self.user = baker.make(settings.AUTH_USER_MODEL)
-        self.sponsorship = baker.make("sponsors.Sponsorship")
+        self.sponsorship = baker.make("sponsors.Sponsorship", sponsor__name="foo")
         self.subject_template = "sponsors/email/psf_new_application_subject.txt"
         self.content_template = "sponsors/email/psf_new_application.txt"
 
@@ -41,7 +41,10 @@ def setUp(self):
         self.unverified_email = baker.make(EmailAddress, verified=False)
         self.sponsor_contacts = [
             baker.make(
-                "sponsors.SponsorContact", email="[email protected]", primary=True
+                "sponsors.SponsorContact",
+                email="[email protected]",
+                primary=True,
+                sponsor__name="foo",
             ),
             baker.make("sponsors.SponsorContact", email=self.verified_email.email),
             baker.make("sponsors.SponsorContact", email=self.unverified_email.email),
@@ -79,7 +82,9 @@ class RejectedSponsorshipNotificationToPSFTests(TestCase):
     def setUp(self):
         self.notification = notifications.RejectedSponsorshipNotificationToPSF()
         self.sponsorship = baker.make(
-            Sponsorship, status=Sponsorship.REJECTED, _fill_optional=["rejected_on"]
+            Sponsorship,
+            status=Sponsorship.REJECTED,
+            _fill_optional=["rejected_on", "sponsor"],
         )
         self.subject_template = "sponsors/email/psf_rejected_sponsorship_subject.txt"
         self.content_template = "sponsors/email/psf_rejected_sponsorship.txt"
@@ -106,7 +111,7 @@ def setUp(self):
         self.sponsorship = baker.make(
             Sponsorship,
             status=Sponsorship.REJECTED,
-            _fill_optional=["rejected_on"],
+            _fill_optional=["rejected_on", "sponsor"],
             submited_by=self.user,
         )
         self.subject_template = (
@@ -133,7 +138,9 @@ class StatementOfWorkNotificationToPSFTests(TestCase):
     def setUp(self):
         self.notification = notifications.StatementOfWorkNotificationToPSF()
         self.sponsorship = baker.make(
-            Sponsorship, status=Sponsorship.APPROVED, _fill_optional=["approved_on"]
+            Sponsorship,
+            status=Sponsorship.APPROVED,
+            _fill_optional=["approved_on", "sponsor"],
         )
         self.subject_template = "sponsors/email/psf_statement_of_work_subject.txt"
         self.content_template = "sponsors/email/psf_statement_of_work.txt"
@@ -160,7 +167,7 @@ def setUp(self):
         self.sponsorship = baker.make(
             Sponsorship,
             status=Sponsorship.APPROVED,
-            _fill_optional=["approved_on"],
+            _fill_optional=["approved_on", "sponsor"],
             submited_by=self.user,
         )
         self.subject_template = "sponsors/email/sponsor_statement_of_work_subject.txt"

sponsors/tests/test_templatetags.py

@@ -9,11 +9,28 @@
 
 class FullSponsorshipTemplatetagTests(TestCase):
     def test_templatetag_context(self):
-        sponsorship = baker.make("sponsors.Sponsorship", _fill_optional=True)
+        sponsorship = baker.make(
+            "sponsors.Sponsorship", for_modified_package=False, _fill_optional=True
+        )
         context = full_sponsorship(sponsorship)
         expected = {
             "sponsorship": sponsorship,
             "sponsor": sponsorship.sponsor,
             "benefits": list(sponsorship.benefits.all()),
+            "display_fee": True,
         }
         self.assertEqual(context, expected)
+
+    def test_do_not_display_fee_if_modified_package(self):
+        sponsorship = baker.make(
+            "sponsors.Sponsorship", for_modified_package=True, _fill_optional=True
+        )
+        context = full_sponsorship(sponsorship)
+        self.assertFalse(context["display_fee"])
+
+    def test_allows_to_overwrite_display_fee_flag(self):
+        sponsorship = baker.make(
+            "sponsors.Sponsorship", for_modified_package=True, _fill_optional=True
+        )
+        context = full_sponsorship(sponsorship, display_fee=True)
+        self.assertTrue(context["display_fee"])

sponsors/tests/test_views.py

@@ -327,7 +327,12 @@ def setUp(self):
             settings.AUTH_USER_MODEL, is_staff=True, is_superuser=True
         )
         self.client.force_login(self.user)
-        self.sponsorship = baker.make(Sponsorship, submited_by=self.user)
+        self.sponsorship = baker.make(
+            Sponsorship,
+            status=Sponsorship.APPLIED,
+            submited_by=self.user,
+            _fill_optional=True,
+        )
         self.url = reverse(
             "admin:sponsors_sponsorship_reject", args=[self.sponsorship.pk]
         )
@@ -395,14 +400,31 @@ def test_staff_required(self):
 
         self.assertRedirects(r, redirect_url, fetch_redirect_response=False)
 
+    def test_message_user_if_rejecting_invalid_sponsorship(self):
+        self.sponsorship.status = Sponsorship.FINALIZED
+        self.sponsorship.save()
+        data = {"confirm": "yes"}
+        response = self.client.post(self.url, data=data)
+        self.sponsorship.refresh_from_db()
+
+        expected_url = reverse(
+            "admin:sponsors_sponsorship_change", args=[self.sponsorship.pk]
+        )
+        self.assertRedirects(response, expected_url, fetch_redirect_response=True)
+        self.assertEqual(self.sponsorship.status, Sponsorship.FINALIZED)
+        msg = list(get_messages(response.wsgi_request))[0]
+        assertMessage(msg, "Can't reject a Finalized sponsorship.", messages.ERROR)
+
 
 class ApproveSponsorshipAdminViewTests(TestCase):
     def setUp(self):
         self.user = baker.make(
             settings.AUTH_USER_MODEL, is_staff=True, is_superuser=True
         )
         self.client.force_login(self.user)
-        self.sponsorship = baker.make(Sponsorship)
+        self.sponsorship = baker.make(
+            Sponsorship, status=Sponsorship.APPLIED, _fill_optional=True
+        )
         self.url = reverse(
             "admin:sponsors_sponsorship_approve", args=[self.sponsorship.pk]
         )
@@ -468,3 +490,18 @@ def test_staff_required(self):
         r = self.client.get(self.url)
 
         self.assertRedirects(r, redirect_url, fetch_redirect_response=False)
+
+    def test_message_user_if_approving_invalid_sponsorship(self):
+        self.sponsorship.status = Sponsorship.FINALIZED
+        self.sponsorship.save()
+        data = {"confirm": "yes"}
+        response = self.client.post(self.url, data=data)
+        self.sponsorship.refresh_from_db()
+
+        expected_url = reverse(
+            "admin:sponsors_sponsorship_change", args=[self.sponsorship.pk]
+        )
+        self.assertRedirects(response, expected_url, fetch_redirect_response=True)
+        self.assertEqual(self.sponsorship.status, Sponsorship.FINALIZED)
+        msg = list(get_messages(response.wsgi_request))[0]
+        assertMessage(msg, "Can't approve a Finalized sponsorship.", messages.ERROR)