Merge pull request #1083 from readthedocs/davidfischer/improve-copy-ads

Improve the copy ads screen

Author: davidfischer

adserver/forms.py

@@ -1371,11 +1371,20 @@ def get_ads(self):
 class AdvertisementCopyForm(forms.Form):
     """Used by advertisers to re-use their ads."""
 
+    MAXIMUM_ADS = 100
+    DEFAULT_ORDER = "-modified"
+
     advertisements = AdvertisementMultipleChoiceField(
         queryset=Advertisement.objects.none(),
         required=False,
         help_text=_("Copy the following advertisements"),
     )
+    live_after_copy = forms.BooleanField(
+        required=False,
+        initial=False,
+        label=_("Make copied ads live"),
+        help_text=_("If checked, the copied ads will be live immediately"),
+    )
 
     def __init__(self, *args, **kwargs):
         """Add the form helper and customize the look of the form."""
@@ -1384,14 +1393,24 @@ def __init__(self, *args, **kwargs):
         else:
             raise RuntimeError("'flight' is required for the ad form")
 
+        if "order" in kwargs:
+            order = kwargs.pop("order")
+        else:
+            order = self.DEFAULT_ORDER
+
         super().__init__(*args, **kwargs)
 
+        # Use the passed order to sort the ads on the form
         self.fields["advertisements"].queryset = (
             Advertisement.objects.filter(flight__campaign=self.flight.campaign)
-            .order_by("-flight__start_date", "slug")
+            .order_by(order, "slug")
             .select_related()
             .prefetch_related("ad_types")
         )
+        # Limit the choices so the form isn't *too* long
+        self.fields["advertisements"].choices = self.fields["advertisements"].choices[
+            : self.MAXIMUM_ADS
+        ]
 
         self.helper = FormHelper()
         self.helper.attrs = {"id": "advertisements-copy"}
@@ -1403,6 +1422,10 @@ def __init__(self, *args, **kwargs):
                     "advertisements",
                     template="adserver/includes/widgets/advertisement-form-option.html",
                 ),
+                Fieldset(
+                    _("Options"),
+                    Field("live_after_copy"),
+                ),
                 css_class="my-3",
             ),
             Submit("submit", _("Copy existing ads")),
@@ -1413,6 +1436,8 @@ def save(self):
         for ad in self.cleaned_data["advertisements"]:
             new_ad = ad.__copy__()
             new_ad.flight = self.flight
+            if self.cleaned_data["live_after_copy"]:
+                new_ad.live = True
             new_ad.save()
 
         return len(self.cleaned_data["advertisements"])

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

@@ -29,6 +29,23 @@ <h1>{% block heading %}{% trans 'Copy advertisements' %}{% endblock heading %}</
   <div class="row">
 
     <div class="col-md-8">
+
+      <form method="get">
+        <div class="form-row">
+          <div class="col-4">
+            <select class="form-control form-control-sm" name="order">
+              {% for ordering, display in ordering_options %}
+                <option value="{{ ordering }}" {% if ordering == order %}selected{% endif %}>{{ display }}</option>
+              {% endfor %}
+            </select>
+          </div>
+
+          <div class="col-2">
+            <button class="btn btn-sm btn-outline-secondary" type="submit">{% trans 'Filter and order' %}</button>
+          </div>
+        </div> <!-- /form-row -->
+      </form>
+
       {% crispy form form.helper %}
     </div>
 

adserver/tests/test_advertiser_dashboard.py

@@ -984,11 +984,22 @@ def test_ad_copy_view(self):
         self.assertContains(response, "Re-use your previous ads")
         self.assertContains(response, self.ad1.name)
 
-        # Perform the copy
+        # Perform the copy - new ad isn't live by default
         count_ads = Advertisement.objects.all().count()
-        response = self.client.post(url, data={"advertisements": [self.ad1.pk]})
-        self.assertEqual(response.status_code, 302)
+        live_ads = Advertisement.objects.filter(live=True).count()
+        response = self.client.post(url, data={"advertisements": [self.ad1.pk]}, follow=True)
+        self.assertEqual(response.status_code, 200)
+        self.assertContains(response, "Successfully copied 1 ads to flight")
+        # Ads is +1 but live ads is the same
         self.assertEqual(Advertisement.objects.all().count(), count_ads + 1)
+        self.assertEqual(Advertisement.objects.filter(live=True).count(), live_ads)
+
+        # Copy again - new ad is live
+        live_ads = Advertisement.objects.filter(live=True).count()
+        response = self.client.post(url, data={"advertisements": [self.ad1.pk], "live_after_copy": True}, follow=True)
+        self.assertEqual(response.status_code, 200)
+        self.assertContains(response, "Successfully copied 1 ads to flight")
+        self.assertEqual(Advertisement.objects.filter(live=True).count(), live_ads + 1)
 
     def test_deprecated_ad_type(self):
         url = reverse(

adserver/views.py

@@ -929,6 +929,13 @@ class AdvertisementCopyView(
 ):
     """Create a copy of an existing ad."""
 
+    ORDERING_OPTIONS = (
+        ("-modified", "Most recent"),
+        ("-flight__start_date", "By flight start date"),
+        ("-sampled_ctr", "Top performing"),
+        ("name", "By name"),
+    )
+
     form_class = AdvertisementCopyForm
     model = Advertisement
     template_name = "adserver/advertiser/advertisement-copy.html"
@@ -963,15 +970,24 @@ def get_context_data(self, **kwargs):
             {
                 "advertiser": self.advertiser,
                 "flight": self.flight,
+                "order": self.get_order(),
+                "ordering_options": self.ORDERING_OPTIONS,
             }
         )
         return context
 
     def get_form_kwargs(self):
         kwargs = super().get_form_kwargs()
         kwargs["flight"] = self.flight
+        kwargs["order"] = self.get_order()
         return kwargs
 
+    def get_order(self):
+        order = self.request.GET.get("order")
+        if not order or order not in [o[0] for o in self.ORDERING_OPTIONS]:
+            order = self.ORDERING_OPTIONS[0][0]
+        return order
+
     def get_success_url(self):
         return reverse(
             "flight_detail",