From d534f5197da7899a7869c6bfc3bcf4cfc769fc6b Mon Sep 17 00:00:00 2001 From: Jared Lockhart <119884+jaredlockhart@users.noreply.github.com> Date: Fri, 15 Nov 2024 18:29:29 -0500 Subject: [PATCH] feat(nimbus): add a ready for review debug api field (#11783) Because * We previously disabled the ready for review serializer after an experiment is live for performance reasons * We now need to inspect the ready for review serliaizer output for live experiments that may have launched without validation This commit * Moves the live check out of the ready for review serializer and into the graphql field * Adds a second graphql field that is not used by the frontend but can be manually invoked to run the ready for review serializer fixes #11782 --- .../experiments/api/v5/serializers.py | 43 +++++++------- .../experimenter/experiments/api/v5/types.py | 21 +++++++ .../experiments/tests/api/v5/test_queries.py | 19 ++++++- ...test_nimbus_ready_for_review_serializer.py | 56 ------------------- .../experimenter/nimbus-ui/schema.graphql | 1 + 5 files changed, 61 insertions(+), 79 deletions(-) diff --git a/experimenter/experimenter/experiments/api/v5/serializers.py b/experimenter/experimenter/experiments/api/v5/serializers.py index 6486fbede..0ed8dda48 100644 --- a/experimenter/experimenter/experiments/api/v5/serializers.py +++ b/experimenter/experimenter/experiments/api/v5/serializers.py @@ -2217,28 +2217,27 @@ class NimbusReviewSerializer(serializers.ModelSerializer): return data def validate(self, data): - if self.instance.status == self.instance.Status.DRAFT: - application = data.get("application") - channel = data.get("channel") - if application != NimbusExperiment.Application.DESKTOP and not channel: - raise serializers.ValidationError( - {"channel": "Channel is required for this application."} - ) - data = super().validate(data) - data = self._validate_versions(data) - data = self._validate_localizations(data) - data = self._validate_feature_configs(data) - data = self._validate_enrollment_targeting(data) - data = self._validate_sticky_enrollment(data) - data = self._validate_rollout_version_support(data) - data = self._validate_bucket_duplicates(data) - data = self._validate_proposed_release_date(data) - if application == NimbusExperiment.Application.DESKTOP: - data = self._validate_desktop_pref_rollouts(data) - data = self._validate_desktop_pref_flips(data) - else: - data = self._validate_languages_versions(data) - data = self._validate_countries_versions(data) + application = data.get("application") + channel = data.get("channel") + if application != NimbusExperiment.Application.DESKTOP and not channel: + raise serializers.ValidationError( + {"channel": "Channel is required for this application."} + ) + data = super().validate(data) + data = self._validate_versions(data) + data = self._validate_localizations(data) + data = self._validate_feature_configs(data) + data = self._validate_enrollment_targeting(data) + data = self._validate_sticky_enrollment(data) + data = self._validate_rollout_version_support(data) + data = self._validate_bucket_duplicates(data) + data = self._validate_proposed_release_date(data) + if application == NimbusExperiment.Application.DESKTOP: + data = self._validate_desktop_pref_rollouts(data) + data = self._validate_desktop_pref_flips(data) + else: + data = self._validate_languages_versions(data) + data = self._validate_countries_versions(data) return data diff --git a/experimenter/experimenter/experiments/api/v5/types.py b/experimenter/experimenter/experiments/api/v5/types.py index 7be638792..77efbb014 100644 --- a/experimenter/experimenter/experiments/api/v5/types.py +++ b/experimenter/experimenter/experiments/api/v5/types.py @@ -573,6 +573,7 @@ class NimbusExperimentType(DjangoObjectType): qa_signoff = graphene.NonNull(graphene.Boolean) qa_status = NimbusExperimentQAStatusEnum() ready_for_review = graphene.Field(NimbusReviewType) + ready_for_review_debug = graphene.Field(NimbusReviewType) recipe_json = graphene.String() reference_branch = graphene.Field(NimbusBranchType) rejection = graphene.Field(NimbusChangeLogType) @@ -658,6 +659,7 @@ class NimbusExperimentType(DjangoObjectType): "qa_comment", "qa_status", "ready_for_review", + "ready_for_review_debug", "recipe_json", "reference_branch", "rejection", @@ -708,6 +710,25 @@ class NimbusExperimentType(DjangoObjectType): return [NimbusBranch(name=NimbusConstants.DEFAULT_TREATMENT_BRANCH_NAME)] def resolve_ready_for_review(self, info): + if self.status == self.Status.DRAFT: + serializer = NimbusReviewSerializer( + self, + data=NimbusReviewSerializer(self).data, + ) + ready = serializer.is_valid() + return NimbusReviewType( + message=serializer.errors, + warnings=serializer.warnings, + ready=ready, + ) + else: + return NimbusReviewType( + message={}, + warnings={}, + ready=True, + ) + + def resolve_ready_for_review_debug(self, info): serializer = NimbusReviewSerializer( self, data=NimbusReviewSerializer(self).data, diff --git a/experimenter/experimenter/experiments/tests/api/v5/test_queries.py b/experimenter/experimenter/experiments/tests/api/v5/test_queries.py index 350908143..2a62e181c 100644 --- a/experimenter/experimenter/experiments/tests/api/v5/test_queries.py +++ b/experimenter/experimenter/experiments/tests/api/v5/test_queries.py @@ -600,6 +600,12 @@ class TestNimbusExperimentBySlugQuery(GraphQLTestCase): data=NimbusReviewSerializer(experiment).data, ) review_ready = review_serializer.is_valid() + review_errors = review_serializer.errors + review_warnings = review_serializer.warnings + if experiment.status != NimbusExperiment.Status.DRAFT: + review_ready = True + review_errors = {} + review_warnings = {} response = self.query( """ @@ -722,6 +728,12 @@ class TestNimbusExperimentBySlugQuery(GraphQLTestCase): warnings } + readyForReviewDebug { + ready + message + warnings + } + startDate computedDurationDays computedEndDate @@ -933,8 +945,13 @@ class TestNimbusExperimentBySlugQuery(GraphQLTestCase): ).name, "qaStatus": NimbusExperiment.QAStatus(experiment.qa_status).name, "readyForReview": { - "message": review_serializer.errors, + "message": review_errors, "ready": review_ready, + "warnings": review_warnings, + }, + "readyForReviewDebug": { + "message": review_serializer.errors, + "ready": review_serializer.is_valid(), "warnings": review_serializer.warnings, }, "recipeJson": json.dumps( diff --git a/experimenter/experimenter/experiments/tests/api/v5/test_serializers/test_nimbus_ready_for_review_serializer.py b/experimenter/experimenter/experiments/tests/api/v5/test_serializers/test_nimbus_ready_for_review_serializer.py index e6eb856fb..83b070500 100644 --- a/experimenter/experimenter/experiments/tests/api/v5/test_serializers/test_nimbus_ready_for_review_serializer.py +++ b/experimenter/experimenter/experiments/tests/api/v5/test_serializers/test_nimbus_ready_for_review_serializer.py @@ -4734,62 +4734,6 @@ class TestNimbusReviewSerializerMultiFeature(MockFmlErrorMixin, TestCase): }, ) - @parameterized.expand( - [ - (NimbusExperimentFactory.Lifecycles.CREATED, False), - (NimbusExperimentFactory.Lifecycles.PREVIEW, True), - (NimbusExperimentFactory.Lifecycles.LIVE_APPROVE_APPROVE, True), - (NimbusExperimentFactory.Lifecycles.ENDING_APPROVE_APPROVE, True), - ] - ) - def test_review_failures_are_skipped_for_non_draft(self, lifecycle, expected_valid): - experiment = NimbusExperimentFactory.create_with_lifecycle( - lifecycle, - application=NimbusExperiment.Application.FENIX, - channel=NimbusExperiment.Channel.RELEASE, - feature_configs=[ - NimbusFeatureConfigFactory.create( - application=NimbusExperiment.Application.FENIX, - schemas=[ - NimbusVersionedSchemaFactory.build( - version=None, - schema=None, - ) - ], - ), - NimbusFeatureConfigFactory.create( - application=NimbusExperiment.Application.IOS, - schemas=[ - NimbusVersionedSchemaFactory.build( - version=None, - schema=None, - ) - ], - ), - ], - is_sticky=True, - firefox_min_version=NimbusExperiment.MIN_REQUIRED_VERSION, - ) - - serializer = NimbusReviewSerializer( - experiment, - data=NimbusReviewSerializer( - experiment, - context={"user": self.user}, - ).data, - context={"user": self.user}, - ) - - self.assertEqual(serializer.is_valid(), expected_valid) - if not expected_valid: - self.assertEqual( - serializer.errors["feature_configs"], - [ - "Feature Config application ios does not " - "match experiment application fenix." - ], - ) - @parameterized.expand( [ ({"feature-1": "bogus-collection"},), diff --git a/experimenter/experimenter/nimbus-ui/schema.graphql b/experimenter/experimenter/nimbus-ui/schema.graphql index 851282960..42458ad21 100644 --- a/experimenter/experimenter/nimbus-ui/schema.graphql +++ b/experimenter/experimenter/nimbus-ui/schema.graphql @@ -87,6 +87,7 @@ type NimbusExperimentType { monitoringDashboardUrl: String qaSignoff: Boolean! readyForReview: NimbusReviewType + readyForReviewDebug: NimbusReviewType recipeJson: String rejection: NimbusChangeLogType requiredExperimentsBranches: [NimbusExperimentBranchThroughRequiredType!]!