diff --git a/experimenter/experimenter/nimbus_ui/forms.py b/experimenter/experimenter/nimbus_ui/forms.py index 5d904aeab1..ecef904c14 100644 --- a/experimenter/experimenter/nimbus_ui/forms.py +++ b/experimenter/experimenter/nimbus_ui/forms.py @@ -50,6 +50,10 @@ def get_changelog_message(self) -> str: def save(self, *args, **kwargs): experiment = super().save(*args, **kwargs) + + if type(experiment) is NimbusBranchScreenshot: + experiment = self.instance.branch.experiment + generate_nimbus_changelog( experiment, self.request.user, self.get_changelog_message() ) @@ -1771,3 +1775,24 @@ def save(self, commit=True): def get_changelog_message(self): return f"{self.request.user} updated collaborators" + + +class BranchLeadingScreenshotForm(NimbusChangeLogFormMixin, forms.ModelForm): + image = forms.ImageField( + required=False, + widget=forms.FileInput(attrs={"class": "form-control"}), + ) + description = forms.CharField( + required=False, + widget=forms.TextInput(attrs={"class": "form-control"}), + ) + + class Meta: + model = NimbusBranchScreenshot + fields = ["image", "description"] + + def get_changelog_message(self): + return ( + f"{self.request.user} updated leading screenshot for " + f"{self.instance.branch.slug} branch" + ) diff --git a/experimenter/experimenter/nimbus_ui/templates/common/branch_card.html b/experimenter/experimenter/nimbus_ui/templates/common/branch_card.html index e45ca9dd39..ece11fae18 100644 --- a/experimenter/experimenter/nimbus_ui/templates/common/branch_card.html +++ b/experimenter/experimenter/nimbus_ui/templates/common/branch_card.html @@ -1,17 +1,31 @@ {% load nimbus_extras %} -
+
{% with shot=branch.screenshots.first %} {% if shot and shot.image %} - {{ shot.description|default:branch.name }} +
+ {{ shot.description|default:branch.name }} + +
{% else %} -
No screenshot +
{% endif %} {% endwith %} @@ -28,3 +42,63 @@
{{ branch.name }}

{{ branch.description|truncatechars:100 }}

+ diff --git a/experimenter/experimenter/nimbus_ui/templates/nimbus_experiments/results-new-inner.html b/experimenter/experimenter/nimbus_ui/templates/nimbus_experiments/results-new-inner.html index 346d9aa8ca..b9bcb3737e 100644 --- a/experimenter/experimenter/nimbus_ui/templates/nimbus_experiments/results-new-inner.html +++ b/experimenter/experimenter/nimbus_ui/templates/nimbus_experiments/results-new-inner.html @@ -89,8 +89,12 @@
Hypothesis
{% for branch in branch_data %}
- {% include "common/branch_card.html" with branch=branch %} + {% for branch_slug, branch_leading_screenshot_form in branch_leading_screenshot_forms.items %} + {% if branch_slug == branch.slug %} + {% include "common/branch_card.html" with branch=branch experiment_slug=experiment.slug screenshot_form=branch_leading_screenshot_form %} + {% endif %} + {% endfor %}
{% endfor %}
diff --git a/experimenter/experimenter/nimbus_ui/tests/test_forms.py b/experimenter/experimenter/nimbus_ui/tests/test_forms.py index ac9b5d9c77..25f10017c8 100644 --- a/experimenter/experimenter/nimbus_ui/tests/test_forms.py +++ b/experimenter/experimenter/nimbus_ui/tests/test_forms.py @@ -41,6 +41,7 @@ ApproveEndExperimentForm, ApproveUpdateRolloutForm, AudienceForm, + BranchLeadingScreenshotForm, BranchScreenshotCreateForm, BranchScreenshotDeleteForm, CancelEndEnrollmentForm, @@ -4411,3 +4412,49 @@ def test_feature_unsubscribe_form_removes_subscriber(self): self.assertTrue(form.is_valid()) form.save() self.assertNotIn(self.request.user, feature_config.subscribers.all()) + + +class ResultsEditBranchLeadingImageTests(RequestFormTestCase): + def test_edit_leading_image(self): + application = NimbusExperiment.Application.DESKTOP + experiment = NimbusExperimentFactory.create_with_lifecycle( + NimbusExperimentFactory.Lifecycles.CREATED, + application=application, + ) + experiment.branches.all().delete() + experiment.changes.all().delete() + + reference_branch = NimbusBranchFactory.create(experiment=experiment, ratio=1) + experiment.reference_branch = reference_branch + experiment.save() + + reference_screenshot = reference_branch.screenshots.first() + + image_bytes = io.BytesIO() + image = Image.new("RGB", (10, 10), color="red") + image.save(image_bytes, format="PNG") + image_bytes.seek(0) + dummy_image = SimpleUploadedFile( + "test.png", image_bytes.read(), content_type="image/png" + ) + + form = BranchLeadingScreenshotForm( + instance=reference_screenshot, + data={ + "description": "Updated control screenshot", + }, + files={ + "image": dummy_image, + }, + request=self.request, + ) + + form.save() + experiment.refresh_from_db() + + self.assertEqual( + experiment.reference_branch.screenshots.get( + id=reference_screenshot.id + ).description, + "Updated control screenshot", + ) diff --git a/experimenter/experimenter/nimbus_ui/tests/test_views.py b/experimenter/experimenter/nimbus_ui/tests/test_views.py index 89f74e18af..77036f22f9 100644 --- a/experimenter/experimenter/nimbus_ui/tests/test_views.py +++ b/experimenter/experimenter/nimbus_ui/tests/test_views.py @@ -1,11 +1,14 @@ import datetime +import io import json from unittest.mock import patch from django.conf import settings +from django.core.files.uploadedfile import SimpleUploadedFile from django.test import TestCase from django.urls import reverse from parameterized import parameterized +from PIL import Image from experimenter.base.tests.factories import ( CountryFactory, @@ -3028,6 +3031,62 @@ def test_get_redirects_to_next_step(self, current_url, next_url, data): ) +@mock_valid_outcomes +class TestResultsEditBranchImagesView(AuthTestCase): + def test_upload_updates_screenshot(self): + experiment = NimbusExperimentFactory.create(owner=self.user) + branch = NimbusBranchFactory.create(experiment=experiment, slug="t1") + + url = reverse( + "nimbus-ui-branch-leading-screenshot-upload", + kwargs={"slug": experiment.slug, "branch_slug": branch.slug}, + ) + + image_bytes = io.BytesIO() + image = Image.new("RGB", (10, 10), color="red") + image.save(image_bytes, format="PNG") + image_bytes.seek(0) + dummy_image = SimpleUploadedFile( + "test.png", image_bytes.read(), content_type="image/png" + ) + + response = self.client.post(url, {"image": dummy_image}) + + self.assertEqual(response.status_code, 200) + + branch.refresh_from_db() + shot = branch.screenshots.first() + self.assertIsNotNone(shot) + + def test_upload_creates_screenshot(self): + experiment = NimbusExperimentFactory.create(owner=self.user) + experiment.branches.all().delete() + + branch = NimbusBranchFactory.create(experiment=experiment, slug="t1") + branch.screenshots.all().delete() + + url = reverse( + "nimbus-ui-branch-leading-screenshot-upload", + kwargs={"slug": experiment.slug, "branch_slug": branch.slug}, + ) + + image_bytes = io.BytesIO() + image = Image.new("RGB", (10, 10), color="red") + image.save(image_bytes, format="PNG") + image_bytes.seek(0) + dummy_image = SimpleUploadedFile( + "test.png", image_bytes.read(), content_type="image/png" + ) + + response = self.client.post(url, {"image": dummy_image}) + + self.assertEqual(response.status_code, 200) + + branch.refresh_from_db() + shot = branch.screenshots.first() + self.assertIsNotNone(shot) + + @mock_valid_outcomes class TestResultsView(AuthTestCase): def test_render_to_response(self): diff --git a/experimenter/experimenter/nimbus_ui/urls.py b/experimenter/experimenter/nimbus_ui/urls.py index a684147327..ae202b7cf5 100644 --- a/experimenter/experimenter/nimbus_ui/urls.py +++ b/experimenter/experimenter/nimbus_ui/urls.py @@ -10,6 +10,7 @@ BranchDeleteView, BranchesPartialUpdateView, BranchesUpdateView, + BranchLeadingScreenshotView, BranchScreenshotCreateView, BranchScreenshotDeleteView, CancelEndEnrollmentView, @@ -298,4 +299,9 @@ BranchScreenshotDeleteView.as_view(), name="nimbus-ui-delete-branch-screenshot", ), + re_path( + r"^(?P[\w-]+)/branches/(?P[\w-]+)/leading-screenshot/", + BranchLeadingScreenshotView.as_view(), + name="nimbus-ui-branch-leading-screenshot-upload", + ), ] diff --git a/experimenter/experimenter/nimbus_ui/views.py b/experimenter/experimenter/nimbus_ui/views.py index 21e9bf0987..5a03644ee5 100644 --- a/experimenter/experimenter/nimbus_ui/views.py +++ b/experimenter/experimenter/nimbus_ui/views.py @@ -36,6 +36,7 @@ ApproveEndExperimentForm, ApproveUpdateRolloutForm, AudienceForm, + BranchLeadingScreenshotForm, BranchScreenshotCreateForm, BranchScreenshotDeleteForm, CancelEndEnrollmentForm, @@ -671,6 +672,38 @@ class ApproveUpdateRolloutView(StatusUpdateView): form_class = ApproveUpdateRolloutForm +class BranchLeadingScreenshotView( + NimbusExperimentViewMixin, + RequestFormMixin, + UpdateView, +): + form_class = BranchLeadingScreenshotForm + + def get_branch(self): + return ( + self.get_object().branches.filter(slug=self.kwargs.get("branch_slug")).first() + ) + + def get_form_kwargs(self): + kwargs = super().get_form_kwargs() + + if branch := self.get_branch(): + if screenshot := branch.screenshots.first(): + kwargs["instance"] = screenshot + else: + # create an unsaved placeholder instance of the correct model + screenshot_model = branch.screenshots.model + kwargs["instance"] = screenshot_model(branch=branch) + + return kwargs + + def form_valid(self, form): + form.save() + response = HttpResponse() + response.headers["HX-Refresh"] = "true" + return response + + class NewResultsView(NimbusExperimentViewMixin, DetailView): template_name = "nimbus_experiments/results-new.html" @@ -710,6 +743,13 @@ def get_context_data(self, **kwargs): analysis_basis, selected_segment, selected_reference_branch ) + branch_leading_screenshot_forms = { + branch.slug: BranchLeadingScreenshotForm(instance=branch.screenshots.first()) + for branch in experiment.branches.all() + } + + context["branch_leading_screenshot_forms"] = branch_leading_screenshot_forms + relative_metric_changes = {} all_metrics = experiment.get_metric_data(