diff --git a/src/wagtail_personalisation/adapters.py b/src/wagtail_personalisation/adapters.py index 0225ced..933f744 100644 --- a/src/wagtail_personalisation/adapters.py +++ b/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: - for rule in rules: - result = rule.test_user(request) - if match_any: - if result is True: - return result - - elif result is False: - return False - if not match_any: - return True - return False + if not rules: + return False + if match_any: + return any(rule.test_user(request) for rule in rules) + return all(rule.test_user(request) for rule in rules) 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() - return next((s for s in segments if s.pk == segment_id), None) + for segment in self.get_segments(): + 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: - segment_rules += rule.objects.filter(segment=segment) + for rule_model in rule_models: + segment_rules.extend(rule_model.objects.filter(segment=segment)) - result = self._test_rules( - segment_rules, self.request, match_any=segment.match_any) + result = self._test_rules(segment_rules, self.request, + 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: - return request.segment_adapter - except AttributeError: + if not hasattr(request, 'segment_adapter'): request.segment_adapter = SEGMENT_ADAPTER_CLASS(request) - return request.segment_adapter + return request.segment_adapter diff --git a/src/wagtail_personalisation/blocks.py b/src/wagtail_personalisation/blocks.py index b3ad67c..6aaa1fe 100644 --- a/src/wagtail_personalisation/blocks.py +++ b/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(): - yield (segment.pk, segment.name) + for pk, name in Segment.objects.values_list('pk', 'name'): + yield pk, name class PersonalisedStructBlock(blocks.StructBlock): diff --git a/src/wagtail_personalisation/models.py b/src/wagtail_personalisation/models.py index 04554c0..6e5f1b4 100644 --- a/src/wagtail_personalisation/models.py +++ b/src/wagtail_personalisation/models.py @@ -61,9 +61,9 @@ class Segment(ClusterableModel): ], heading="Segment"), MultiFieldPanel([ InlinePanel( - "{}_related".format(rule._meta.db_table), - label=rule.__str__, - ) for rule in AbstractBaseRule.__subclasses__() + "{}_related".format(rule_model._meta.db_table), + label=rule_model._meta.verbose_name, + ) 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: - segment_rules += rule.objects.filter(segment=self) + for rule_model in AbstractBaseRule.get_descendant_models(): + 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()) diff --git a/src/wagtail_personalisation/rules.py b/src/wagtail_personalisation/rules.py index c6f1ee7..c5af0e0 100644 --- a/src/wagtail_personalisation/rules.py +++ b/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): - return _('Time Rule') + class Meta: + verbose_name = _('Time Rule') def test_user(self, request=None): - current_time = datetime.now().time() - starting_time = self.start_time - ending_time = self.end_time - - return starting_time <= current_time <= ending_time + return self.start_time <= datetime.now().time() <= self.end_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): - return _('Day Rule') + class Meta: + verbose_name = _('Day Rule') def test_user(self, request=None): - current_day = datetime.today().weekday() - - days = [self.mon, self.tue, self.wed, self.thu, - self.fri, self.sat, self.sun] - - return days[current_day] + return [self.mon, self.tue, self.wed, self.thu, + self.fri, self.sat, self.sun][datetime.today().weekday()] def description(self): - days = { - 'mon': self.mon, 'tue': self.tue, 'wed': self.wed, - 'thu': self.thu, 'fri': self.fri, 'sat': self.sat, - 'sun': self.sun - } + days = ( + ('mon', self.mon), ('tue', self.tue), ('wed', self.wed), + ('thu', self.thu), ('fri', self.fri), ('sat', self.sat), + ('sun', self.sun), + ) - chosen_days = [] + chosen_days = [day_name for day_name, chosen in days if chosen] - for key, value in days.items(): - if days[key]: - chosen_days.append(key) - - description = { + return { '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): - return _('Referral Rule') + class Meta: + 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( - self.regex_string - ), + 'value': 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): - return _('Query Rule') + class Meta: + verbose_name = _('Query Rule') def test_user(self, request): - parameter = self.parameter - value = self.value - - req_value = request.GET.get(parameter, '') - return req_value == value + return request.GET.get(self.parameter, '') == self.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): - return _('Device Rule') + class Meta: + 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 + 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): - return _('Logged in Rule') + class Meta: + 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') - if self.is_logged_in is False: - status = _('Not logged in') - - description = { + return { 'title': _('These visitors are'), - 'value': _('{}').format( - status - ), + 'value': _('Logged in') if self.is_logged_in else _('Not logged in'), } - - return description diff --git a/src/wagtail_personalisation/views.py b/src/wagtail_personalisation/views.py index 2aebb0c..5cc7961 100644 --- a/src/wagtail_personalisation/views.py +++ b/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.status = Segment.STATUS_DISABLED - elif segment.status == Segment.STATUS_DISABLED: - segment.status = Segment.STATUS_ENABLED - - segment.save() + segment.toggle() return HttpResponseRedirect(request.META.get('HTTP_REFERER', '/')) diff --git a/src/wagtail_personalisation/wagtail_hooks.py b/src/wagtail_personalisation/wagtail_hooks.py index 86a06c7..ac76d6a 100644 --- a/src/wagtail_personalisation/wagtail_hooks.py +++ b/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: - variations = _check_for_variations(user_segments, page) + if user_segments: + 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 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)): + if isinstance(page, PersonalisablePageMixin) and page.get_unused_segments(): 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__ - 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: + for segment in page.get_unused_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)) diff --git a/tests/unit/test_hooks.py b/tests/unit/test_hooks.py index 12e10f0..000a3ab 100644 --- a/tests/unit/test_hooks.py +++ b/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