Tidy up the logic checks and remove the frozen property

src/wagtail_personalisation/forms.py

@@ -10,6 +10,7 @@ from django.core.exceptions import ValidationError
 from django.test.client import RequestFactory
 from django.utils import timezone
 from django.utils.lru_cache import lru_cache
+from django.utils.translation import ugettext_lazy as _
 from wagtail.wagtailadmin.forms import WagtailAdminModelForm
 
 
@@ -30,39 +31,39 @@ def user_from_data(user_id):
 class SegmentAdminForm(WagtailAdminModelForm):
     def clean(self):
         cleaned_data = super(SegmentAdminForm, self).clean()
+        Segment = self._meta.model
 
-        rules = [form for formset in self.formsets.values() for form in formset if form not in formset.deleted_forms]
+        rules = [
-        consistent = rules and all(rule.instance.static for rule in rules)
+            form.instance for formset in self.formsets.values()
+            for form in formset
+            if form not in formset.deleted_forms
+        ]
+        consistent = rules and Segment.all_static(rules)
 
-        from .models import Segment
         if cleaned_data.get('type') == Segment.TYPE_STATIC and not cleaned_data.get('count') and not consistent:
-            raise ValidationError({
+            self.add_error('count', _('Static segments with non-static compatible rules must include a count.'))
-                'count': ('Static segments with non-static compatible rules must include a count.'),
-            })
 
         return cleaned_data
 
     def save(self, *args, **kwargs):
+        is_new = not self.instance.id
+
         instance = super(SegmentAdminForm, self).save(*args, **kwargs)
 
-        if instance.can_populate:
+        if is_new and instance.is_static and instance.all_rules_static:
             from .adapters import get_segment_adapter
 
             request = RequestFactory().get('/')
             request.session = SessionStore()
             adapter = get_segment_adapter(request)
 
-            rules = [rule for rule in instance.get_rules() if rule.static]
-
             for session in Session.objects.filter(expire_date__gt=timezone.now()).iterator():
                 session_data = session.get_decoded()
                 user = user_from_data(session_data.get('_auth_id'))
                 request.user = user
                 request.session = SessionStore(session_key=session.session_key)
-                passes = adapter._test_rules(rules, request, instance.match_any)
+                passes = adapter._test_rules(instance.get_rules(), request, instance.match_any)
                 if passes:
                     instance.sessions.add(session)
 
-        instance.frozen = True
-        instance.save()
         return instance

src/wagtail_personalisation/migrations/0014_add_frozen_to_segment.py

@@ -1,20 +0,0 @@
-# -*- coding: utf-8 -*-
-# Generated by Django 1.11.6 on 2017-10-20 16:26
-from __future__ import unicode_literals
-
-from django.db import migrations, models
-
-
-class Migration(migrations.Migration):
-
-    dependencies = [
-        ('wagtail_personalisation', '0013_add_dynamic_static_to_segment'),
-    ]
-
-    operations = [
-        migrations.AddField(
-            model_name='segment',
-            name='frozen',
-            field=models.BooleanField(default=False),
-        ),
-    ]

src/wagtail_personalisation/models.py

@@ -83,7 +83,6 @@ class Segment(ClusterableModel):
         )
     )
     sessions = models.ManyToManyField(Session)
-    frozen = models.BooleanField(default=False)
 
     objects = SegmentQuerySet.as_manager()
 
@@ -121,22 +120,19 @@ class Segment(ClusterableModel):
     def is_static(self):
         return self.type == self.TYPE_STATIC
 
+    @classmethod
+    def all_static(cls, rules):
+       return all(rule.static for rule in rules)
+
     @property
-    def is_consistent(self):
+    def all_rules_static(self):
         rules = self.get_rules()
-        all_static = all(rule.static for rule in rules)
+        return rules and self.all_static(rules)
-        return rules and all_static
 
     @property
     def is_full(self):
         return self.sessions.count() >= self.count
 
-    @property
-    def can_populate(self):
-        return (
-            self.id and self.is_static and not self.frozen and self.is_consistent
-        )
-
     def encoded_name(self):
         """Return a string with a slug for the segment."""
         return slugify(self.name.lower())

tests/unit/test_static_dynamic_segments.py

@@ -46,7 +46,6 @@ def test_session_added_to_static_segment_at_creation(site, client):
     form = form_with_data(segment, rule)
     instance = form.save()
 
-    assert instance.frozen
     assert session.session_key in instance.sessions.values_list('session_key', flat=True)
 
 
@@ -66,7 +65,6 @@ def test_match_any_correct_populates(site, client):
     form = form_with_data(segment, rule_1, rule_2)
     instance = form.save()
 
-    assert instance.frozen
     assert session.session_key != second_session.session_key
     assert session.session_key in instance.sessions.values_list('session_key', flat=True)
     assert second_session.session_key in instance.sessions.values_list('session_key', flat=True)
@@ -87,7 +85,6 @@ def test_mixed_static_dynamic_session_doesnt_generate_at_creation(site, client):
     form = form_with_data(segment, static_rule, non_static_rule)
     instance = form.save()
 
-    assert instance.frozen
     assert not instance.sessions.all()
 
 
@@ -102,7 +99,6 @@ def test_session_not_added_to_static_segment_after_creation(site, client):
     session.save()
     client.get(site.root_page.url)
 
-    assert instance.frozen
     assert not instance.sessions.all()
 
 
@@ -117,7 +113,6 @@ def test_session_added_to_static_segment_after_creation(site, client):
     session.save()
     client.get(site.root_page.url)
 
-    assert instance.frozen
     assert session.session_key in instance.sessions.values_list('session_key', flat=True)
 
 
@@ -128,7 +123,6 @@ def test_session_not_added_to_static_segment_after_full(site, client):
     form = form_with_data(segment, rule)
     instance = form.save()
 
-    assert instance.frozen
     assert instance.sessions.count() == 0
 
     session = client.session
@@ -161,7 +155,6 @@ def test_sessions_not_added_to_static_segment_if_rule_not_static(client, site):
     form = form_with_data(segment, rule)
     instance = form.save()
 
-    assert instance.frozen
     assert not instance.sessions.all()
 
 
@@ -175,7 +168,7 @@ def test_does_not_calculate_the_segment_again(site, client, mocker):
     rule = VisitCountRule(counted_page=site.root_page, segment=segment)
     form = form_with_data(segment, rule)
     instance = form.save()
-    assert instance.frozen
+
     assert session.session_key in instance.sessions.values_list('session_key', flat=True)
 
     mock_test_rule = mocker.patch('wagtail_personalisation.adapters.SessionSegmentsAdapter._test_rules')