Merge branch 'main' into rel

Author: davidfischer

CHANGELOG.rst

@@ -6,6 +6,22 @@ CHANGELOG
 .. This is included by docs/developer/changelog.rst
 
 
+Version v5.28.0
+---------------
+
+This release contains an improvement to the copy ads screen that includes filtering and sorting
+as well as other options. It also turns on a few Postgres and Redis performance improvements
+notably using connection pooling on Postgres.
+
+:Date: October 22, 2025
+
+ * @davidfischer: Improve the copy ads screen (#1083)
+ * @davidfischer: Fix a bug with niche URL handling (#1082)
+ * @davidfischer: Use Hiredis client for performance (#1081)
+ * @davidfischer: Upgrade psycopg and use connection pools (#1080)
+ * @davidfischer: Handle if clients send x-forwarded-for (#1079)
+
+
 Version v5.27.0
 ---------------
 

adserver/analyzer/utils.py

@@ -30,6 +30,7 @@ def normalize_url(url):
     Currently, this means:
     - Removing ignored query paramters
     """
+    url = url.strip()
     parts = urlparse.urlparse(url)
 
     query_params = urlparse.parse_qs(parts.query, keep_blank_values=True)

adserver/api/mixins.py

@@ -31,7 +31,14 @@ def initial(self, request, *args, **kwargs):
 
         # Get the actual client IP address and UA (the user who will view the ad)
         if self.ip_field in request.data and request.data[self.ip_field]:
-            request.ip_address = request.data[self.ip_field]
+            ip = request.data[self.ip_field]
+            if ip and "," in ip:
+                # If there are multiple IPs, take the first one
+                # The client has probably sent the X-Forwarded-For header
+                # https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/X-Forwarded-For
+                ip = ip.split(",")[0].strip()
+
+            request.ip_address = ip
 
             # Geolocate the actual IP address (not the requestor's IP)
             # This is needed for the case of a server requesting ads on a user's behalf

adserver/api/serializers.py

@@ -4,6 +4,7 @@
 
 from django.core.exceptions import ValidationError
 from django.core.validators import URLValidator
+from django.core.validators import validate_ipv46_address
 from django.utils.translation import gettext_lazy as _
 from rest_framework import serializers
 
@@ -73,7 +74,9 @@ class AdDecisionSerializer(serializers.Serializer):
     )
 
     # Used to pass the actual ad viewer's data for targeting purposes
-    user_ip = serializers.IPAddressField(required=False)
+    # The IP field purposefully isn't an IPAddressField so we can handle some invalid values
+    # instead of rejecting them outright
+    user_ip = serializers.CharField(required=False)
     user_ua = serializers.CharField(required=False)
 
     # Used to specify a specific ad or campaign to show (used for debugging mostly)
@@ -124,6 +127,22 @@ def validate_url(self, url):
             log.warning("Invalid ad decision referring URL: %s", url)
             return None
 
+    def validate_user_ip(self, ip):
+        if ip and "," in ip:
+            # If there are multiple IPs, take the first one
+            # The client has probably sent the X-Forwarded-For header
+            # https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/X-Forwarded-For
+            ip = ip.split(",")[0].strip()
+
+        # Perform basic validation of the IP address (after splitting if necessary)
+        try:
+            validate_ipv46_address(ip)
+        except ValidationError:
+            log.warning("Invalid ad decision user IP address: %s", ip)
+            raise serializers.ValidationError("Invalid IPv4 or IPv6 address")
+
+        return ip
+
 
 class PublisherSerializer(serializers.HyperlinkedModelSerializer):
     class Meta:

adserver/forms.py

@@ -346,7 +346,7 @@ def clean_niche_urls(self):
                     raise forms.ValidationError(
                         _("'%(url)s' is an invalid URL."), params={"url": url}
                     )
-        return data
+        return "\n".join(niche_urls)
 
     def save(self, commit=True):
         if not self.instance.targeting_parameters:
@@ -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

@@ -408,7 +408,8 @@ def test_flight_update_view(self):
             "include_keywords": "python, django",
             "niche_distance": 0.5,
             "niche_urls": "\n".join([
-                "https://example.com/niche1/",
+                # Make sure comments are stripped properly
+                "https://example.com/niche1/ ",
                 "https://example.com/niche2/",
             ]),
         }
@@ -983,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/tests/test_api.py

@@ -1304,6 +1304,44 @@ def test_user_geoip_passed_ip(self):
         self.assertEqual(resp["X-Adserver-Country"], "US")
         self.assertEqual(resp["X-Adserver-Region"], "NY")
 
+    def test_multiple_passed_ip(self):
+        first_ip = "255.255.255.255"
+        new_ip = f"{first_ip},1.1.1.1,8.8.8.8"
+        data = {
+            "placements": self.placements,
+            "publisher": self.publisher1.slug,
+            "url": self.page_url,
+            "user_ip": new_ip,
+            "user_ua": self.user_agent,
+        }
+        with mock.patch("adserver.utils.get_geoipdb_geolocation") as get_geo:
+            get_geo.return_value = GeolocationData("US", "NY")
+
+            resp = self.client.post(
+                self.url, json.dumps(data), content_type="application/json"
+            )
+        self.assertEqual(resp.status_code, 200, resp.content)
+
+        # Check that the first IP was used
+        self.assertEqual(resp["X-Adserver-RealIP"], first_ip)
+
+    def test_invalid_ip(self):
+        ip = "255.255.256.255"
+        data = {
+            "placements": self.placements,
+            "publisher": self.publisher1.slug,
+            "url": self.page_url,
+            "user_ip": ip,
+            "user_ua": self.user_agent,
+        }
+        with mock.patch("adserver.utils.get_geoipdb_geolocation") as get_geo:
+            get_geo.return_value = GeolocationData(None, None)
+
+            resp = self.client.post(
+                self.url, json.dumps(data), content_type="application/json"
+            )
+        self.assertEqual(resp.status_code, 400)
+
     @override_settings(ADSERVER_RECORD_VIEWS=False)
     def test_record_views_false(self):
         self.publisher1.record_views = False

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",

config/settings/base.py

@@ -142,6 +142,7 @@
         default=f"sqlite:///{DB_PATH_SQLITE}",
     )
 }
+# https://docs.djangoproject.com/en/dev/topics/db/transactions/#tying-transactions-to-http-requests
 DATABASES["default"]["ATOMIC_REQUESTS"] = True
 DEFAULT_AUTO_FIELD = "django.db.models.AutoField"