diff --git a/.github/report_nightly_build_failure.py b/.github/report_nightly_build_failure.py new file mode 100644 index 0000000..7ce29ec --- /dev/null +++ b/.github/report_nightly_build_failure.py @@ -0,0 +1,28 @@ + +""" +Called by GitHub Action when the nightly build fails. +This reports an error to the #nightly-build-failures Slack channel. +""" +import os +import requests + +if "SLACK_WEBHOOK_URL" in os.environ: + # https://docs.github.com/en/free-pro-team@latest/actions/reference/environment-variables#default-environment-variables + repository = os.environ["GITHUB_REPOSITORY"] + run_id = os.environ["GITHUB_RUN_ID"] + url = f"https://github.com/{repository}/actions/runs/{run_id}" + + print("Reporting to #nightly-build-failures slack channel") + response = requests.post( + os.environ["SLACK_WEBHOOK_URL"], + json={ + "text": f"A Nightly build failed. See {url}", + }, + ) + + print(f"Slack responded with: {response}") + +else: + print( + "Unable to report to #nightly-build-failures slack channel because SLACK_WEBHOOK_URL is not set" + ) diff --git a/.github/workflows/nightly-tests.yml b/.github/workflows/nightly-tests.yml new file mode 100644 index 0000000..0dcaaca --- /dev/null +++ b/.github/workflows/nightly-tests.yml @@ -0,0 +1,35 @@ +name: Nightly Wagtail test + +on: + schedule: + - cron: "20 1 * * *" + + workflow_dispatch: + +jobs: + nightly-test: + # Cannot check the existence of secrets, so limiting to repository name to prevent all forks to run nightly. + # See: https://github.com/actions/runner/issues/520 + if: ${{ github.repository == 'torchbox/wagtail-experiments' }} + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - name: Set up Python 3.9 + uses: actions/setup-python@v2 + with: + python-version: 3.9 + - name: Install dependencies + run: | + python -m pip install --upgrade pip + pip install "git+https://github.com/wagtail/wagtail.git@main#egg=wagtail" + pip install -e .[testing] + - name: Test + id: test + continue-on-error: true + run: ./runtests.py + - name: Send Slack notification on failure + if: steps.test.outcome == 'failure' + run: | + python .github/report_nightly_build_failure.py + env: + SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK_URL }} diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml new file mode 100644 index 0000000..e604864 --- /dev/null +++ b/.github/workflows/test.yml @@ -0,0 +1,85 @@ + +name: CI + +on: + push: + branches: [ main ] + pull_request: + +# Current configuration: +# - django 3.2, python 3.8, wagtail 4.1, postgres +# - django 4.0, python 3.9, wagtail 5.0, sqlite +# - django 4.1, python 3.10, wagtail 5.1, postgres +# - django 4.2, python 3.11, wagtail 5.2, sqlite +# - django 4.2, python 3.12, wagtail main, sqlite (allow failures) +jobs: + test: + runs-on: ubuntu-latest + continue-on-error: ${{ matrix.experimental }} + strategy: + matrix: + include: + - python: "3.8" + django: "Django>=3.2,<4.0" + wagtail: "wagtail>=4.1,<4.2" + database: "postgresql" + modeladmin: false + experimental: false + - python: "3.9" + django: "Django>=4.0,<4.1" + wagtail: "wagtail>=5.0,<5.1" + database: "sqlite3" + modeladmin: false + experimental: false + - python: "3.10" + django: "Django>=4.1,<4.2" + wagtail: "wagtail>=5.1,<5.2" + database: "postgresql" + modeladmin: true + experimental: false + - python: "3.11" + django: "Django>=4.2,<4.3" + wagtail: "wagtail>=5.2,<5.3" + database: "sqlite3" + modeladmin: true + experimental: false + + - python: "3.12" + django: "Django>=4.2,<4.3" + wagtail: "git+https://github.com/wagtail/wagtail.git@main#egg=wagtail" + database: "sqlite3" + modeladmin: true + experimental: true + + services: + postgres: + image: postgres:14 + env: + POSTGRES_PASSWORD: postgres + ports: + - 5432:5432 + options: --health-cmd pg_isready --health-interval 10s --health-timeout 5s --health-retries 5 + + steps: + - uses: actions/checkout@v3 + - name: Set up Python ${{ matrix.python }} + uses: actions/setup-python@v4 + with: + python-version: ${{ matrix.python }} + - name: Install dependencies + run: | + python -m pip install --upgrade pip + pip install "psycopg2>=2.9.3" + pip install "${{ matrix.django }}" + pip install "${{ matrix.wagtail }}" + pip install -e .[testing] + - name: Install wagtail-modeladmin + if: ${{ matrix.modeladmin }} + run: pip install wagtail-modeladmin + - name: Test + run: ./runtests.py + env: + DATABASE_ENGINE: django.db.backends.${{ matrix.database }} + DATABASE_HOST: localhost + DATABASE_USER: postgres + DATABASE_PASS: postgres diff --git a/.gitignore b/.gitignore index bbded1f..45f141f 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,5 @@ *.pyc /.tox/ /dist/ +/build/ /wagtail_experiments.egg-info/ diff --git a/.travis.yml b/.travis.yml deleted file mode 100644 index 66c4ef0..0000000 --- a/.travis.yml +++ /dev/null @@ -1,39 +0,0 @@ -language: python -cache: pip - -matrix: - include: - - env: TOXENV=py27-dj18-wt17 - python: 2.7 - - env: TOXENV=py27-dj18-wt18 - python: 2.7 - - env: TOXENV=py27-dj19-wt19 - python: 2.7 - - env: TOXENV=py27-dj110-wt110 - python: 2.7 - - env: TOXENV=py27-dj110-wt111 - python: 2.7 - - env: TOXENV=py27-dj111-wt112 - python: 2.7 - - env: TOXENV=py27-dj111-wt113 - python: 2.7 - - env: TOXENV=py34-dj111-wt113 - python: 3.4 - - env: TOXENV=py35-dj111-wt21 - python: 3.5 - - env: TOXENV=py35-dj20-wt20 - python: 3.5 - - env: TOXENV=py36-dj20-wt21 - python: 3.6 - - env: TOXENV=py36-dj20-wt22 - python: 3.6 - - env: TOXENV=py36-dj21-wt23 - python: 3.6 - -# Package installation -install: - - pip install tox - -# Run the tests -script: - tox diff --git a/CHANGELOG.txt b/CHANGELOG.txt index 8ea3c67..78528b9 100644 --- a/CHANGELOG.txt +++ b/CHANGELOG.txt @@ -1,6 +1,21 @@ Changelog ========= +0.3.1 (2023-11-06) +~~~~~~~~~~~~~~~~~~ + + * Use external wagtail-modeladmin package where available + * Added support for Wagtail 5.1 - 5.2 and provisional support for Wagtail 6.0 + + +0.3 (2023-08-10) +~~~~~~~~~~~~~~~~ + + * Added support for Wagtail 4.1 thru 5.0 + * Added support for Django 3.2 thru 4.2 + * Added docstrings to all 'experiment' functions and classes. + * Support internationalization for models + 0.2 (28.11.2018) ~~~~~~~~~~~~~~~~ diff --git a/README.rst b/README.rst index c889f65..acb6253 100644 --- a/README.rst +++ b/README.rst @@ -3,9 +3,6 @@ Wagtail Experiments =================== -.. image:: https://api.travis-ci.org/torchbox/wagtail-experiments.svg?branch=master - :target: https://travis-ci.org/torchbox/wagtail-experiments - **A/B testing for Wagtail** This module supports the creation of A/B testing experiments within a Wagtail site. Several alternative versions of a page are set up, and on visiting a designated control page, a user is presented with one of those variations, selected at random (using a simplified version of the `PlanOut algorithm `_). The number of visitors receiving each variation is logged, along with the number that subsequently go on to complete the experiment by visiting a designated goal page. @@ -14,7 +11,32 @@ This module supports the creation of A/B testing experiments within a Wagtail si Installation ------------ -wagtail-experiments is compatible with Wagtail 1.7 to 2.3, and Django 1.8 to 2.1. To install:: +wagtail-experiments is compatible with Wagtail 4.1 to 5.2, and Django 3.2 to 4.2. It depends on the Wagtail ModelAdmin module, which is available as an external package as of Wagtail 5.0; we recommend using this rather than the bundled `wagtail.contrib.modeladmin` module to avoid deprecation warnings. The external package will be required as of Wagtail 6.0. + +### On Wagtail 5.0 and above + +To install:: + + pip install wagtail-experiments wagtail-modeladmin + +and ensure that the apps ``wagtail_modeladmin`` and ``experiments`` are included in your project's ``INSTALLED_APPS``: + +.. code-block:: python + + INSTALLED_APPS = [ + # ... + 'wagtail_modeladmin', + 'experiments', + # ... + ] + +Then migrate:: + + ./manage.py migrate + +### On Wagtail 4.x + +To install:: pip install wagtail-experiments @@ -29,7 +51,7 @@ and ensure that the apps ``wagtail.contrib.modeladmin`` and ``experiments`` are # ... ] -Then migrate:: +Then migrate:: ./manage.py migrate diff --git a/experiments/admin_urls.py b/experiments/admin_urls.py index 98553b6..93e4019 100644 --- a/experiments/admin_urls.py +++ b/experiments/admin_urls.py @@ -1,12 +1,10 @@ -from __future__ import absolute_import, unicode_literals - -from django.urls import path +from django.urls import re_path from experiments import views app_name = 'experiments' urlpatterns = [ - path('experiment/report//', views.experiment_report, name='report'), - path('experiment/select_winner///', views.select_winner, name='select_winner'), - path('experiment/report/preview///', views.preview_for_report, name='preview_for_report'), + re_path(r'^experiment/report/(\d+)/$', views.experiment_report, name='report'), + re_path(r'^experiment/select_winner/(\d+)/(\d+)/$', views.select_winner, name='select_winner'), + re_path(r'^experiment/report/preview/(\d+)/(\d+)/$', views.preview_for_report, name='preview_for_report'), ] diff --git a/experiments/backends/db.py b/experiments/backends/db.py index 7493c87..d155b05 100644 --- a/experiments/backends/db.py +++ b/experiments/backends/db.py @@ -1,13 +1,24 @@ -from __future__ import absolute_import, unicode_literals - import datetime - from django.db.models import F, Sum from experiments.models import ExperimentHistory #TODO: potentially the same session? clearing cookies def record_participant(experiment, user_id, variation, request): + ''' + If the user hasn't already participated in this experiment, + then save the variation the user viewed. + + Args: + experiment: instance of experiments.models.Experiment + user_id: the id for this user. + variation: variation user viewed. + request: django HttpRequest. + + Return: + Nothing + ''' + # abort if this user has participated already experiments_started = request.session.get('experiments_started', []) if experiment.id in experiments_started: @@ -23,8 +34,21 @@ def record_participant(experiment, user_id, variation, request): # increment the participant_count ExperimentHistory.objects.filter(pk=history.pk).update(participant_count=F('participant_count') + 1) - def record_completion(experiment, user_id, variation, request): + ''' + If the user has started this experiment, but not completed it yet, + then mark the user's participation as completed. + + Args: + experiment: instance of experiments.models.Experiment + user_id: the id for this user. + variation: variation user viewed. + request: django HttpRequest. + + Return: + Nothing + ''' + # abort if this user never started the experiment if experiment.id not in request.session.get('experiments_started', []): return @@ -46,6 +70,16 @@ def record_completion(experiment, user_id, variation, request): def get_report(experiment): + ''' + Generate a report about the experiment's results. + + Args: + experiment: instance of experiments.models.Experiment + + Return: + A report of experiment results as a dictionary. + ''' + result = {} result.setdefault('variations', []) diff --git a/experiments/models.py b/experiments/models.py index a5ef01b..9023cd5 100644 --- a/experiments/models.py +++ b/experiments/models.py @@ -1,26 +1,31 @@ -from __future__ import absolute_import, unicode_literals - from hashlib import sha1 from importlib import import_module - from django.conf import settings from django.db import models +from django.utils.translation import gettext_lazy as _ + +from wagtail.admin.panels import FieldPanel, PageChooserPanel, InlinePanel +from wagtail.models import Orderable, Page from modelcluster.fields import ParentalKey from modelcluster.models import ClusterableModel -try: - from wagtail.admin.edit_handlers import FieldPanel, PageChooserPanel, InlinePanel - from wagtail.core.models import Orderable -except ImportError: # fallback for Wagtail <2.0 - from wagtail.wagtailadmin.edit_handlers import FieldPanel, PageChooserPanel, InlinePanel - from wagtail.wagtailcore.models import Orderable - BACKEND = None def get_backend(): + ''' + Get the backend to use for wagtail-experiments. + + Args: + None + + Return: + Backend module. + If WAGTAIL_EXPERIMENTS_BACKEND not defined, defaults to experiments.backends.db. + ''' + global BACKEND if BACKEND is None: backend_name = getattr(settings, 'WAGTAIL_EXPERIMENTS_BACKEND', 'experiments.backends.db') @@ -30,10 +35,14 @@ def get_backend(): class Experiment(ClusterableModel): + ''' + Define an experiment for a page. + ''' + STATUS_CHOICES = [ - ('draft', "Draft"), - ('live', "Live"), - ('completed', "Completed"), + ('draft', _("Draft")), + ('live', _("Live")), + ('completed', _("Completed")), ] name = models.CharField(max_length=255) slug = models.SlugField(max_length=255) @@ -47,53 +56,128 @@ class Experiment(ClusterableModel): FieldPanel('name'), FieldPanel('slug'), PageChooserPanel('control_page'), - InlinePanel('alternatives', label="Alternatives"), + InlinePanel('alternatives', heading=_("Alternatives"), label=_("Alternative")), PageChooserPanel('goal'), FieldPanel('goal_url'), FieldPanel('status'), ] + + class Meta: + verbose_name = _('experiment') + verbose_name_plural = _('experiments') + + def __init__(self, *args, **kwargs): super(Experiment, self).__init__(*args, **kwargs) self._initial_status = self.status def activate_alternative_draft_content(self): - # For any alternative pages that are unpublished, copy the latest draft revision - # to the main table (with is_live=False) so that the revision shown as an alternative - # is not an out-of-date one + ''' + For any alternative pages that are unpublished, copy the latest + draft revision to the main table (with is_live=False) so that the + revision shown as an alternative is not an out-of-date one. + + Args: + self: instance of the class Experiment + + Return: + Nothing + ''' + for alternative in self.alternatives.select_related('page'): if not alternative.page.live: - revision = alternative.page.get_latest_revision_as_page() + revision = alternative.page.get_latest_revision_as_object() revision.live = False revision.has_unpublished_changes = True revision.save() def get_variations(self): - return [self.control_page] + [alt.page for alt in self.alternatives.select_related('page')] + ''' + Get all the variations for the control page. + + Return: + variations: a list with the control page and all alternatives + ''' + + variations = [self.control_page] + for alternative in self.alternatives.select_related('page'): + variations.append(alternative.page) + + return variations def get_variation_for_user(self, user_id): + ''' + Get a page variation for this user and request session. + + Args: + user_id: the id for this user + + Return: + variation: a page variation + ''' + variations = self.get_variations() # choose uniformly from variations, based on a hash of user_id and experiment.slug hash_input = "{0}.{1}".format(self.slug, user_id) hash_str = sha1(hash_input.encode('utf-8')).hexdigest()[:7] variation_index = int(hash_str, 16) % len(variations) + return variations[variation_index] def start_experiment_for_user(self, user_id, request): - """ - Record a new participant and return the variation for them to use - """ + ''' + Record a new participant and return the variation for them to use. + + Args: + user_id: the id for this user + request: django HttpRequest + + Return: + Variation the user will see + ''' + variation = self.get_variation_for_user(user_id) get_backend().record_participant(self, user_id, variation, request) return variation def record_completion_for_user(self, user_id, request): + ''' + Record the completion of the variation for the user. + + Args: + user_id: the id for this user + request: django HttpRequest + + Return: + Nothing + + Is the following appropriate? Or should we only consider it a win if + the user sees an experiment page and immediately goes to the goal? + If: + 1. The user goes to a experiment page + 2. Then wanders the site for hours + 3. Finally hits a goal page + Then: + We record a win for the experiment. It isn't. + ''' + backend = get_backend() variation = self.get_variation_for_user(user_id) backend.record_completion(self, user_id, variation, request) def select_winner(self, variation): + ''' + Save the variation that won. + + Args: + variation: winning variation + + Return: + Nothing + ''' + self.winning_variation = variation self.status = 'completed' self.save() @@ -103,6 +187,10 @@ def __str__(self): class Alternative(Orderable): + ''' + Alternative page for the control page. + ''' + experiment = ParentalKey(Experiment, related_name='alternatives', on_delete=models.CASCADE) page = models.ForeignKey('wagtailcore.Page', related_name='+', on_delete=models.CASCADE) @@ -110,11 +198,17 @@ class Alternative(Orderable): PageChooserPanel('page'), ] + class Meta: + verbose_name = _('alternative') + verbose_name_plural = _('alternatives') + class ExperimentHistory(models.Model): - """ - Records the number of participants and completions on a given day for a given variation of an experiment - """ + ''' + Maintains the number of participants and completions + on a given day for a given variation of an experiment. + ''' + experiment = models.ForeignKey(Experiment, related_name='history', on_delete=models.CASCADE) date = models.DateField() variation = models.ForeignKey('wagtailcore.Page', related_name='+', on_delete=models.CASCADE) @@ -125,3 +219,6 @@ class Meta: unique_together = [ ('experiment', 'date', 'variation'), ] + + verbose_name = _('experiment history') + verbose_name_plural = _('experiment histories') diff --git a/experiments/templates/experiments/report.html b/experiments/templates/experiments/report.html index afc2c6c..caa49ec 100644 --- a/experiments/templates/experiments/report.html +++ b/experiments/templates/experiments/report.html @@ -1,5 +1,5 @@ {% extends "wagtailadmin/base.html" %} -{% load i18n staticfiles %} +{% load i18n static %} {% block titletag %}{% trans "Experiment results" %}{% endblock %} @@ -76,7 +76,7 @@

{% trans "Conversion rate / Day" %}

['{{ variation.title|escapejs }}', {% for history_entry in variation_report.history %} - '{{ history_entry.conversion_rate|floatformat:2|escapejs }}'{% if not forloop.last %},{% endif %} + '{{ history_entry.conversion_rate|stringformat:".2f"|escapejs }}'{% if not forloop.last %},{% endif %} {% endfor %} ]{% if not forloop.last %},{% endif %} {% endfor %} diff --git a/experiments/utils.py b/experiments/utils.py index faa4b73..7e5d6ca 100644 --- a/experiments/utils.py +++ b/experiments/utils.py @@ -1,13 +1,38 @@ -from __future__ import absolute_import, unicode_literals - import uuid +from .models import Alternative + def get_user_id(request): + ''' + Get user id for this request. A user ID is assigned randomly + for each request session. + + Args: + request: django HttpRequest. + + Return: + User ID for the request as a str. + + To do: + Require unique user ID for each session. + ''' + return request.session.setdefault('experiment_user_id', str(uuid.uuid4())) def percentage(fraction, population): + ''' + Calc percentage. + + Args: + fraction: Portion of population. + population: Population count. + + Return: + Percentage as float. + ''' + try: return float(fraction) / float(population) * 100 except (ValueError, ZeroDivisionError, TypeError): @@ -15,7 +40,17 @@ def percentage(fraction, population): def impersonate_other_page(page, other): - """Modify the title and tree location data of `page` to resemble `other`""" + ''' + Modify the tree location data of `page` to resemble `other`. + + Args: + page: the page to modify + other: the page to impersonate + + Return: + None + ''' + page.path = other.path page.depth = other.depth page.url_path = other.url_path diff --git a/experiments/views.py b/experiments/views.py index 7b71445..49ca51b 100644 --- a/experiments/views.py +++ b/experiments/views.py @@ -1,22 +1,26 @@ -from __future__ import absolute_import, unicode_literals - from django.core.exceptions import PermissionDenied from django.http import HttpResponse from django.shortcuts import get_object_or_404, redirect, render from django.utils.translation import gettext as _ -try: - from wagtail.admin import messages - from wagtail.core.models import Page -except ImportError: # fallback for Wagtail <2.0 - from wagtail.wagtailadmin import messages - from wagtail.wagtailcore.models import Page +from wagtail.admin import messages +from wagtail.models import Page from .models import Experiment, get_backend from .utils import get_user_id, impersonate_other_page, percentage def record_completion(request, slug): + ''' + Record the completion of the experiment for the user. + + Args: + request: django HttpRequest. + slug: the experiment's slug. + + Return: + OK as the HttpResponse + ''' experiment = get_object_or_404(Experiment, slug=slug) user_id = get_user_id(request) @@ -25,7 +29,17 @@ def record_completion(request, slug): def experiment_report(request, experiment_id): - # TODO: Decide if we need a custom permission to access reports + ''' + Generate a report for the experiment. + + Args: + request: django HttpRequest. + experiment_id: the primary key for the experiment. + + Return: + The experiment's report in rendered HTML using + the experiments/report.html for formatting. + ''' backend = get_backend() experiment = get_object_or_404(Experiment, pk=experiment_id) @@ -59,6 +73,21 @@ def experiment_report(request, experiment_id): def select_winner(request, experiment_id, variation_id): + ''' + Record the winner for the experiment if user has permission + to change the experiment. + + Args: + request: django HttpRequest. + experiment_id: the primary key for the experiment. + variation_id: the primary key for the variation. + + Return: + If request is a POST and the user has permission to change experiment, + then redirect to generate a report. + + If request is not a POST, then return nothing. + ''' if not request.user.has_perm('experiments.change_experiment'): raise PermissionDenied experiment = get_object_or_404(Experiment, pk=experiment_id) @@ -74,14 +103,24 @@ def select_winner(request, experiment_id, variation_id): return redirect('experiments:report', experiment.pk) - def preview_for_report(request, experiment_id, page_id): + ''' + Preview a report if the user has permission to publish. + + Args: + request: django HttpRequest. + experiment_id: the primary key for the experiment. + page_id: the primary key for the page. + + Return: + The rendered page by wagtail. + ''' experiment = get_object_or_404(Experiment, pk=experiment_id) page = get_object_or_404(Page, id=page_id).specific if not page.permissions_for_user(request.user).can_publish(): raise PermissionDenied - # hack the title and page-tree-related fields to match the control page + # hack the page-tree-related fields to match the control page impersonate_other_page(page, experiment.control_page) # pass in the real user request rather than page.dummy_request(), so that request.user diff --git a/experiments/wagtail_hooks.py b/experiments/wagtail_hooks.py index 380d908..b77d8aa 100644 --- a/experiments/wagtail_hooks.py +++ b/experiments/wagtail_hooks.py @@ -1,24 +1,20 @@ -from __future__ import absolute_import, unicode_literals - -from django.conf.urls import include -from django.urls import path +from django.urls import include, re_path from django.contrib.admin.utils import quote - -try: - from django.urls import reverse -except ImportError: # fallback for Django <=1.9 - from django.core.urlresolvers import reverse +from django.urls import reverse from django.utils.translation import gettext_lazy as _ from experiments import admin_urls -from wagtail.contrib.modeladmin.helpers import ButtonHelper -from wagtail.contrib.modeladmin.options import ModelAdmin, modeladmin_register -from wagtail.contrib.modeladmin.views import CreateView, EditView +from wagtail import hooks try: - from wagtail.core import hooks -except ImportError: # fallback for Wagtail <2.0 - from wagtail.wagtailcore import hooks + from wagtail_modeladmin.helpers import ButtonHelper + from wagtail_modeladmin.options import ModelAdmin, modeladmin_register + from wagtail_modeladmin.views import CreateView, EditView +except ImportError: + from wagtail.contrib.modeladmin.helpers import ButtonHelper + from wagtail.contrib.modeladmin.options import ModelAdmin, modeladmin_register + from wagtail.contrib.modeladmin.views import CreateView, EditView + from .models import Experiment from .utils import get_user_id, impersonate_other_page @@ -27,7 +23,7 @@ @hooks.register('register_admin_urls') def register_admin_urls(): return [ - path('experiments/', include(admin_urls, namespace='experiments')), + re_path(r'^experiments/', include(admin_urls, namespace='experiments')), ] @@ -57,6 +53,9 @@ def get_buttons_for_obj(self, obj, exclude=[], classnames_add=[], class CreateExperimentView(CreateView): def form_valid(self, form): + # Called when the creation form is submitted and valid. + # If the form's status is "live", then the alternative + # draft content is activated response = super(CreateExperimentView, self).form_valid(form) if form.instance.status == 'live': form.instance.activate_alternative_draft_content() @@ -65,6 +64,9 @@ def form_valid(self, form): class EditExperimentView(EditView): def form_valid(self, form): + # Called when the edit form is submitted and valid. + # If the form's status is changing from "draft" to "live", then + # the alternative draft content is activated response = super(EditExperimentView, self).form_valid(form) if self.instance._initial_status == 'draft' and self.instance.status == 'live': self.instance.activate_alternative_draft_content() @@ -73,6 +75,9 @@ def form_valid(self, form): class ExperimentModelAdmin(ModelAdmin): + ''' + Define the admin interface for experiments via the ModelAdmin app. + ''' model = Experiment add_to_settings_menu = True button_helper_class = ExperimentButtonHelper @@ -84,6 +89,23 @@ class ExperimentModelAdmin(ModelAdmin): @hooks.register('before_serve_page') def check_experiments(page, request, serve_args, serve_kwargs): + ''' + Check whether the page is a control or goal of an experiment. + If the page is a control page, run the experiment. + If the page is a goal page, log a completion. + + Args: + page: page being served. + request: django HttpRequest. + serve_args: non-keyword arguments for page being served. + serve_kwargs: keyword arguments for the page being served + + Return: + If the page is a control page in a live, yet uncompleted, experiment + and the variation page for this user is not the same as the page, + then show the variation page. Otherwise, return nothing. + ''' + # If the page being served is the goal page of an experiment, log a completion completed_experiments = Experiment.objects.filter(goal=page, status='live') @@ -110,7 +132,7 @@ def check_experiments(page, request, serve_args, serve_kwargs): variation = variation.specific - # hack the title and page-tree-related fields to match the control page + # hack the page-tree-related fields to match the control page impersonate_other_page(variation, page) return variation.serve(request, *serve_args, **serve_kwargs) diff --git a/setup.py b/setup.py index 216ac9e..99030d4 100644 --- a/setup.py +++ b/setup.py @@ -4,7 +4,7 @@ setup( name='wagtail-experiments', - version='0.2.1', + version='0.3.1', description="A/B testing for Wagtail", author='Matthew Westcott', author_email='matthew.westcott@torchbox.com', @@ -13,6 +13,7 @@ include_package_data=True, license='BSD', long_description=open('README.rst').read(), + python_requires=">=3.8", classifiers=[ 'Development Status :: 4 - Beta', 'Environment :: Web Environment', @@ -20,17 +21,19 @@ 'License :: OSI Approved :: BSD License', 'Operating System :: OS Independent', 'Programming Language :: Python', - 'Programming Language :: Python :: 2', - 'Programming Language :: Python :: 2.7', - 'Programming Language :: Python :: 3', - 'Programming Language :: Python :: 3.3', - 'Programming Language :: Python :: 3.4', - 'Programming Language :: Python :: 3.5', - 'Programming Language :: Python :: 3.6', - 'Programming Language :: Python :: 3.7', + 'Programming Language :: Python :: 3.8', + 'Programming Language :: Python :: 3.9', + 'Programming Language :: Python :: 3.10', + 'Programming Language :: Python :: 3.11', + 'Programming Language :: Python :: 3.12', 'Framework :: Django', + 'Framework :: Django :: 3.2', + 'Framework :: Django :: 4.0', + 'Framework :: Django :: 4.1', + 'Framework :: Django :: 4.2', 'Framework :: Wagtail', - 'Framework :: Wagtail :: 1', - 'Framework :: Wagtail :: 2', + 'Framework :: Wagtail :: 4', + 'Framework :: Wagtail :: 5', ], ) + diff --git a/tests/models.py b/tests/models.py index 2a7345c..694ea42 100644 --- a/tests/models.py +++ b/tests/models.py @@ -1,13 +1,6 @@ -from __future__ import absolute_import, unicode_literals - from django.db import models - from modelcluster.fields import ParentalKey - -try: - from wagtail.core.models import Page, Orderable -except ImportError: # fallback for Wagtail <2.0 - from wagtail.wagtailcore.models import Page, Orderable +from wagtail.models import Orderable, Page, Site class SimplePage(Page): @@ -15,7 +8,8 @@ class SimplePage(Page): def get_context(self, request): context = super(SimplePage, self).get_context(request) - context['breadcrumb'] = self.get_ancestors(inclusive=True).filter(depth__gte=request.site.root_page.depth) + site = Site.find_for_request(request) + context['breadcrumb'] = self.get_ancestors(inclusive=True).filter(depth__gte=site.root_page.depth) return context diff --git a/tests/settings.py b/tests/settings.py index b760128..92d42c1 100644 --- a/tests/settings.py +++ b/tests/settings.py @@ -1,10 +1,16 @@ -from __future__ import absolute_import, unicode_literals - import os import django from wagtail import VERSION as WAGTAIL_VERSION +try: + import wagtail_modeladmin +except ImportError: + HAS_MODELADMIN_PACKAGE = False +else: + HAS_MODELADMIN_PACKAGE = True + + DATABASES = { 'default': { 'ENGINE': os.environ.get('DATABASE_ENGINE', 'django.db.backends.sqlite3'), @@ -51,83 +57,46 @@ }, ] -if django.VERSION >= (1, 10): - MIDDLEWARE = [ - 'django.middleware.common.CommonMiddleware', - 'django.contrib.sessions.middleware.SessionMiddleware', - 'django.middleware.csrf.CsrfViewMiddleware', - 'django.contrib.auth.middleware.AuthenticationMiddleware', - 'django.contrib.messages.middleware.MessageMiddleware', - 'django.middleware.clickjacking.XFrameOptionsMiddleware', - ] - - if WAGTAIL_VERSION < (2, 0): - MIDDLEWARE.append('wagtail.wagtailcore.middleware.SiteMiddleware') - else: - MIDDLEWARE.append('wagtail.core.middleware.SiteMiddleware') +MIDDLEWARE = [ + 'django.contrib.sessions.middleware.SessionMiddleware', + 'django.middleware.common.CommonMiddleware', + 'django.middleware.csrf.CsrfViewMiddleware', + 'django.contrib.auth.middleware.AuthenticationMiddleware', + 'django.contrib.messages.middleware.MessageMiddleware', + 'django.contrib.sites.middleware.CurrentSiteMiddleware', + 'django.contrib.sessions.middleware.SessionMiddleware', + 'django.middleware.clickjacking.XFrameOptionsMiddleware', + 'wagtail.contrib.redirects.middleware.RedirectMiddleware', +] -else: - MIDDLEWARE_CLASSES = ( - 'django.middleware.common.CommonMiddleware', - 'django.contrib.sessions.middleware.SessionMiddleware', - 'django.middleware.csrf.CsrfViewMiddleware', - 'django.contrib.auth.middleware.AuthenticationMiddleware', - 'django.contrib.auth.middleware.SessionAuthenticationMiddleware', - 'django.contrib.messages.middleware.MessageMiddleware', - 'django.middleware.clickjacking.XFrameOptionsMiddleware', - - 'wagtail.wagtailcore.middleware.SiteMiddleware', - ) - -if WAGTAIL_VERSION < (2, 0): - INSTALLED_APPS = ( - 'experiments', - 'tests', - - 'wagtail.contrib.modeladmin', - 'wagtail.wagtailsearch', - 'wagtail.wagtailsites', - 'wagtail.wagtailusers', - 'wagtail.wagtailimages', - 'wagtail.wagtaildocs', - 'wagtail.wagtailadmin', - 'wagtail.wagtailcore', - - 'taggit', - - 'django.contrib.admin', - 'django.contrib.auth', - 'django.contrib.contenttypes', - 'django.contrib.sessions', - 'django.contrib.messages', - 'django.contrib.staticfiles', - ) -else: - INSTALLED_APPS = ( - 'experiments', - 'tests', - - 'wagtail.contrib.modeladmin', - 'wagtail.search', - 'wagtail.sites', - 'wagtail.users', - 'wagtail.images', - 'wagtail.documents', - 'wagtail.admin', - 'wagtail.core', - - 'taggit', - - 'django.contrib.admin', - 'django.contrib.auth', - 'django.contrib.contenttypes', - 'django.contrib.sessions', - 'django.contrib.messages', - 'django.contrib.staticfiles', - ) +INSTALLED_APPS = [ + 'experiments', + 'tests', + + 'wagtail_modeladmin' if HAS_MODELADMIN_PACKAGE else 'wagtail.contrib.modeladmin', + 'wagtail.search', + 'wagtail.sites', + 'wagtail.users', + 'wagtail.images', + 'wagtail.documents', + 'wagtail.admin', + 'wagtail', + + 'taggit', + + 'django.contrib.admin', + 'django.contrib.auth', + 'django.contrib.contenttypes', + 'django.contrib.sessions', + 'django.contrib.messages', + 'django.contrib.staticfiles', + ] PASSWORD_HASHERS = ( 'django.contrib.auth.hashers.MD5PasswordHasher', # don't use the intentionally slow default password hasher ) +DEFAULT_AUTO_FIELD = 'django.db.models.AutoField' + WAGTAIL_SITE_NAME = 'wagtail-experiments test' +WAGTAILADMIN_BASE_URL = 'http://127.0.0.1:8000' diff --git a/tests/tests.py b/tests/tests.py index fc1c7fc..83cb877 100644 --- a/tests/tests.py +++ b/tests/tests.py @@ -1,23 +1,15 @@ -from __future__ import absolute_import, unicode_literals +from django import __version__ as DJANGO_VERSION from django.contrib.auth.models import User - -try: - from django.urls import reverse -except ImportError: # fallback for Django <=1.9 - from django.core.urlresolvers import reverse - from django.test import TestCase - -try: - from wagtail.core.models import Page -except ImportError: # fallback for Wagtail <2.0 - from wagtail.wagtailcore.models import Page +from django.urls import reverse +from wagtail.models import Page from experiments.models import Experiment, ExperimentHistory class TestFrontEndView(TestCase): + fixtures = ['test.json'] def setUp(self): @@ -59,7 +51,7 @@ def test_selected_variation_depends_on_user_id(self): for x in range(0, 5): response = self.client.get('/') self.assertEqual(response.status_code, 200) - self.assertContains(response, '

Welcome to our site! It's lovely to meet you.

') + self.assertContains(response, "

Welcome to our site! It's lovely to meet you.

") self.assertContains(response, 'a lovely link') def test_participant_is_logged(self): @@ -289,7 +281,8 @@ def test_completed_status(self): response = self.client.get('/') self.assertEqual(response.status_code, 200) - self.assertContains(response, "

Oh, it's you. What do you want?

") + self.assertContains(response, "Home") + self.assertContains(response, "What do you want?") class TestAdmin(TestCase): @@ -305,6 +298,8 @@ def setUp(self): self.homepage_alternative_1 = Page.objects.get(url_path='/home/home-alternative-1/').specific self.homepage_alternative_2 = Page.objects.get(url_path='/home/home-alternative-2/').specific + self.admin_home = reverse('wagtailadmin_home').strip('/') + def get_edit_postdata(self, **kwargs): alternatives = self.experiment.alternatives.all() @@ -336,18 +331,21 @@ def get_edit_postdata(self, **kwargs): def test_experiments_menu_item(self): response = self.client.get(reverse('wagtailadmin_home')) self.assertEqual(response.status_code, 200) - self.assertContains(response, 'href="/admin/experiments/experiment/"') + try: + self.assertEqual(response.context_data['page_title'], 'Dashboard') + except AssertionError: # fallback for Django <2.0 + self.assertContains(response, f'href="/{self.admin_home}/experiments/experiment/"') def test_experiments_index(self): - response = self.client.get('/admin/experiments/experiment/') + response = self.client.get(f'/{self.admin_home}/experiments/experiment/') self.assertEqual(response.status_code, 200) self.assertContains(response, 'Homepage text') def test_experiment_new(self): - response = self.client.get('/admin/experiments/experiment/create/') + response = self.client.get(f'/{self.admin_home}/experiments/experiment/create/') self.assertEqual(response.status_code, 200) - response = self.client.post('/admin/experiments/experiment/create/', { + response = self.client.post(f'/{self.admin_home}/experiments/experiment/create/', { 'name': "Another experiment", 'slug': 'another-experiment', 'control_page': self.homepage_alternative_1.pk, @@ -362,18 +360,18 @@ def test_experiment_new(self): 'goal': self.homepage.pk, 'status': 'draft', }) - self.assertRedirects(response, '/admin/experiments/experiment/') + self.assertRedirects(response, f'/{self.admin_home}/experiments/experiment/') self.assertTrue(Experiment.objects.filter(slug='another-experiment').exists()) def test_experiment_edit(self): - response = self.client.get('/admin/experiments/experiment/edit/%d/' % self.experiment.pk) + response = self.client.get(f'/{self.admin_home}/experiments/experiment/edit/%d/' % self.experiment.pk) self.assertEqual(response.status_code, 200) response = self.client.post( - '/admin/experiments/experiment/edit/%d/' % self.experiment.pk, + f'/{self.admin_home}/experiments/experiment/edit/{self.experiment.pk}/', self.get_edit_postdata(name="Homepage text updated") ) - self.assertRedirects(response, '/admin/experiments/experiment/') + self.assertRedirects(response, f'/{self.admin_home}/experiments/experiment/') experiment = Experiment.objects.get(pk=self.experiment.pk) self.assertEqual(experiment.name, "Homepage text updated") @@ -388,7 +386,7 @@ def test_draft_page_content_is_activated_when_experiment_goes_live(self): # submit an edit to the experiment, but preserve its live status self.client.post( - '/admin/experiments/experiment/edit/%d/' % self.experiment.pk, + f'/{self.admin_home}/experiments/experiment/edit/{self.experiment.pk}/', self.get_edit_postdata() ) # editing an already-live experiment should not update the page content @@ -397,7 +395,7 @@ def test_draft_page_content_is_activated_when_experiment_goes_live(self): # make the experiment draft self.client.post( - '/admin/experiments/experiment/edit/%d/' % self.experiment.pk, + f'/{self.admin_home}/experiments/experiment/edit/{self.experiment.pk}/', self.get_edit_postdata(status='draft') ) # page content should still be unchanged @@ -406,7 +404,7 @@ def test_draft_page_content_is_activated_when_experiment_goes_live(self): # set the experiment from draft to live self.client.post( - '/admin/experiments/experiment/edit/%d/' % self.experiment.pk, + f'/{self.admin_home}/experiments/experiment/edit/{self.experiment.pk}/', self.get_edit_postdata(status='live') ) # page content should be updated to follow the draft revision now @@ -418,8 +416,12 @@ def test_draft_page_content_is_activated_when_creating_experiment_as_live(self): self.homepage_alternative_1.body = 'updated' self.homepage_alternative_1.save_revision() + # live database entry should not have been updated yet + homepage_alternative_1 = Page.objects.get(pk=self.homepage_alternative_1.pk).specific + self.assertEqual(homepage_alternative_1.body, "Welcome to our site! It's lovely to meet you.") + # create a new experiment with an immediate live status - response = self.client.post('/admin/experiments/experiment/create/', { + response = self.client.post(f'/{self.admin_home}/experiments/experiment/create/', { 'name': "Another experiment", 'slug': 'another-experiment', 'control_page': self.homepage.pk, @@ -435,7 +437,7 @@ def test_draft_page_content_is_activated_when_creating_experiment_as_live(self): 'status': 'live', }) - self.assertRedirects(response, '/admin/experiments/experiment/') + self.assertRedirects(response, f'/{self.admin_home}/experiments/experiment/') # page content should be updated to follow the draft revision now homepage_alternative_1 = Page.objects.get(pk=self.homepage_alternative_1.pk).specific @@ -451,12 +453,12 @@ def test_draft_page_content_is_not_activated_on_published_pages(self): # make the experiment draft self.client.post( - '/admin/experiments/experiment/edit/%d/' % self.experiment.pk, + f'/{self.admin_home}/experiments/experiment/edit/{self.experiment.pk}/', self.get_edit_postdata(status='draft') ) # set the experiment from draft to live self.client.post( - '/admin/experiments/experiment/edit/%d/' % self.experiment.pk, + f'/{self.admin_home}/experiments/experiment/edit/{self.experiment.pk}/', self.get_edit_postdata(status='live') ) @@ -473,7 +475,7 @@ def test_draft_page_content_is_not_activated_on_published_pages_when_creating_ex self.homepage_alternative_1.save_revision() # create a new experiment with an immediate live status - response = self.client.post('/admin/experiments/experiment/create/', { + response = self.client.post(f'/{self.admin_home}/experiments/experiment/create/', { 'name': "Another experiment", 'slug': 'another-experiment', 'control_page': self.homepage.pk, @@ -489,44 +491,39 @@ def test_draft_page_content_is_not_activated_on_published_pages_when_creating_ex 'status': 'live', }) - self.assertRedirects(response, '/admin/experiments/experiment/') + self.assertRedirects(response, f'/{self.admin_home}/experiments/experiment/') # page content should still be unchanged homepage_alternative_1 = Page.objects.get(pk=self.homepage_alternative_1.pk).specific self.assertEqual(homepage_alternative_1.body, "Welcome to our site! It's lovely to meet you.") def test_experiment_delete(self): - response = self.client.get('/admin/experiments/experiment/delete/%d/' % self.experiment.pk) + response = self.client.get(f'/{self.admin_home}/experiments/experiment/delete/{self.experiment.pk}/') self.assertEqual(response.status_code, 200) self.assertContains(response, "Are you sure you want to delete this experiment?") - response = self.client.post('/admin/experiments/experiment/delete/%d/' % self.experiment.pk) - self.assertRedirects(response, '/admin/experiments/experiment/') + response = self.client.post(f'/{self.admin_home}/experiments/experiment/delete/{self.experiment.pk}/') + self.assertRedirects(response, f'/{self.admin_home}/experiments/experiment/') self.assertFalse(Experiment.objects.filter(slug='homepage-text').exists()) def test_show_report(self): - response = self.client.get('/admin/experiments/experiment/report/%d/' % self.experiment.pk) + response = self.client.get(f'/{self.admin_home}/experiments/experiment/report/{self.experiment.pk}/') self.assertEqual(response.status_code, 200) def test_select_winner(self): response = self.client.post( - '/admin/experiments/experiment/select_winner/%d/%d/' % ( - self.experiment.pk, self.homepage_alternative_1.pk - ) + f'/{self.admin_home}/experiments/experiment/select_winner/{self.experiment.pk}/{self.homepage_alternative_1.pk}/' ) self.assertRedirects( response, - '/admin/experiments/experiment/report/%d/' % self.experiment.pk - ) + f'/{self.admin_home}/experiments/experiment/report/{self.experiment.pk}/') experiment = Experiment.objects.get(pk=self.experiment.pk) self.assertEqual(experiment.status, 'completed') self.assertEqual(experiment.winning_variation.pk, self.homepage_alternative_1.pk) def test_preview(self): response = self.client.get( - '/admin/experiments/experiment/report/preview/%d/%d/' % ( - self.experiment.pk, self.homepage_alternative_1.pk - ) + f'/{self.admin_home}/experiments/experiment/report/preview/{self.experiment.pk}/{self.homepage_alternative_2.pk}/' ) self.assertEqual(response.status_code, 200) - self.assertContains(response, "Home") + self.assertContains(response, 'Home') diff --git a/tests/urls.py b/tests/urls.py index 39c538e..1c5790b 100644 --- a/tests/urls.py +++ b/tests/urls.py @@ -1,23 +1,17 @@ -from __future__ import absolute_import, unicode_literals -from django.conf.urls import include, url - -try: - from wagtail.admin import urls as wagtailadmin_urls - from wagtail.core import urls as wagtail_urls -except ImportError: # fallback for Wagtail <2.0 - from wagtail.wagtailadmin import urls as wagtailadmin_urls - from wagtail.wagtailcore import urls as wagtail_urls +from django.urls import include, re_path +from wagtail import urls as wagtail_urls +from wagtail.admin import urls as wagtailadmin_urls from experiments import views as experiment_views urlpatterns = [ - url(r'^admin/', include(wagtailadmin_urls)), + re_path(r'^admin/', include(wagtailadmin_urls)), - url(r'^experiments/complete/([^\/]+)/$', experiment_views.record_completion), + re_path(r'^experiments/complete/([^\/]+)/$', experiment_views.record_completion), # For anything not caught by a more specific rule above, hand over to # Wagtail's serving mechanism - url(r'', include(wagtail_urls)), + re_path(r'', include(wagtail_urls)), ] diff --git a/tox.ini b/tox.ini deleted file mode 100644 index 9608748..0000000 --- a/tox.ini +++ /dev/null @@ -1,34 +0,0 @@ -[tox] -envlist = py{27,34,35,36,37}-dj{18,19,110,111,20,21}-wt{17,18,19,110,111,112,113,20,21,22,23} - -[testenv] -basepython = - py27: python2.7 - py34: python3.4 - py35: python3.5 - py36: python3.6 - py37: python3.7 - -deps = - dj18: Django>=1.8,<1.9 - dj18: djangorestframework>=3.6,<3.7 - dj19: Django>=1.9,<1.10 - dj19: djangorestframework>=3.6,<3.7 - dj110: Django>=1.10,<1.11 - dj111: Django>=1.11,<1.12 - dj20: Django>=2.0,<2.1 - dj21: Django>=2.1,<2.2 - - wt17: wagtail>=1.7,<1.8 - wt18: wagtail>=1.8,<1.9 - wt19: wagtail>=1.9,<1.10 - wt110: wagtail>=1.10,<1.11 - wt111: wagtail>=1.11,<1.12 - wt112: wagtail>=1.12,<1.13 - wt113: wagtail>=1.13,<1.14 - wt20: wagtail>=2.0,<2.1 - wt21: wagtail>=2.1,<2.2 - wt22: wagtail>=2.2,<2.3 - wt23: wagtail>=2.3,<2.4 - -commands = ./runtests.py diff --git a/wagtail_experiments b/wagtail_experiments new file mode 100644 index 0000000..e69de29