Merge branch 'release/0.11.2'

Fix off-by-one error in static segment population

CHANGES

@@ -1,3 +1,11 @@
+0.11.2
+==================
+ - Bugfix: Stop populating static segments when the count is reached
+
+0.11.1
+==================
+ - Populate entirely static segments from registered Users not active Sessions
+
 0.11.0
 ==================
  - Bug Fix: Query rule should not be static

docs/conf.py

@@ -55,10 +55,10 @@ author = 'Lab Digital BV'
 # built documents.
 #
 # The short X.Y version.
-version = '0.11.0'
+version = '0.11.2'
 
 # The full version, including alpha/beta/rc tags.
-release = '0.11.0'
+release = '0.11.2'
 
 # The language for content autogenerated by Sphinx. Refer to documentation
 # for a list of supported languages.

setup.cfg

@@ -1,5 +1,5 @@
 [bumpversion]
-current_version = 0.11.0
+current_version = 0.11.2
 commit = true
 tag = true
 tag_name = {new_version}

setup.py

@@ -32,7 +32,7 @@ with open('README.rst') as fh:
 
 setup(
     name='wagtail-personalisation-molo',
-    version='0.11.0',
+    version='0.11.2',
     description='A forked version of Wagtail add-on for showing personalized content',
     author='Praekelt.org',
     author_email='dev@praekeltfoundation.org',

src/wagtail_personalisation/forms.py

@@ -2,12 +2,10 @@ from __future__ import absolute_import, unicode_literals
 
 from datetime import datetime
 from importlib import import_module
-from itertools import takewhile
 
 from django.conf import settings
 from django.contrib.auth import get_user_model
 from django.contrib.auth.models import AnonymousUser
-from django.contrib.sessions.models import Session
 from django.contrib.staticfiles.templatetags.staticfiles import static
 from django.test.client import RequestFactory
 from django.utils.lru_cache import lru_cache
@@ -87,7 +85,7 @@ class SegmentAdminForm(WagtailAdminModelForm):
         if not self.instance.is_static:
             self.instance.count = 0
 
-        if is_new:
+        if is_new and self.instance.is_static and not self.instance.all_rules_static:
             rules = [
                 form.instance for formset in self.formsets.values()
                 for form in formset
@@ -108,23 +106,24 @@ class SegmentAdminForm(WagtailAdminModelForm):
 
             users_to_add = []
             users_to_exclude = []
-            sessions = Session.objects.iterator()
-            take_session = takewhile(
-                lambda x: instance.count == 0 or len(users_to_add) <= instance.count,
-                sessions
-            )
-            for session in take_session:
-                session_data = session.get_decoded()
-                user = user_from_data(session_data.get('_auth_user_id'))
-                if user.is_authenticated():
-                    request.user = user
-                    request.session = SessionStore(session_key=session.session_key)
-                    passes = adapter._test_rules(instance.get_rules(), request, instance.match_any)
-                    if passes and instance.randomise_into_segment():
-                        users_to_add.append(user)
-                    elif passes:
-                        users_to_exclude.append(user)
 
+            User = get_user_model()
+            users = User.objects.filter(is_active=True, is_staff=False)
+
+            matched_count = 0
+            for user in users.iterator():
+                request.user = user
+                passes = adapter._test_rules(instance.get_rules(), request, instance.match_any)
+                if passes:
+                    matched_count += 1
+                    if instance.count == 0 or len(users_to_add) < instance.count:
+                        if instance.randomise_into_segment():
+                            users_to_add.append(user)
+                        else:
+                            users_to_exclude.append(user)
+
+            instance.matched_users_count = matched_count
+            instance.matched_count_updated_at = datetime.now()
             instance.static_users.add(*users_to_add)
             instance.excluded_users.add(*users_to_exclude)
 

tests/unit/test_static_dynamic_segments.py

@@ -35,22 +35,32 @@ def form_with_data(segment, *rules):
 
 
 @pytest.mark.django_db
-def test_session_added_to_static_segment_at_creation(site, client, user):
+def test_user_added_to_static_segment_at_creation(site, user, mocker):
-    session = client.session
-    session.save()
-    client.force_login(user)
-    client.get(site.root_page.url)
-
     segment = SegmentFactory.build(type=Segment.TYPE_STATIC)
     rule = VisitCountRule(counted_page=site.root_page)
     form = form_with_data(segment, rule)
+    mocker.patch('wagtail_personalisation.rules.VisitCountRule.test_user', return_value=True)
     instance = form.save()
 
     assert user in instance.static_users.all()
 
 
 @pytest.mark.django_db
-def test_anonymous_user_not_added_to_static_segment_at_creation(site, client):
+def test_user_not_added_to_full_static_segment_at_creation(site, django_user_model, mocker):
+    django_user_model.objects.create(username='first')
+    django_user_model.objects.create(username='second')
+    segment = SegmentFactory.build(type=Segment.TYPE_STATIC, count=1)
+    rule = VisitCountRule(counted_page=site.root_page)
+    form = form_with_data(segment, rule)
+    mocker.patch('wagtail_personalisation.rules.VisitCountRule.test_user',
+                 side_effect=[True, True])
+    instance = form.save()
+
+    assert len(instance.static_users.all()) == 1
+
+
+@pytest.mark.django_db
+def test_anonymous_user_not_added_to_static_segment_at_creation(site, client, mocker):
     session = client.session
     session.save()
     client.get(site.root_page.url)
@@ -58,43 +68,32 @@ def test_anonymous_user_not_added_to_static_segment_at_creation(site, client):
     segment = SegmentFactory.build(type=Segment.TYPE_STATIC)
     rule = VisitCountRule(counted_page=site.root_page)
     form = form_with_data(segment, rule)
+    mock_test_rule = mocker.patch('wagtail_personalisation.adapters.SessionSegmentsAdapter._test_rules')
     instance = form.save()
 
     assert not instance.static_users.all()
+    assert mock_test_rule.call_count == 0
 
 
 @pytest.mark.django_db
-def test_match_any_correct_populates(site, client, django_user_model):
+def test_match_any_correct_populates(site, django_user_model, mocker):
     user = django_user_model.objects.create(username='first')
-    session = client.session
-    client.force_login(user)
-    client.get(site.root_page.url)
-
     other_user = django_user_model.objects.create(username='second')
-    client.cookies.clear()
-    second_session = client.session
     other_page = site.root_page.get_last_child()
-    client.force_login(other_user)
-    client.get(other_page.url)
 
     segment = SegmentFactory.build(type=Segment.TYPE_STATIC, match_any=True)
     rule_1 = VisitCountRule(counted_page=site.root_page)
     rule_2 = VisitCountRule(counted_page=other_page)
     form = form_with_data(segment, rule_1, rule_2)
+    mocker.patch('wagtail_personalisation.rules.VisitCountRule.test_user', side_effect=[True, False, True, False])
     instance = form.save()
 
-    assert session.session_key != second_session.session_key
     assert user in instance.static_users.all()
     assert other_user in instance.static_users.all()
 
 
 @pytest.mark.django_db
-def test_mixed_static_dynamic_session_doesnt_generate_at_creation(site, client, user):
+def test_mixed_static_dynamic_session_doesnt_generate_at_creation(site, mocker):
-    session = client.session
-    session.save()
-    client.force_login(user)
-    client.get(site.root_page.url)
-
     segment = SegmentFactory.build(type=Segment.TYPE_STATIC, count=1)
     static_rule = VisitCountRule(counted_page=site.root_page)
     non_static_rule = TimeRule(
@@ -102,9 +101,12 @@ def test_mixed_static_dynamic_session_doesnt_generate_at_creation(site, client,
         end_time=datetime.time(23, 59, 59),
     )
     form = form_with_data(segment, static_rule, non_static_rule)
+
+    mock_test_rule = mocker.patch('wagtail_personalisation.adapters.SessionSegmentsAdapter._test_rules')
     instance = form.save()
 
     assert not instance.static_users.all()
+    assert mock_test_rule.call_count == 0
 
 
 @pytest.mark.django_db
@@ -180,12 +182,7 @@ def test_session_not_added_to_static_segment_after_full(site, client, django_use
 
 
 @pytest.mark.django_db
-def test_sessions_not_added_to_static_segment_if_rule_not_static(client, site, user):
+def test_sessions_not_added_to_static_segment_if_rule_not_static(mocker):
-    session = client.session
-    session.save()
-    client.force_login(user)
-    client.get(site.root_page.url)
-
     segment = SegmentFactory.build(type=Segment.TYPE_STATIC, count=1)
     rule = TimeRule(
         start_time=datetime.time(0, 0, 0),
@@ -193,26 +190,27 @@ def test_sessions_not_added_to_static_segment_if_rule_not_static(client, site, u
         segment=segment,
     )
     form = form_with_data(segment, rule)
+    mock_test_rule = mocker.patch('wagtail_personalisation.adapters.SessionSegmentsAdapter._test_rules')
     instance = form.save()
 
     assert not instance.static_users.all()
+    assert mock_test_rule.call_count == 0
 
 
 @pytest.mark.django_db
 def test_does_not_calculate_the_segment_again(site, client, mocker, user):
-    session = client.session
-    session.save()
-    client.force_login(user)
-    client.get(site.root_page.url)
-
     segment = SegmentFactory.build(type=Segment.TYPE_STATIC, count=2)
     rule = VisitCountRule(counted_page=site.root_page, segment=segment)
     form = form_with_data(segment, rule)
+    mocker.patch('wagtail_personalisation.rules.VisitCountRule.test_user', return_value=True)
     instance = form.save()
 
     assert user in instance.static_users.all()
 
     mock_test_rule = mocker.patch('wagtail_personalisation.adapters.SessionSegmentsAdapter._test_rules')
+    session = client.session
+    session.save()
+    client.force_login(user)
     client.get(site.root_page.url)
     assert mock_test_rule.call_count == 0
 
@@ -389,16 +387,12 @@ def test_always_in_segment_if_percentage_is_100(site, client, mocker, user):
 
 
 @pytest.mark.django_db
-def test_not_added_to_static_segment_at_creation_if_random_above_percent(site, client, mocker, user):
+def test_not_added_to_static_segment_at_creation_if_random_above_percent(site, mocker, user):
-    session = client.session
-    session.save()
-    client.force_login(user)
-    client.get(site.root_page.url)
-
     mocker.patch('random.randint', return_value=41)
     segment = SegmentFactory.build(type=Segment.TYPE_STATIC, randomisation_percent=40)
     rule = VisitCountRule(counted_page=site.root_page)
     form = form_with_data(segment, rule)
+    mocker.patch('wagtail_personalisation.rules.VisitCountRule.test_user', return_value=True)
     instance = form.save()
 
     assert user not in instance.static_users.all()
@@ -406,16 +400,12 @@ def test_not_added_to_static_segment_at_creation_if_random_above_percent(site, c
 
 
 @pytest.mark.django_db
-def test_added_to_static_segment_at_creation_if_random_below_percent(site, client, mocker, user):
+def test_added_to_static_segment_at_creation_if_random_below_percent(site, mocker, user):
-    session = client.session
-    session.save()
-    client.force_login(user)
-    client.get(site.root_page.url)
-
     mocker.patch('random.randint', return_value=39)
     segment = SegmentFactory.build(type=Segment.TYPE_STATIC, randomisation_percent=40)
     rule = VisitCountRule(counted_page=site.root_page)
     form = form_with_data(segment, rule)
+    mocker.patch('wagtail_personalisation.rules.VisitCountRule.test_user', return_value=True)
     instance = form.save()
 
     assert user in instance.static_users.all()
@@ -471,7 +461,7 @@ def test_rules_check_skipped_if_dynamic_segment_in_excluded(site, client, mocker
 
 
 @pytest.mark.django_db
-def test_matched_user_count_added_to_segment_at_creation(site, client, mocker, django_user_model):
+def test_matched_user_count_added_to_segment_at_creation(site, mocker, django_user_model):
     django_user_model.objects.create(username='first')
     django_user_model.objects.create(username='second')
 
@@ -479,6 +469,7 @@ def test_matched_user_count_added_to_segment_at_creation(site, client, mocker, d
     rule = VisitCountRule()
 
     form = form_with_data(segment, rule)
+    form.instance.type = Segment.TYPE_STATIC
     mock_test_user = mocker.patch('wagtail_personalisation.rules.VisitCountRule.test_user', return_value=True)
     instance = form.save()