diff --git a/docs/experimenter/openapi-schema.json b/docs/experimenter/openapi-schema.json index e92e29c26b..b61032a61f 100644 --- a/docs/experimenter/openapi-schema.json +++ b/docs/experimenter/openapi-schema.json @@ -1237,6 +1237,35 @@ "takeaways_summary": { "type": "string", "nullable": true + }, + "conclusion_recommendations": { + "type": "string", + "readOnly": true + }, + "project_impact": { + "enum": [ + "HIGH", + "MODERATE", + "TARGETED" + ], + "type": "string", + "nullable": true + }, + "next_steps": { + "type": "string", + "nullable": true + }, + "reviewer_emails": { + "type": "string", + "readOnly": true + }, + "editor_emails": { + "type": "string", + "readOnly": true + }, + "analysis_errors_count": { + "type": "string", + "readOnly": true } }, "required": [ @@ -1282,6 +1311,14 @@ "type": "string", "readOnly": true }, + "reviewer_emails": { + "type": "string", + "readOnly": true + }, + "editor_emails": { + "type": "string", + "readOnly": true + }, "application_display": { "type": "string", "readOnly": true @@ -1473,6 +1510,10 @@ "results_data": { "type": "string", "readOnly": true + }, + "analysis_errors_by_key": { + "type": "string", + "readOnly": true } }, "required": [ diff --git a/docs/experimenter/swagger-ui.html b/docs/experimenter/swagger-ui.html index 43b56fee87..958ecc9efb 100644 --- a/docs/experimenter/swagger-ui.html +++ b/docs/experimenter/swagger-ui.html @@ -1249,6 +1249,35 @@ "takeaways_summary": { "type": "string", "nullable": true + }, + "conclusion_recommendations": { + "type": "string", + "readOnly": true + }, + "project_impact": { + "enum": [ + "HIGH", + "MODERATE", + "TARGETED" + ], + "type": "string", + "nullable": true + }, + "next_steps": { + "type": "string", + "nullable": true + }, + "reviewer_emails": { + "type": "string", + "readOnly": true + }, + "editor_emails": { + "type": "string", + "readOnly": true + }, + "analysis_errors_count": { + "type": "string", + "readOnly": true } }, "required": [ @@ -1294,6 +1323,14 @@ "type": "string", "readOnly": true }, + "reviewer_emails": { + "type": "string", + "readOnly": true + }, + "editor_emails": { + "type": "string", + "readOnly": true + }, "application_display": { "type": "string", "readOnly": true @@ -1485,6 +1522,10 @@ "results_data": { "type": "string", "readOnly": true + }, + "analysis_errors_by_key": { + "type": "string", + "readOnly": true } }, "required": [ diff --git a/experimenter/experimenter/experiments/api/v5/serializers.py b/experimenter/experimenter/experiments/api/v5/serializers.py index 0dc5a67c68..4bda49b1d0 100644 --- a/experimenter/experimenter/experiments/api/v5/serializers.py +++ b/experimenter/experimenter/experiments/api/v5/serializers.py @@ -229,6 +229,9 @@ class NimbusExperimentCsvSerializer(serializers.ModelSerializer): feature_configs = serializers.SerializerMethodField() experiment_summary = serializers.CharField(source="experiment_url") results_url = serializers.SerializerMethodField() + conclusion_recommendations = serializers.SerializerMethodField() + reviewer_emails = serializers.SerializerMethodField() + editor_emails = serializers.SerializerMethodField() class Meta: model = NimbusExperiment @@ -249,6 +252,12 @@ class Meta: "takeaways_gain_amount", "takeaways_qbr_learning", "takeaways_summary", + "conclusion_recommendations", + "project_impact", + "next_steps", + "reviewer_emails", + "editor_emails", + "analysis_errors_count", ] def get_feature_configs(self, obj): @@ -260,6 +269,15 @@ def get_feature_configs(self, obj): def get_results_url(self, obj): return f"{obj.experiment_url}results" if obj.results_ready else "" + def get_conclusion_recommendations(self, obj): + return ",".join(obj.conclusion_recommendation_labels) + + def get_reviewer_emails(self, obj): + return ",".join(obj.reviewer_emails) + + def get_editor_emails(self, obj): + return ",".join(obj.editor_emails) + class NimbusExperimentYamlSerializer(serializers.ModelSerializer): owner = serializers.SlugRelatedField(read_only=True, slug_field="email") @@ -294,6 +312,8 @@ class Meta: "hypothesis", "is_rollout", "owner", + "reviewer_emails", + "editor_emails", "application_display", "channels", "feature_configs", @@ -338,6 +358,7 @@ class Meta: "excluded_experiments", "parent_experiment", "results_data", + "analysis_errors_by_key", ] def get_hypothesis(self, obj): diff --git a/experimenter/experimenter/experiments/api/v5/views.py b/experimenter/experimenter/experiments/api/v5/views.py index 9a8ab47ab7..79a7ae83af 100644 --- a/experimenter/experimenter/experiments/api/v5/views.py +++ b/experimenter/experimenter/experiments/api/v5/views.py @@ -5,7 +5,7 @@ import yaml from dateutil.relativedelta import relativedelta -from django.db.models import F +from django.db.models import F, Prefetch from django.http import HttpResponse from rest_framework.generics import ListAPIView, UpdateAPIView from rest_framework.pagination import PageNumberPagination @@ -32,7 +32,15 @@ class NimbusExperimentCsvListView(CachedListMixin, ListAPIView): cache_content_type = "text/csv; charset=utf-8" queryset = ( NimbusExperiment.objects.select_related("owner") - .prefetch_related("feature_configs") + .prefetch_related( + "feature_configs", + Prefetch( + "changes", + queryset=NimbusChangeLog.objects.select_related("changed_by").defer( + "experiment_data" + ), + ), + ) .filter(is_archived=False) ) serializer_class = NimbusExperimentCsvSerializer @@ -195,6 +203,12 @@ class NimbusExperimentYamlListView(CachedListMixin, ListAPIView): "tags", "required_experiments", "excluded_experiments", + Prefetch( + "changes", + queryset=NimbusChangeLog.objects.select_related("changed_by").defer( + "experiment_data" + ), + ), ) .filter(is_archived=False, status=NimbusExperiment.Status.COMPLETE) .order_by(F("_start_date").desc(nulls_last=True), "-id") diff --git a/experimenter/experimenter/experiments/models.py b/experimenter/experimenter/experiments/models.py index d70b3866c5..3ecb59912d 100644 --- a/experimenter/experimenter/experiments/models.py +++ b/experimenter/experimenter/experiments/models.py @@ -2773,6 +2773,41 @@ def has_results_errors(self): return True return False + @property + def analysis_errors_by_key(self): + counts = {} + if self.results_data: + errors = self.results_data.get("v3", {}).get("errors", {}) + for key, key_errors in errors.items(): + if key_errors: + counts[key] = len(key_errors) + return counts + + @property + def analysis_errors_count(self): + return sum(self.analysis_errors_by_key.values()) + + @property + def reviewer_emails(self): + return sorted( + { + change.changed_by.email + for change in self.changes.all() + if change.old_publish_status == self.PublishStatus.REVIEW + and change.new_publish_status == self.PublishStatus.APPROVED + } + ) + + @property + def editor_emails(self): + return sorted( + { + change.changed_by.email + for change in self.changes.all() + if change.changed_by.email != settings.KINTO_DEFAULT_CHANGELOG_USER + } + ) + def get_invalid_fields_errors(self, serializer_class=None): if serializer_class is None: serializer = self._review_serializer diff --git a/experimenter/experimenter/experiments/tests/api/v5/test_serializers/test_nimbus_experiment_csv_serializer.py b/experimenter/experimenter/experiments/tests/api/v5/test_serializers/test_nimbus_experiment_csv_serializer.py index cbbea6a413..801e874408 100644 --- a/experimenter/experimenter/experiments/tests/api/v5/test_serializers/test_nimbus_experiment_csv_serializer.py +++ b/experimenter/experimenter/experiments/tests/api/v5/test_serializers/test_nimbus_experiment_csv_serializer.py @@ -5,9 +5,11 @@ from experimenter.experiments.api.v5.serializers import NimbusExperimentCsvSerializer from experimenter.experiments.models import NimbusExperiment from experimenter.experiments.tests.factories import ( + NimbusChangeLogFactory, NimbusExperimentFactory, NimbusFeatureConfigFactory, ) +from experimenter.openidc.tests.factories import UserFactory class TestNimbusExperimentCsvSerializer(TestCase): @@ -45,5 +47,65 @@ def test_serializer_outputs_expected_schema_with_results_link(self): "takeaways_gain_amount": experiment.takeaways_gain_amount, "takeaways_qbr_learning": experiment.takeaways_qbr_learning, "takeaways_summary": experiment.takeaways_summary, + "conclusion_recommendations": "", + "project_impact": None, + "next_steps": None, + "reviewer_emails": experiment.owner.email, + "editor_emails": experiment.owner.email, + "analysis_errors_count": 0, }, ) + + def test_serializer_outputs_reviewers_editors_errors_and_takeaways(self): + application = NimbusExperiment.Application.DESKTOP + feature_config = NimbusFeatureConfigFactory.create(application=application) + + experiment = NimbusExperimentFactory.create_with_lifecycle( + NimbusExperimentFactory.Lifecycles.ENDING_APPROVE_APPROVE, + application=application, + feature_configs=[feature_config], + owner=UserFactory.create(email="owner@example.com"), + conclusion_recommendations=[ + NimbusExperiment.ConclusionRecommendation.RERUN, + NimbusExperiment.ConclusionRecommendation.GRADUATE, + ], + project_impact=NimbusExperiment.ProjectImpact.HIGH, + next_steps="Ship it.", + results_data={ + "v3": { + "errors": { + "ad_clicks": [ + {"analysis_basis": "enrollments", "segment": "all"}, + {"analysis_basis": "exposures", "segment": "all"}, + ], + "experiment": [{"analysis_basis": "enrollments"}], + }, + } + }, + ) + NimbusChangeLogFactory.create( + experiment=experiment, + changed_by=UserFactory.create(email="reviewer@example.com"), + old_publish_status=NimbusExperiment.PublishStatus.REVIEW, + new_publish_status=NimbusExperiment.PublishStatus.APPROVED, + ) + NimbusChangeLogFactory.create( + experiment=experiment, + changed_by=UserFactory.create(email="editor@example.com"), + old_publish_status=NimbusExperiment.PublishStatus.IDLE, + new_publish_status=NimbusExperiment.PublishStatus.IDLE, + ) + + data = NimbusExperimentCsvSerializer(experiment).data + + self.assertEqual( + data["reviewer_emails"], "owner@example.com,reviewer@example.com" + ) + self.assertEqual( + data["editor_emails"], + "editor@example.com,owner@example.com,reviewer@example.com", + ) + self.assertEqual(data["analysis_errors_count"], 3) + self.assertEqual(data["conclusion_recommendations"], "Rerun,Graduate") + self.assertEqual(data["project_impact"], NimbusExperiment.ProjectImpact.HIGH) + self.assertEqual(data["next_steps"], "Ship it.") diff --git a/experimenter/experimenter/experiments/tests/api/v5/test_views.py b/experimenter/experimenter/experiments/tests/api/v5/test_views.py index 8a0af80cff..8b8e5e1e42 100644 --- a/experimenter/experimenter/experiments/tests/api/v5/test_views.py +++ b/experimenter/experimenter/experiments/tests/api/v5/test_views.py @@ -15,10 +15,12 @@ MockFmlErrorMixin, ) from experimenter.experiments.tests.factories import ( + NimbusChangeLogFactory, NimbusExperimentFactory, NimbusFeatureConfigFactory, NimbusFmlErrorDataClass, ) +from experimenter.openidc.tests.factories import UserFactory @override_settings( @@ -357,6 +359,66 @@ def test_yaml_contains_all_fields(self): ]["search_count"] self.assertEqual(search["significance"]["treatment"]["overall"]["1"], "positive") + def test_yaml_contains_reviewers_editors_and_analysis_errors(self): + application = NimbusExperiment.Application.DESKTOP + feature_config = NimbusFeatureConfigFactory.create(application=application) + experiment = NimbusExperimentFactory.create_with_lifecycle( + NimbusExperimentFactory.Lifecycles.ENDING_APPROVE_APPROVE, + name="Reviewed Experiment", + slug="reviewed-experiment", + application=application, + feature_configs=[feature_config], + owner=UserFactory.create(email="owner@example.com"), + results_data={ + "v3": { + "errors": { + "ad_clicks": [ + {"analysis_basis": "enrollments", "segment": "all"}, + {"analysis_basis": "exposures", "segment": "all"}, + ], + "retained": [], + "experiment": [{"analysis_basis": "enrollments"}], + }, + } + }, + ) + NimbusChangeLogFactory.create( + experiment=experiment, + changed_by=UserFactory.create(email="reviewer@example.com"), + old_publish_status=NimbusExperiment.PublishStatus.REVIEW, + new_publish_status=NimbusExperiment.PublishStatus.APPROVED, + ) + NimbusChangeLogFactory.create( + experiment=experiment, + changed_by=UserFactory.create(email=settings.KINTO_DEFAULT_CHANGELOG_USER), + old_publish_status=NimbusExperiment.PublishStatus.IDLE, + new_publish_status=NimbusExperiment.PublishStatus.IDLE, + ) + + data = self._get_yaml() + exp = next(e for e in data if e["slug"] == "reviewed-experiment") + + self.assertEqual( + exp["reviewer_emails"], ["owner@example.com", "reviewer@example.com"] + ) + self.assertEqual( + exp["editor_emails"], ["owner@example.com", "reviewer@example.com"] + ) + self.assertEqual(exp["analysis_errors_by_key"], {"ad_clicks": 2, "experiment": 1}) + + def test_yaml_omits_empty_analysis_errors(self): + application = NimbusExperiment.Application.DESKTOP + feature_config = NimbusFeatureConfigFactory.create(application=application) + NimbusExperimentFactory.create_with_lifecycle( + NimbusExperimentFactory.Lifecycles.ENDING_APPROVE_APPROVE, + application=application, + feature_configs=[feature_config], + results_data={"v3": {"errors": {"experiment": []}}}, + ) + + data = self._get_yaml() + self.assertNotIn("analysis_errors_by_key", data[0]) + def test_default_hypothesis_excluded(self): application = NimbusExperiment.Application.DESKTOP feature_config = NimbusFeatureConfigFactory.create(application=application) diff --git a/experimenter/experimenter/experiments/tests/test_models.py b/experimenter/experimenter/experiments/tests/test_models.py index 49818cddcd..2473817378 100644 --- a/experimenter/experimenter/experiments/tests/test_models.py +++ b/experimenter/experimenter/experiments/tests/test_models.py @@ -6136,6 +6136,96 @@ def test_check_experiment_has_results_errors(self, results_data, expected_result self.assertEqual(experiment.has_results_errors, expected_results) + @parameterized.expand( + [ + (None, {}, 0), + ({"other_key": {}}, {}, 0), + ({"v3": {"errors": {"experiment": []}}}, {}, 0), + ( + { + "v3": { + "errors": { + "ad_clicks": [ + {"analysis_basis": "enrollments", "segment": "all"}, + {"analysis_basis": "exposures", "segment": "all"}, + ], + "retained": [], + "experiment": [{"analysis_basis": "enrollments"}], + }, + } + }, + {"ad_clicks": 2, "experiment": 1}, + 3, + ), + ] + ) + def test_analysis_errors(self, results_data, expected_by_key, expected_count): + experiment = NimbusExperimentFactory.create(results_data=results_data) + + self.assertEqual(experiment.analysis_errors_by_key, expected_by_key) + self.assertEqual(experiment.analysis_errors_count, expected_count) + + def test_reviewer_emails_returns_sorted_distinct_approvers(self): + experiment = NimbusExperimentFactory.create() + approver = UserFactory.create(email="zoe@example.com") + + for changed_by, old_publish_status, new_publish_status in [ + ( + approver, + NimbusExperiment.PublishStatus.REVIEW, + NimbusExperiment.PublishStatus.APPROVED, + ), + ( + approver, + NimbusExperiment.PublishStatus.REVIEW, + NimbusExperiment.PublishStatus.APPROVED, + ), + ( + UserFactory.create(email="amy@example.com"), + NimbusExperiment.PublishStatus.REVIEW, + NimbusExperiment.PublishStatus.APPROVED, + ), + ( + UserFactory.create(email="requester@example.com"), + NimbusExperiment.PublishStatus.IDLE, + NimbusExperiment.PublishStatus.REVIEW, + ), + ( + UserFactory.create(email="rejecter@example.com"), + NimbusExperiment.PublishStatus.REVIEW, + NimbusExperiment.PublishStatus.IDLE, + ), + ]: + NimbusChangeLogFactory.create( + experiment=experiment, + changed_by=changed_by, + old_publish_status=old_publish_status, + new_publish_status=new_publish_status, + ) + + self.assertEqual( + experiment.reviewer_emails, ["amy@example.com", "zoe@example.com"] + ) + + def test_editor_emails_excludes_automated_changelog_user(self): + experiment = NimbusExperimentFactory.create() + editor = UserFactory.create(email="zoe@example.com") + + for changed_by in [ + editor, + editor, + UserFactory.create(email="amy@example.com"), + UserFactory.create(email=settings.KINTO_DEFAULT_CHANGELOG_USER), + ]: + NimbusChangeLogFactory.create( + experiment=experiment, + changed_by=changed_by, + old_publish_status=NimbusExperiment.PublishStatus.IDLE, + new_publish_status=NimbusExperiment.PublishStatus.IDLE, + ) + + self.assertEqual(experiment.editor_emails, ["amy@example.com", "zoe@example.com"]) + def test_monitoring_summary_returns_none_when_no_monitoring_data(self): experiment = NimbusExperimentFactory.create(monitoring_data=None) self.assertIsNone(experiment.monitoring_summary)