Merge pull request #1084 from readthedocs/davidfischer/allow-ad-deletion

Allow ad deletion (restrictions apply)

Author: davidfischer

adserver/models.py

@@ -103,7 +103,9 @@ class IndestructibleModel(models.Model):
     objects = IndestructibleManager()
 
     def delete(self, using=None, keep_parents=False):
-        """Always raises ``IntegrityError``."""
+        """Raises `IntegrityError` unless the model also has a `can_be_deleted()` method and it returns True."""
+        if hasattr(self, "can_be_deleted") and self.can_be_deleted():
+            return super().delete(using=using, keep_parents=keep_parents)
         raise IntegrityError
 
     class Meta:
@@ -1854,6 +1856,11 @@ def __str__(self):
         """Simple override."""
         return self.name
 
+    def can_be_deleted(self):
+        """Check if this advertisement can be deleted."""
+        # An advertisement can be deleted if it has never been offered
+        return not AdImpression.objects.filter(advertisement=self).exists()
+
     def get_absolute_url(self):
         return reverse(
             "advertisement_detail",

adserver/templates/adserver/advertiser/advertisement-delete.html

@@ -0,0 +1,44 @@
+{% extends "adserver/advertiser/overview.html" %}
+{% load i18n %}
+{% load static %}
+{% load humanize %}
+
+
+{% block title %}{% trans 'Remove advertisement' %}{% endblock %}
+
+
+{% block breadcrumbs %}
+  {{ block.super }}
+  <li class="breadcrumb-item"><a href="{% url 'flight_list' advertiser.slug %}">{% trans 'Flights' %}</a></li>
+  <li class="breadcrumb-item"><a href="{% url 'flight_detail' advertiser.slug flight.slug %}">{{ flight.name }}</a></li>
+  <li class="breadcrumb-item"><a href="{% url 'advertisement_detail' advertiser.slug flight.slug advertisement.slug %}">{{ advertisement.name }}</a></li>
+  <li class="breadcrumb-item active">{% trans 'Remove advertisement' %}</li>
+{% endblock breadcrumbs %}
+
+
+{% block content_container %}
+
+<section>
+
+  <h1>{% block heading %}{% blocktrans with ad_name=advertisement.name %}Remove {{ ad_name }}?{% endblocktrans %}{% endblock heading %}</h1>
+  <p>{% blocktrans %}Only ads that have never been shown can be deleted.{% endblocktrans %}</p>
+
+  <div class="row">
+
+    <div class="col-md">
+
+      {% for ad_type in advertisement.ad_types.all %}
+        {% include "adserver/includes/ad-preview.html" %}
+      {% endfor %}
+
+      <form method="post">
+        {% csrf_token %}
+        <input type="submit" class="btn btn-danger" value="{% trans "Remove advertisement" %}">
+      </form>
+    </div>
+
+  </div>
+
+</section>
+
+{% endblock content_container %}

adserver/templates/adserver/advertiser/advertisement-detail.html

@@ -67,6 +67,9 @@ <h1>{% block heading %}{% trans 'Advertisement: ' %}{{ advertisement.name }}{% e
   {% if has_advertiser_edit_permission %}
   <div class="col">
     <a href="{% url 'advertisement_update' advertiser.slug advertisement.flight.slug advertisement.slug %}" class="btn btn-outline-primary" role="button" aria-pressed="true">{% trans 'Edit advertisement' %}</a>
+    {% if advertisement.can_be_deleted %}
+      <a href="{% url 'advertisement_remove' advertiser.slug advertisement.flight.slug advertisement.slug %}" class="btn btn-outline-danger" role="button" aria-pressed="true" title="{% trans 'Only advertisements that have never been shown can be removed. You will have an opportunity to confirm.' %}">{% trans 'Remove advertisement' %}</a>
+    {% endif %}
   </div>
   {% endif %}
 </div>

adserver/tests/test_advertiser_dashboard.py

@@ -897,6 +897,87 @@ def test_ad_create_view(self):
             Advertisement.objects.filter(flight=self.flight, name="New Name").exists()
         )
 
+    def test_ad_delete_view(self):
+        url = reverse(
+            "advertisement_remove",
+            kwargs={
+                "advertiser_slug": self.advertiser.slug,
+                "flight_slug": self.flight.slug,
+                "advertisement_slug": self.ad1.slug,
+            },
+        )
+
+        # Anonymous - no access
+        response = self.client.get(url)
+        self.assertEqual(response.status_code, 302)
+        self.assertTrue(response["location"].startswith("/accounts/login/"))
+
+        self.client.force_login(self.user)
+
+        # Make it a reporter who can't access
+        member = UserAdvertiserMember.objects.get(user=self.user, advertiser=self.advertiser)
+        member.role = UserAdvertiserMember.ROLE_REPORTER
+        member.save()
+
+        response = self.client.get(url)
+        self.assertEqual(response.status_code, 403)
+
+        member.role = UserAdvertiserMember.ROLE_MANAGER
+        member.save()
+
+        response = self.client.get(url)
+        self.assertEqual(response.status_code, 200)
+        self.assertContains(response, "Remove advertisement")
+
+        # Can remove ad1 - no impressions
+        response = self.client.post(url, data={}, follow=True)
+        self.assertEqual(response.status_code, 200)
+        self.assertContains(response, "Successfully removed advertisement")
+
+        self.assertFalse(
+            Advertisement.objects.filter(pk=self.ad1.pk).exists()
+        )
+
+    def test_ad_delete_view_fail(self):
+        url = reverse(
+            "advertisement_remove",
+            kwargs={
+                "advertiser_slug": self.advertiser.slug,
+                "flight_slug": self.flight.slug,
+                "advertisement_slug": self.ad1.slug,
+            },
+        )
+
+        self.client.force_login(self.user)
+        member = UserAdvertiserMember.objects.get(user=self.user, advertiser=self.advertiser)
+        member.role = UserAdvertiserMember.ROLE_MANAGER
+        member.save()
+
+        response = self.client.get(url)
+        self.assertEqual(response.status_code, 200)
+        self.assertContains(response, "Remove advertisement")
+
+        # Add an offer to ad1 so it can't be removed
+        request = self.factory.get("/")
+        self.ad1.offer_ad(
+            request=request,
+            publisher=self.publisher,
+            ad_type_slug=self.ad_type1,
+            div_id="foo",
+            keywords=None,
+        )
+
+        response = self.client.get(url)
+        self.assertEqual(response.status_code, 404)
+
+        # Can't remove ad1 with a POST either
+        response = self.client.post(url, data={})
+        self.assertEqual(response.status_code, 404)
+
+        self.assertTrue(
+            Advertisement.objects.filter(pk=self.ad1.pk).exists()
+        )
+
     def test_ad_bulk_create_view(self):
         url = reverse(
             "advertisement_bulk_create",

adserver/tests/test_models.py

@@ -28,10 +28,22 @@ class TestProtectedModels(BaseAdModelsTestCase):
     """Test that models extending IndestructibleModel can't be deleted"""
 
     def test_delete_model(self):
-        self.assertRaises(IntegrityError, self.ad1.delete)
+        self.ad1.delete()  # Shouldn't raise because ads without impressions can be deleted
         self.assertRaises(IntegrityError, self.campaign.delete)
         self.assertRaises(IntegrityError, self.flight.delete)
 
+        request = self.factory.get("/")
+        self.ad2.offer_ad(
+            request=request,
+            publisher=self.publisher,
+            ad_type_slug=self.text_ad_type,
+            div_id="foo",
+            keywords=None,
+        )
+
+        # Now that ad2 has an offer, it can't be deleted
+        self.assertRaises(IntegrityError, self.ad2.delete)
+
     def test_queryset(self):
         self.assertRaises(IntegrityError, Advertisement.objects.all().delete)
         self.assertRaises(IntegrityError, Flight.objects.all().delete)

adserver/urls.py

@@ -11,6 +11,7 @@
 from .views import AdvertisementCopyView
 from .views import AdvertisementCreateView
 from .views import AdvertisementDetailView
+from .views import AdvertisementRemoveView
 from .views import AdvertisementUpdateView
 from .views import AdvertiserAuthorizedUsersInviteView
 from .views import AdvertiserAuthorizedUsersRemoveView
@@ -270,6 +271,11 @@
         AdvertisementUpdateView.as_view(),
         name="advertisement_update",
     ),
+    path(
+        r"advertiser/<slug:advertiser_slug>/flights/<slug:flight_slug>/advertisements/<slug:advertisement_slug>/remove/",
+        AdvertisementRemoveView.as_view(),
+        name="advertisement_remove",
+    ),
     path(
         r"advertiser/<slug:advertiser_slug>/users/",
         AdvertiserAuthorizedUsersView.as_view(),

adserver/views.py

@@ -778,6 +778,63 @@ def get_success_url(self):
         )
 
 
+class AdvertisementRemoveView(
+    AdvertiserManagerAccessMixin, UserPassesTestMixin, DeleteView
+):
+    """View to delete an ad that has *never* been used."""
+
+    model = Advertisement
+    template_name = "adserver/advertiser/advertisement-delete.html"
+
+    def dispatch(self, request, *args, **kwargs):
+        self.advertiser = get_object_or_404(
+            Advertiser, slug=self.kwargs["advertiser_slug"]
+        )
+        self.flight = get_object_or_404(
+            Flight,
+            slug=self.kwargs["flight_slug"],
+            campaign__advertiser=self.advertiser,
+        )
+        self.advertisement = get_object_or_404(
+            Advertisement,
+            flight=self.flight,
+            slug=self.kwargs["advertisement_slug"],
+        )
+        return super().dispatch(request, *args, **kwargs)
+
+    def form_valid(self, form):
+        result = super().form_valid(form)
+        messages.info(self.request, _("Successfully removed advertisement"))
+        return result
+
+    def get_success_url(self):
+        return reverse(
+            "flight_detail",
+            kwargs={
+                "advertiser_slug": self.advertiser.slug,
+                "flight_slug": self.flight.slug,
+            },
+        )
+
+    def get_object(self, queryset=None):
+        # can_be_deleted checks if there are any impressions or clicks for this ad
+        if not self.advertisement or not self.advertisement.can_be_deleted():
+            log.warning("Ad isn't eligible for deletion: %s", self.advertisement)
+            raise Http404
+        return self.advertisement
+
+    def get_context_data(self, **kwargs):
+        context = super().get_context_data(**kwargs)
+        context.update(
+            {
+                "advertiser": self.advertiser,
+                "flight": self.flight,
+                "advertisement": self.advertisement,
+            }
+        )
+        return context
+
+
 class AdvertisementBulkCreateView(
     AdvertiserManagerAccessMixin,
     UserPassesTestMixin,