From f4fc0e904cac614017b2826a7bcac7a6366268f2 Mon Sep 17 00:00:00 2001 From: Jared Kerim Date: Fri, 18 May 2018 15:53:04 -0400 Subject: [PATCH] Filter archived experiments from list page fixes #388 --- .../experiments/tests/test_views.py | 79 +++++++++++++++++-- app/experimenter/experiments/views.py | 41 +++++++++- .../templates/experiments/list.html | 7 ++ app/requirements.txt | 2 +- 4 files changed, 119 insertions(+), 10 deletions(-) diff --git a/app/experimenter/experiments/tests/test_views.py b/app/experimenter/experiments/tests/test_views.py index 1febd9883..7c4064086 100644 --- a/app/experimenter/experiments/tests/test_views.py +++ b/app/experimenter/experiments/tests/test_views.py @@ -12,23 +12,63 @@ from experimenter.experiments.models import Experiment from experimenter.experiments.tests.factories import ExperimentFactory from experimenter.experiments.views import ( ExperimentCreateView, - ExperimentFilter, + ExperimentFilterset, ExperimentFormMixin, ExperimentOrderingForm, ) from experimenter.projects.tests.factories import ProjectFactory -class TestExperimentFilter(TestCase): +class TestExperimentFilterset(TestCase): + + def test_filters_out_archived_by_default(self): + for i in range(3): + ExperimentFactory.create_with_status( + Experiment.STATUS_DRAFT, archived=False) + + for i in range(3): + ExperimentFactory.create_with_status( + Experiment.STATUS_DRAFT, archived=True) + + filter = ExperimentFilterset( + data={}, + queryset=Experiment.objects.all(), + ) + + self.assertEqual( + set(filter.qs), + set(Experiment.objects.filter(archived=False)), + ) + + def test_allows_archived_if_True(self): + for i in range(3): + ExperimentFactory.create_with_status( + Experiment.STATUS_DRAFT, archived=False) + + for i in range(3): + ExperimentFactory.create_with_status( + Experiment.STATUS_DRAFT, archived=True) + + filter = ExperimentFilterset( + data={'archived': True}, + queryset=Experiment.objects.all(), + ) + + self.assertEqual( + set(filter.qs), + set(Experiment.objects.all()), + ) def test_filters_by_status(self): for i in range(3): ExperimentFactory.create_with_status(Experiment.STATUS_DRAFT) ExperimentFactory.create_with_status(Experiment.STATUS_REVIEW) - filter = ExperimentFilter( + + filter = ExperimentFilterset( {'status': Experiment.STATUS_DRAFT}, queryset=Experiment.objects.all(), ) + self.assertEqual( set(filter.qs), set(Experiment.objects.filter(status=Experiment.STATUS_DRAFT)), @@ -44,7 +84,7 @@ class TestExperimentFilter(TestCase): ExperimentFactory.create_with_variants( firefox_version=exclude_version) - filter = ExperimentFilter( + filter = ExperimentFilterset( {'firefox_version': include_version}, queryset=Experiment.objects.all(), ) @@ -63,7 +103,7 @@ class TestExperimentFilter(TestCase): ExperimentFactory.create_with_variants( firefox_channel=exclude_channel) - filter = ExperimentFilter( + filter = ExperimentFilterset( {'firefox_channel': include_channel}, queryset=Experiment.objects.all(), ) @@ -87,7 +127,7 @@ class TestExperimengOrderingForm(TestCase): class TestExperimentListView(TestCase): - def test_list_view_lists_experiments_with_default_order(self): + def test_list_view_lists_experiments_with_default_order_no_archived(self): user_email = 'user@example.com' # Archived experiment is ommitted @@ -110,6 +150,33 @@ class TestExperimentListView(TestCase): self.assertEqual(response.status_code, 200) self.assertEqual(list(context['experiments']), list(experiments)) + def test_list_view_shows_all_including_archived(self): + user_email = 'user@example.com' + + # Archived experiment is included + ExperimentFactory.create_with_status( + Experiment.STATUS_DRAFT, archived=True) + + for i in range(3): + ExperimentFactory.create_with_status( + random.choice(Experiment.STATUS_CHOICES)[0]) + + experiments = Experiment.objects.all() + + response = self.client.get( + '{url}?{params}'.format( + url=reverse('home'), + params=urlencode({ + 'archived': True, + }), + ), + **{settings.OPENIDC_EMAIL_HEADER: user_email}, + ) + + context = response.context[0] + self.assertEqual(response.status_code, 200) + self.assertEqual(set(context['experiments']), set(experiments)) + def test_list_view_filters_and_orders_experiments(self): user_email = 'user@example.com' diff --git a/app/experimenter/experiments/views.py b/app/experimenter/experiments/views.py index 882a582be..fc9da65c1 100644 --- a/app/experimenter/experiments/views.py +++ b/app/experimenter/experiments/views.py @@ -15,7 +15,31 @@ from experimenter.experiments.forms import ( from experimenter.experiments.models import Experiment -class ExperimentFilter(filters.FilterSet): +class ExperimentFiltersetForm(forms.ModelForm): + + class Meta: + model = Experiment + fields = ( + 'archived', + ) + + def clean_archived(self): + allow_archived = self.cleaned_data.get('archived', False) + + # If we pass in archived=True what we actually mean is + # don't filter on archived at all, ie show all experiments + # including archived + if allow_archived: + return None + + return False + + +class ExperimentFilterset(filters.FilterSet): + archived = filters.BooleanFilter( + label='Show archived experiments', + widget=forms.CheckboxInput(), + ) status = filters.ChoiceFilter( empty_label='All Statuses', choices=Experiment.STATUS_CHOICES, @@ -34,7 +58,9 @@ class ExperimentFilter(filters.FilterSet): class Meta: model = Experiment + form = ExperimentFiltersetForm fields = ( + 'archived', 'firefox_channel', 'firefox_version', 'status', @@ -58,15 +84,24 @@ class ExperimentOrderingForm(forms.Form): class ExperimentListView(FilterView): - queryset = Experiment.objects.filter(archived=False) + model = Experiment context_object_name = 'experiments' template_name = 'experiments/list.html' - filterset_class = ExperimentFilter + filterset_class = ExperimentFilterset def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) self.ordering_form = None + def get_filterset_kwargs(self, *args, **kwargs): + kwargs = super().get_filterset_kwargs(*args, **kwargs) + + # Always pass in request.GET otherwise the + # filterset form will be unbound and our custom + # validation won't kick in + kwargs['data'] = self.request.GET + return kwargs + def get_ordering(self): self.ordering_form = ExperimentOrderingForm(self.request.GET) diff --git a/app/experimenter/templates/experiments/list.html b/app/experimenter/templates/experiments/list.html index 5623ecfc0..fe47b15a1 100644 --- a/app/experimenter/templates/experiments/list.html +++ b/app/experimenter/templates/experiments/list.html @@ -106,6 +106,13 @@ {{ filter.form.firefox_version }} + +
+ +
diff --git a/app/requirements.txt b/app/requirements.txt index 6c6e42919..1b9ca225b 100644 --- a/app/requirements.txt +++ b/app/requirements.txt @@ -1,7 +1,7 @@ Django==1.11 coverage==4.3.4 django-cors-headers==2.1.0 -django-filter==1.0.4 +django-filter==1.1.0 djangorestframework==3.6.2 dockerflow==2017.4.0 factory-boy==2.8.1