Merge pull request #146 from LabD/simplifications+optimisations

Handles both Python 2 & 3, and multiple optimisations & simplifications.

src/wagtail_personalisation/adapters.py

@@ -23,28 +23,22 @@ class BaseSegmentsAdapter(object):
 
     def setup(self):
         """Prepare the adapter for segment storage."""
-        return None
 
     def get_segments(self):
         """Return the segments stored in the adapter storage."""
-        return None
 
     def get_segment_by_id(self):
         """Return a single segment stored in the adapter storage."""
-        return None
 
     def add(self):
         """Add a new segment to the adapter storage."""
-        return None
 
     def refresh(self):
         """Refresh the segments stored in the adapter storage."""
-        return None
 
     def _test_rules(self, rules, request, match_any=False):
         """Tests the provided rules to see if the request still belongs
         to a segment.
-
         :param rules: The rules to test for
         :type rules: list of wagtail_personalisation.rules
         :param request: The http request
@@ -53,20 +47,12 @@ class BaseSegmentsAdapter(object):
         :type match_any: bool
         :returns: A boolean indicating the segment matches the request
         :rtype: bool
-
         """
-        if len(rules) > 0:
+        if not rules:
-            for rule in rules:
+            return False
-                result = rule.test_user(request)
         if match_any:
-                    if result is True:
+            return any(rule.test_user(request) for rule in rules)
-                        return result
+        return all(rule.test_user(request) for rule in rules)
-
-                elif result is False:
-                    return False
-            if not match_any:
-                return True
-        return False
 
     class Meta:
         abstract = True
@@ -95,7 +81,7 @@ class SessionSegmentsAdapter(BaseSegmentsAdapter):
 
         segments = (
             Segment.objects
-            .filter(status=Segment.STATUS_ENABLED)
+            .enabled()
             .filter(persistent=True)
             .in_bulk(segment_ids))
 
@@ -134,8 +120,9 @@ class SessionSegmentsAdapter(BaseSegmentsAdapter):
         :rtype: wagtail_personalisation.models.Segment or None
 
         """
-        segments = self.get_segments()
+        for segment in self.get_segments():
-        return next((s for s in segments if s.pk == segment_id), None)
+            if segment.pk == segment_id:
+                return segment
 
     def add_page_visit(self, page):
         """Mark the page as visited by the user"""
@@ -179,20 +166,20 @@ class SessionSegmentsAdapter(BaseSegmentsAdapter):
         still apply to the requesting visitor.
 
         """
-        all_segments = Segment.objects.filter(status=Segment.STATUS_ENABLED)
+        enabled_segments = Segment.objects.enabled()
+        rule_models = AbstractBaseRule.get_descendant_models()
 
         current_segments = self.get_segments()
-        rules = AbstractBaseRule.__subclasses__()
 
         # Run tests on all remaining enabled segments to verify applicability.
         additional_segments = []
-        for segment in all_segments:
+        for segment in enabled_segments:
             segment_rules = []
-            for rule in rules:
+            for rule_model in rule_models:
-                segment_rules += rule.objects.filter(segment=segment)
+                segment_rules.extend(rule_model.objects.filter(segment=segment))
 
-            result = self._test_rules(
+            result = self._test_rules(segment_rules, self.request,
-                segment_rules, self.request, match_any=segment.match_any)
+                                      match_any=segment.match_any)
 
             if result:
                 additional_segments.append(segment)
@@ -209,8 +196,6 @@ SEGMENT_ADAPTER_CLASS = import_string(getattr(
 
 def get_segment_adapter(request):
     """Return the Segment Adapter for the given request"""
-    try:
+    if not hasattr(request, 'segment_adapter'):
-        return request.segment_adapter
-    except AttributeError:
         request.segment_adapter = SEGMENT_ADAPTER_CLASS(request)
     return request.segment_adapter

src/wagtail_personalisation/blocks.py

@@ -8,8 +8,8 @@ from wagtail_personalisation.models import Segment
 
 
 def list_segment_choices():
-    for segment in Segment.objects.all():
+    for pk, name in Segment.objects.values_list('pk', 'name'):
-        yield (segment.pk, segment.name)
+        yield pk, name
 
 
 class PersonalisedStructBlock(blocks.StructBlock):

src/wagtail_personalisation/models.py

@@ -61,9 +61,9 @@ class Segment(ClusterableModel):
             ], heading="Segment"),
             MultiFieldPanel([
                 InlinePanel(
-                    "{}_related".format(rule._meta.db_table),
+                    "{}_related".format(rule_model._meta.db_table),
-                    label=rule.__str__,
+                    label=rule_model._meta.verbose_name,
-                ) for rule in AbstractBaseRule.__subclasses__()
+                ) for rule_model in AbstractBaseRule.__subclasses__()
             ], heading=_("Rules")),
         ]
 
@@ -82,12 +82,19 @@ class Segment(ClusterableModel):
 
     def get_rules(self):
         """Retrieve all rules in the segment."""
-        rules = AbstractBaseRule.__subclasses__()
         segment_rules = []
-        for rule in rules:
+        for rule_model in AbstractBaseRule.get_descendant_models():
-            segment_rules += rule.objects.filter(segment=self)
+            segment_rules.extend(
+                rule_model._default_manager.filter(segment=self))
         return segment_rules
 
+    def toggle(self, save=True):
+        self.status = (
+            self.STATUS_ENABLED if self.status == self.STATUS_DISABLED
+            else self.STATUS_DISABLED)
+        if save:
+            self.save()
+
 
 class PersonalisablePageMixin(models.Model):
     """The personalisable page model. Allows creation of variants with linked
@@ -103,7 +110,7 @@ class PersonalisablePageMixin(models.Model):
         blank=True, null=True
     )
     segment = models.ForeignKey(
-        Segment, related_name='+', on_delete=models.PROTECT,
+        Segment, related_name='pages', on_delete=models.PROTECT,
         blank=True, null=True
     )
     is_segmented = models.BooleanField(default=False)
@@ -117,9 +124,6 @@ class PersonalisablePageMixin(models.Model):
 
     base_form_class = AdminPersonalisablePageForm
 
-    def __str__(self):
-        return "{}".format(self.title)
-
     @cached_property
     def has_variations(self):
         """Return a boolean indicating whether or not the personalisable page
@@ -137,12 +141,20 @@ class PersonalisablePageMixin(models.Model):
         """Return a boolean indicating whether or not the personalisable page
         is a canonical page.
 
-        :returns: A boolean indicating whether or not the personalisable page
+        :returns: A boolean indicating whether or not the personalisable
+        page
                   is a canonical page.
         :rtype: bool
 
         """
-        return not self.canonical_page and self.has_variations
+        return self.canonical_page_id is None
+
+    def get_unused_segments(self):
+        if not hasattr(self, '_unused_segments'):
+            self._unused_segments = (
+                Segment.objects.exclude(pages__canonical_page=self)
+                if self.is_canonical else Segment.objects.none())
+        return self._unused_segments
 
     def copy_for_segment(self, segment):
         slug = "{}-{}".format(self.slug, segment.encoded_name())

src/wagtail_personalisation/rules.py

@@ -3,9 +3,10 @@ from __future__ import absolute_import, unicode_literals
 import re
 from datetime import datetime
 
+from django.apps import apps
 from django.db import models
 from django.template.defaultfilters import slugify
-from django.utils.encoding import python_2_unicode_compatible
+from django.utils.encoding import python_2_unicode_compatible, force_text
 from django.utils.translation import ugettext_lazy as _
 from modelcluster.fields import ParentalKey
 from user_agents import parse
@@ -26,7 +27,7 @@ class AbstractBaseRule(models.Model):
         abstract = True
 
     def __str__(self):
-        return _('Abstract segmentation rule')
+        return force_text(self._meta.verbose_name)
 
     def test_user(self):
         """Test if the user matches this rule."""
@@ -34,7 +35,7 @@ class AbstractBaseRule(models.Model):
 
     def encoded_name(self):
         """Return a string with a slug for the rule."""
-        return slugify(self.__str__().lower())
+        return slugify(force_text(self).lower())
 
     def description(self):
         """Return a description explaining the functionality of the rule.
@@ -51,8 +52,16 @@ class AbstractBaseRule(models.Model):
 
         return description
 
+    @classmethod
+    def get_descendant_models(cls):
+        return [model for model in apps.get_models()
+                if issubclass(model, AbstractBaseRule)]
+
+    class Meta:
+        abstract = True
+        verbose_name = 'Abstract segmentation rule'
+
 
-@python_2_unicode_compatible
 class TimeRule(AbstractBaseRule):
     """Time rule to segment users based on a start and end time.
 
@@ -70,18 +79,14 @@ class TimeRule(AbstractBaseRule):
         ]),
     ]
 
-    def __str__(self):
+    class Meta:
-        return _('Time Rule')
+        verbose_name = _('Time Rule')
 
     def test_user(self, request=None):
-        current_time = datetime.now().time()
+        return self.start_time <= datetime.now().time() <= self.end_time
-        starting_time = self.start_time
-        ending_time = self.end_time
-
-        return starting_time <= current_time <= ending_time
 
     def description(self):
-        description = {
+        return {
             'title': _('These users visit between'),
             'value': _('{} and {}').format(
                 self.start_time.strftime("%H:%M"),
@@ -89,10 +94,7 @@ class TimeRule(AbstractBaseRule):
             ),
         }
 
-        return description
 
-
-@python_2_unicode_compatible
 class DayRule(AbstractBaseRule):
     """Day rule to segment users based on the day(s) of a visit.
 
@@ -118,39 +120,28 @@ class DayRule(AbstractBaseRule):
         FieldPanel('sun'),
     ]
 
-    def __str__(self):
+    class Meta:
-        return _('Day Rule')
+        verbose_name = _('Day Rule')
 
     def test_user(self, request=None):
-        current_day = datetime.today().weekday()
+        return [self.mon, self.tue, self.wed, self.thu,
-
+                self.fri, self.sat, self.sun][datetime.today().weekday()]
-        days = [self.mon, self.tue, self.wed, self.thu,
-                self.fri, self.sat, self.sun]
-
-        return days[current_day]
 
     def description(self):
-        days = {
+        days = (
-            'mon': self.mon, 'tue': self.tue, 'wed': self.wed,
+            ('mon', self.mon), ('tue', self.tue), ('wed', self.wed),
-            'thu': self.thu, 'fri': self.fri, 'sat': self.sat,
+            ('thu', self.thu), ('fri', self.fri), ('sat', self.sat),
-            'sun': self.sun
+            ('sun', self.sun),
-        }
+        )
 
-        chosen_days = []
+        chosen_days = [day_name for day_name, chosen in days if chosen]
 
-        for key, value in days.items():
+        return {
-            if days[key]:
-                chosen_days.append(key)
-
-        description = {
             'title': _('These users visit on'),
-            'value': (', '.join(_(day) for day in chosen_days)).title()
+            'value': ", ".join([day for day in chosen_days]).title(),
         }
 
-        return description
 
-
-@python_2_unicode_compatible
 class ReferralRule(AbstractBaseRule):
     """Referral rule to segment users based on a regex test.
 
@@ -165,8 +156,8 @@ class ReferralRule(AbstractBaseRule):
         FieldPanel('regex_string'),
     ]
 
-    def __str__(self):
+    class Meta:
-        return _('Referral Rule')
+        verbose_name = _('Referral Rule')
 
     def test_user(self, request):
         pattern = re.compile(self.regex_string)
@@ -178,18 +169,13 @@ class ReferralRule(AbstractBaseRule):
         return False
 
     def description(self):
-        description = {
+        return {
             'title': _('These visits originate from'),
-            'value': _('{}').format(
+            'value': self.regex_string,
-                self.regex_string
-            ),
             'code': True
         }
 
-        return description
 
-
-@python_2_unicode_compatible
 class VisitCountRule(AbstractBaseRule):
     """Visit count rule to segment users based on amount of visits to a
     specified page.
@@ -222,6 +208,9 @@ class VisitCountRule(AbstractBaseRule):
         ]),
     ]
 
+    class Meta:
+        verbose_name = _('Visit count Rule')
+
     def test_user(self, request):
         operator = self.operator
         segment_count = self.count
@@ -243,11 +232,8 @@ class VisitCountRule(AbstractBaseRule):
                 return True
         return False
 
-    def __str__(self):
-        return _('Visit count Rule')
-
     def description(self):
-        description = {
+        return {
             'title': _('These users visited {}').format(
                 self.counted_page
             ),
@@ -257,10 +243,7 @@ class VisitCountRule(AbstractBaseRule):
             ),
         }
 
-        return description
 
-
-@python_2_unicode_compatible
 class QueryRule(AbstractBaseRule):
     """Query rule to segment users based on matching queries.
 
@@ -278,18 +261,14 @@ class QueryRule(AbstractBaseRule):
         FieldPanel('value'),
     ]
 
-    def __str__(self):
+    class Meta:
-        return _('Query Rule')
+        verbose_name = _('Query Rule')
 
     def test_user(self, request):
-        parameter = self.parameter
+        return request.GET.get(self.parameter, '') == self.value
-        value = self.value
-
-        req_value = request.GET.get(parameter, '')
-        return req_value == value
 
     def description(self):
-        description = {
+        return {
             'title': _('These users used a URL with the query'),
             'value': _('?{}={}').format(
                 self.parameter,
@@ -298,10 +277,7 @@ class QueryRule(AbstractBaseRule):
             'code': True
         }
 
-        return description
 
-
-@python_2_unicode_compatible
 class DeviceRule(AbstractBaseRule):
     """Device rule to segment users based on matching devices.
 
@@ -319,8 +295,8 @@ class DeviceRule(AbstractBaseRule):
         FieldPanel('desktop'),
     ]
 
-    def __str__(self):
+    class Meta:
-        return _('Device Rule')
+        verbose_name = _('Device Rule')
 
     def test_user(self, request=None):
         ua_header = request.META['HTTP_USER_AGENT']
@@ -328,15 +304,13 @@ class DeviceRule(AbstractBaseRule):
 
         if user_agent.is_mobile:
             return self.mobile
-        elif user_agent.is_tablet:
+        if user_agent.is_tablet:
             return self.tablet
-        elif user_agent.is_pc:
+        if user_agent.is_pc:
             return self.desktop
-        else:
         return False
 
 
-@python_2_unicode_compatible
 class UserIsLoggedInRule(AbstractBaseRule):
     """User is logged in rule to segment users based on their authentication
     status.
@@ -350,22 +324,14 @@ class UserIsLoggedInRule(AbstractBaseRule):
         FieldPanel('is_logged_in'),
     ]
 
-    def __str__(self):
+    class Meta:
-        return _('Logged in Rule')
+        verbose_name = _('Logged in Rule')
 
     def test_user(self, request=None):
         return request.user.is_authenticated() == self.is_logged_in
 
     def description(self):
-        status = _('Logged in')
+        return {
-        if self.is_logged_in is False:
-            status = _('Not logged in')
-
-        description = {
             'title': _('These visitors are'),
-            'value': _('{}').format(
+            'value': _('Logged in') if self.is_logged_in else _('Not logged in'),
-                status
-            ),
         }
-
-        return description

src/wagtail_personalisation/views.py

@@ -98,12 +98,7 @@ def toggle(request, segment_id):
     if request.user.has_perm('wagtailadmin.access_admin'):
         segment = get_object_or_404(Segment, pk=segment_id)
 
-        if segment.status == Segment.STATUS_ENABLED:
+        segment.toggle()
-            segment.status = Segment.STATUS_DISABLED
-        elif segment.status == Segment.STATUS_DISABLED:
-            segment.status = Segment.STATUS_ENABLED
-
-        segment.save()
 
         return HttpResponseRedirect(request.META.get('HTTP_REFERER', '/'))
 

src/wagtail_personalisation/wagtail_hooks.py

@@ -3,13 +3,12 @@ from __future__ import absolute_import, unicode_literals
 import logging
 
 from django.conf.urls import include, url
-from django.shortcuts import reverse
+from django.core.urlresolvers import reverse
 from django.utils.safestring import mark_safe
 from django.utils.translation import ugettext_lazy as _
 from wagtail.wagtailadmin.site_summary import SummaryItem
 from wagtail.wagtailadmin.widgets import Button, ButtonWithDropdownFromHook
 from wagtail.wagtailcore import hooks
-from wagtail.wagtailcore.models import Page
 
 from wagtail_personalisation import admin_urls
 from wagtail_personalisation.adapters import get_segment_adapter
@@ -72,12 +71,11 @@ def serve_variation(page, request, serve_args, serve_kwargs):
     :rtype: wagtail.wagtailcore.models.Page
 
     """
-    user_segments = []
     adapter = get_segment_adapter(request)
     user_segments = adapter.get_segments()
 
-    if len(user_segments) > 0:
+    if user_segments:
-        variations = _check_for_variations(user_segments, page)
+        variations = page.variants_for_segments(user_segments)
 
         if variations:
             variation = variations[0]
@@ -87,36 +85,13 @@ def serve_variation(page, request, serve_args, serve_kwargs):
             return variation.serve(request, *serve_args, **serve_kwargs)
 
 
-def _check_for_variations(segments, page):
-    """Check whether there are variations available for the provided segments
-    on the page being served.
-
-    :param segments: The segments applicable to the request.
-    :type segments: list of wagtail_personalisation.models.Segment
-    :param page: The page being served
-    :type page: wagtail_personalisation.models.PersonalisablePage or
-                wagtail.wagtailcore.models.Page
-    :returns: A variant of the requested page matching the segments or None
-    :rtype: wagtail_personalisation.models.PersonalisablePage or None
-
-    """
-    return page.variants_for_segments(segments)
-
-
 @hooks.register('register_page_listing_buttons')
 def page_listing_variant_buttons(page, page_perms, is_parent=False):
     """Adds page listing buttons to personalisable pages. Shows variants for
     the page (if any) and a 'Create a new variant' button.
 
     """
-
+    if isinstance(page, PersonalisablePageMixin) and page.get_unused_segments():
-    if not hasattr(page, 'segment'):
-        return
-    pages = page.__class__.objects.filter(pk=page.pk)
-    segments = Segment.objects.all()
-
-    if pages and len(segments) > 0 and not (
-            any(item.segment for item in pages)):
         yield ButtonWithDropdownFromHook(
             _('Variants'),
             hook_name='register_page_listing_variant_buttons',
@@ -133,16 +108,9 @@ def page_listing_more_buttons(page, page_perms, is_parent=False):
     create a new variant for the selected segment.
 
     """
-    model = page.__class__
+    for segment in page.get_unused_segments():
-    segments = Segment.objects.all()
-    available_segments = [
-        item for item in segments
-        if not model.objects.filter(segment=item, pk=page.pk)
-    ]
-
-    for segment in available_segments:
         yield Button(segment.name,
-                     reverse('segment:copy_page', args=[page.id, segment.id]),
+                     reverse('segment:copy_page', args=[page.pk, segment.pk]),
                      attrs={"title": _('Create this variant')})
 
 
@@ -166,7 +134,8 @@ class SegmentSummaryPanel(SummaryItem):
 @hooks.register('construct_homepage_summary_items')
 def add_segment_summary_panel(request, items):
     """Adds a summary panel to the Wagtail dashboard showing the total amount
-    of segments on the site and allowing quick access to the Segment dashboard.
+    of segments on the site and allowing quick access to the Segment
+    dashboard.
 
     """
-    return items.append(SegmentSummaryPanel(request))
+    items.append(SegmentSummaryPanel(request))

tests/unit/test_hooks.py

@@ -1,13 +1,12 @@
 import pytest
 
-from wagtail_personalisation import wagtail_hooks as hooks
 from wagtail_personalisation.models import Segment
 
 
 @pytest.mark.django_db
-def test_check_for_variations(segmented_page):
+def test_variants(segmented_page):
     segments = Segment.objects.all()
     page = segmented_page.canonical_page
 
-    variations = hooks._check_for_variations(segments, page)
+    variations = page.variants_for_segments(segments)
     assert variations