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

Improve copying ads logic

adserver/forms.py

@@ -805,6 +805,57 @@ class Meta:
         }
 
 
+class AdvertisementCopyForm(forms.Form):
+
+    """Used by advertisers to re-use their ads."""
+
+    advertisements = AdvertisementMultipleChoiceField(
+        queryset=Advertisement.objects.none(),
+        required=False,
+        help_text=_("Copy the following advertisements"),
+    )
+
+    def __init__(self, *args, **kwargs):
+        """Add the form helper and customize the look of the form."""
+        if "flight" in kwargs:
+            self.flight = kwargs.pop("flight")
+        else:
+            raise RuntimeError("'flight' is required for the ad form")
+
+        super().__init__(*args, **kwargs)
+
+        self.fields["advertisements"].queryset = (
+            Advertisement.objects.filter(flight__campaign=self.flight.campaign)
+            .order_by("-flight__start_date", "slug")
+            .select_related()
+            .prefetch_related("ad_types")
+        )
+
+        self.helper = FormHelper()
+        self.helper.attrs = {"id": "advertisements-copy"}
+
+        self.helper.layout = Layout(
+            Fieldset(
+                "",
+                Field(
+                    "advertisements",
+                    template="adserver/includes/widgets/advertisement-form-option.html",
+                ),
+                css_class="my-3",
+            ),
+            Submit("submit", _("Copy existing ads")),
+        )
+
+    def save(self):
+        # Duplicate the advertisements into the new flight
+        for ad in self.cleaned_data["advertisements"]:
+            new_ad = ad.__copy__()
+            new_ad.flight = self.flight
+            new_ad.save()
+
+        return len(self.cleaned_data["advertisements"])
+
+
 class PublisherSettingsForm(forms.ModelForm):
 
     """Form for letting publishers control publisher specific settings."""

adserver/models.py

@@ -3,6 +3,7 @@
 import html
 import logging
 import math
+import re
 import uuid
 from collections import Counter
 
@@ -23,7 +24,6 @@
 from django.templatetags.static import static
 from django.urls import reverse
 from django.utils import timezone
-from django.utils.crypto import get_random_string
 from django.utils.functional import cached_property
 from django.utils.html import mark_safe
 from django.utils.text import slugify
@@ -1421,15 +1421,32 @@ def __copy__(self):
         # Get a fresh reference so that "self" doesn't become the new copy
         ad = Advertisement.objects.get(pk=self.pk)
 
+        new_name = ad.name
+        new_slug = ad.slug
+
+        # Fix up names/slugs of ads that have been copied before
+        # Remove dates and (" Copy") from the end of the name/slug
+        new_name = re.sub(" \d{4}-\d{2}-\d{2}$", "", new_name)
+        while new_name.endswith(" Copy"):
+            new_name = new_name[:-5]
+        new_slug = re.sub("-copy\d*$", "", new_slug)
+        new_slug = re.sub("-\d{8}(-\d+)?$", "", new_slug)
+
         # Get a slug that doesn't already exist
-        new_slug = ad.slug + "-copy"
+        # This tries -20230501, then -20230501-1, etc.
+        new_slug += "-{}".format(timezone.now().strftime("%Y%m%d"))
+        digit = 0
         while Advertisement.objects.filter(slug=new_slug).exists():
-            new_slug += "-" + get_random_string(3)
+            ending = f"-{digit}"
+            if new_slug.endswith(ending):
+                new_slug = new_slug[: -len(ending)]
+            digit += 1
+            new_slug += f"-{digit}"
 
         ad_types = ad.ad_types.all()
 
         ad.pk = None
-        ad.name += " Copy"
+        ad.name = new_name + " {}".format(timezone.now().strftime("%Y-%m-%d"))
         ad.slug = new_slug
         ad.live = False  # The new ad should always be non-live
         ad.save()

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

@@ -5,7 +5,7 @@
 {% load crispy_forms_tags %}
 
 
-{% block title %}{% trans 'Copy advertisement' %}{% endblock %}
+{% block title %}{% trans 'Copy advertisements' %}{% endblock %}
 
 
 {% block breadcrumbs %}
@@ -22,89 +22,17 @@
 
 <section>
 
-  <h1>{% block heading %}{% trans 'Copy advertisement' %}{% endblock heading %}</h1>
-  <p>{% blocktrans with flight_name=flight.name %}Re-use one of your previous ads in "{{ flight_name }}".{% endblocktrans %}</p>
-
-  {% if source_advertisement %}
-    {# Confirm the copy to this source #}
-    <div class="row">
-      {# Show the ad #}
-      <div class="col">
-        {% with advertisement=source_advertisement ad_type=source_advertisement.ad_types.first %}
-          {% include "adserver/includes/ad-preview.html" %}
-        {% endwith %}
-      </div>
-
-      {# Ad metadata #}
-      <div class="col">
-        <dl>
-          <dt>{% trans 'Source advertisement' %}</dt>
-          <dd>{{ source_advertisement.name }}</dd>
-          <dt>{% trans 'Source flight' %}</dt>
-          <dd>{{ source_advertisement.flight.name }}</dd>
-          <dt>{% trans 'Display' %}</dt>
-          <dd>{{ source_advertisement.ad_types.all | join:"<br>" }}</dd>
-          {% if source_advertisement.link %}
-            <dt>{% trans 'Click-through link' %}</dt>
-            <dd><small><a href="{{ source_advertisement.link }}">{{ source_advertisement.link|truncatechars:50 }}</a></small></dd>
-          {% endif %}
-        </dl>
-      </div>
+  <h1>{% block heading %}{% trans 'Copy advertisements' %}{% endblock heading %}</h1>
+  <p>{% blocktrans with flight_name=flight.name %}Re-use your previous ads in "{{ flight_name }}".{% endblocktrans %}</p>
+
+
+  <div class="row">
+
+    <div class="col-md-8">
+      {% crispy form form.helper %}
     </div>
 
-    <form method="post">
-      {% csrf_token %}
-      <input type="hidden" name="source_advertisement" value="{{ source_advertisement.pk }}">
-      <input type="Submit" class="btn btn-primary" value="{% trans 'Copy advertisement' %}">
-      <p class="form-text text-muted small">{% trans 'You will have a chance to edit the copy before it goes live.' %}</p>
-    </form>
-  {% elif advertisements %}
-    {# Choose an ad to copy #}
-    <form method="get">
-      <div class="table-responsive">
-        <table class="table">
-          <thead>
-            <tr>
-              <th><!-- Intentionally blank --></th>
-              <th><strong>{% trans 'Name' %}</strong></th>
-              <th><strong>{% trans 'Flight' %}</strong></th>
-              <th><strong>{% trans 'Preview' %}</strong></th>
-              <th><strong>{% trans 'Ad types' %}</strong></th>
-              <th><strong>{% blocktrans %}<abbr title="Click through rate">CTR</abbr>{% endblocktrans %}</strong></th>
-            </tr>
-          </thead>
-          <tbody>
-            {% for advertisement in advertisements %}
-              <tr>
-                <td>
-                  <div class="form-check">
-                    <input class="form-check-input" type="radio" name="source_advertisement" value="{{ advertisement.pk }}">
-                  </div>
-                </td>
-                <td>{{ advertisement.name }}</td>
-                <td>{{ advertisement.flight.name }}</td>
-                <td>
-                  <details>
-                    <summary>Preview</summary>
-                    {% with ad_type=advertisement.ad_types.first %}
-                      {% include "adserver/includes/ad-preview.html" %}
-                    {% endwith %}
-                  </details>
-                </td>
-                <td>{{ advertisement.ad_types.all | join:"<br>" }}</td>
-                <td>{{ advertisement.ctr|floatformat:3 }}%</td>
-              </tr>
-            {% endfor %}
-          </tbody>
-        </table>
-      </div>
-
-      <input type="Submit" class="btn btn-primary" value="{% trans 'Review & copy advertisement' %}">
-      <p class="form-text text-muted small">{% trans 'You will have a chance to review before copying.' %}</p>
-    </form>
-  {% else %}
-    <p>{% trans 'You have no advertisements to copy.' %}</p>
-  {% endif %}
+  </div>
 
 </section>
 

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

@@ -54,7 +54,7 @@ <h5 class="col-md-8">{% trans 'Live ads' %}</h5>
       </a>
       <a href="{% url 'advertisement_copy' advertiser.slug flight.slug %}" class="btn btn-sm btn-outline-primary mb-3" role="button" aria-pressed="true">
         <span class="fa fa-clone mr-1" aria-hidden="true"></span>
-        <span>{% trans 'Copy existing ad' %}</span>
+        <span>{% trans 'Copy existing ads' %}</span>
       </a>
     </aside>
   </div>

adserver/templates/adserver/includes/widgets/advertisement-form-option.html

@@ -45,6 +45,6 @@
       {% endif %}
     </div>
   {% empty %}
-    <div>{% trans 'No advertisements to renew' %}</div>
+    <div>{% trans 'No advertisements to re-use' %}</div>
   {% endfor %}
 </div>

adserver/tests/test_advertiser_dashboard.py

@@ -581,17 +581,12 @@ def test_ad_copy_view(self):
 
         self.client.force_login(self.user)
         response = self.client.get(url)
-        self.assertContains(response, "Copy advertisement")
+        self.assertContains(response, "Re-use your previous ads")
         self.assertContains(response, self.ad1.name)
 
-        # Goes to the confirm screen
-        response = self.client.get(url, data={"source_advertisement": self.ad1.pk})
-        self.assertContains(response, "Copy advertisement")
-        self.assertContains(response, "Source advertisement")
-
         # Perform the copy
         count_ads = Advertisement.objects.all().count()
-        response = self.client.post(url, data={"source_advertisement": self.ad1.pk})
+        response = self.client.post(url, data={"advertisements": [self.ad1.pk]})
         self.assertEqual(response.status_code, 302)
         self.assertEqual(Advertisement.objects.all().count(), count_ads + 1)
 

adserver/views.py

@@ -53,6 +53,7 @@
 from .constants import PUBLISHER_HOUSE_CAMPAIGN
 from .constants import VIEWS
 from .forms import AccountForm
+from .forms import AdvertisementCopyForm
 from .forms import AdvertisementForm
 from .forms import FlightCreateForm
 from .forms import FlightForm
@@ -584,10 +585,12 @@ def get_success_url(self):
         )
 
 
-class AdvertisementCopyView(AdvertiserAccessMixin, UserPassesTestMixin, TemplateView):
+class AdvertisementCopyView(AdvertiserAccessMixin, UserPassesTestMixin, FormView):
 
     """Create a copy of an existing ad."""
 
+    form_class = AdvertisementCopyForm
+    model = Advertisement
     template_name = "adserver/advertiser/advertisement-copy.html"
 
     def dispatch(self, request, *args, **kwargs):
@@ -599,80 +602,45 @@ def dispatch(self, request, *args, **kwargs):
             slug=self.kwargs["flight_slug"],
             campaign__advertiser=self.advertiser,
         )
-        self.source_advertisement = source_ad_id = None
-
-        if (
-            "source_advertisement" in request.GET
-            and request.GET["source_advertisement"].isdigit()
-        ):
-            source_ad_id = request.GET["source_advertisement"]
-        elif (
-            "source_advertisement" in request.POST
-            and request.POST["source_advertisement"].isdigit()
-        ):
-            source_ad_id = request.POST["source_advertisement"]
-
-        if source_ad_id:
-            self.source_advertisement = (
-                Advertisement.objects.filter(
-                    flight__campaign__advertiser=self.advertiser
-                )
-                .filter(pk=source_ad_id)
-                .first()
-            )
         return super().dispatch(request, *args, **kwargs)
 
-    def post(self, request, *args, **kwargs):
-        if self.flight and self.source_advertisement:
-            new_ad = self.copy_instance()
-            messages.success(
-                self.request,
-                _("Successfully created %(ad)s from a copy.") % {"ad": new_ad},
-            )
-            return redirect(
-                reverse(
-                    "advertisement_update",
-                    kwargs={
-                        "advertiser_slug": self.advertiser.slug,
-                        "flight_slug": self.flight.slug,
-                        "advertisement_slug": new_ad.slug,
-                    },
-                )
-            )
+    def form_valid(self, form):
+        result = super().form_valid(form)
 
-        # This should basically never be taken
-        return redirect(
-            reverse(
-                "flight_detail",
-                kwargs={
-                    "advertiser_slug": self.advertiser.slug,
-                    "flight_slug": self.flight.slug,
-                },
-            )
-        )
+        # Actually copy the ads
+        ads_copied = form.save()
 
-    def copy_instance(self):
-        instance = self.source_advertisement.__copy__()
-        instance.flight = self.flight
-        instance.save()  # Automatically gets a new slug
-        return instance
+        messages.success(
+            self.request,
+            _("Successfully copied %(ads_copied)s ads to flight '%(flight)s'")
+            % {"ads_copied": ads_copied, "flight": self.flight},
+        )
+        return result
 
     def get_context_data(self, **kwargs):
         context = super().get_context_data(**kwargs)
         context.update(
             {
                 "advertiser": self.advertiser,
                 "flight": self.flight,
-                "advertisements": Advertisement.objects.filter(
-                    flight__campaign__advertiser=self.advertiser
-                )
-                .order_by("-created")
-                .select_related(),
-                "source_advertisement": self.source_advertisement,
             }
         )
         return context
 
+    def get_form_kwargs(self):
+        kwargs = super().get_form_kwargs()
+        kwargs["flight"] = self.flight
+        return kwargs
+
+    def get_success_url(self):
+        return reverse(
+            "flight_detail",
+            kwargs={
+                "advertiser_slug": self.advertiser.slug,
+                "flight_slug": self.flight.slug,
+            },
+        )
+
 
 class BaseProxyView(View):