diff --git a/.github/workflows/experimenter-mozcloud-pr-preview.yaml b/.github/workflows/experimenter-mozcloud-pr-preview.yaml new file mode 100644 index 0000000000..9edf19fcfe --- /dev/null +++ b/.github/workflows/experimenter-mozcloud-pr-preview.yaml @@ -0,0 +1,62 @@ +name: Experimenter -- PR Preview + +on: + pull_request: + types: [labeled, unlabeled, synchronize] + +env: + PROJECT_ID: moz-fx-experimenter-prod-6cd5 + IMAGE_BASE: us-docker.pkg.dev/moz-fx-experimenter-prod-6cd5/experimenter-prod/experimenter + +jobs: + build_and_push: + if: > + ( + github.event_name == 'pull_request' && + contains(github.event.pull_request.labels.*.name, 'pr-preview') && + github.event.pull_request.head.repo.full_name == github.repository + ) + runs-on: ubuntu-latest + permissions: + contents: read + packages: write + id-token: write + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + ref: ${{ github.event.pull_request.head.sha }} + persist-credentials: false + fetch-tags: true + fetch-depth: 0 + + - uses: ./.github/actions/check-changed-paths + id: check-paths + with: + paths: "experimenter/" + + - uses: ./.github/actions/retry + if: steps.check-paths.outputs.should-run == 'true' + with: + label: Build experimenter prod image + run: | + ./scripts/store_git_info.sh + make build_prod + + - name: Tag image(s) for GAR + if: steps.check-paths.outputs.should-run == 'true' + id: meta + shell: bash + run: | + IMAGE_TAG="$(git rev-parse --short=10 HEAD)" + + docker tag experimenter:deploy "${IMAGE_BASE}:${IMAGE_TAG}" + echo "image_tag=${IMAGE_TAG}" >> "$GITHUB_OUTPUT" + + - name: Push to Google Artifact Registry + if: steps.check-paths.outputs.should-run == 'true' + uses: mozilla-it/deploy-actions/docker-push@ef0f037316873ff408a598f1cd98876dd7851e53 # v6.7.0 + with: + image_tags: |- + ${{ env.IMAGE_BASE }}:${{ steps.meta.outputs.image_tag }} + workload_identity_pool_project_number: ${{ vars.GCPV2_WORKLOAD_IDENTITY_POOL_PROJECT_NUMBER }} + project_id: ${{ env.PROJECT_ID }} diff --git a/.github/workflows/experimenter-mozcloud-publish.yaml b/.github/workflows/experimenter-mozcloud-publish.yaml index 3378e6e3ec..15edfe2996 100644 --- a/.github/workflows/experimenter-mozcloud-publish.yaml +++ b/.github/workflows/experimenter-mozcloud-publish.yaml @@ -1,8 +1,6 @@ name: Experimenter -- Build, Tag and Push Container Images to GAR Repository on: - pull_request: - types: [labeled, unlabeled, synchronize] push: branches: - main @@ -25,7 +23,7 @@ jobs: steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - ref: ${{ (github.event_name == 'pull_request' && github.event.pull_request.head.sha) || github.ref }} + ref: ${{ github.ref }} persist-credentials: false fetch-tags: true fetch-depth: 0 diff --git a/experimenter/experimenter/experiments/api/v5/serializers.py b/experimenter/experimenter/experiments/api/v5/serializers.py index 63c254ce06..2877279964 100644 --- a/experimenter/experimenter/experiments/api/v5/serializers.py +++ b/experimenter/experimenter/experiments/api/v5/serializers.py @@ -452,8 +452,6 @@ def get_risk_flags(self, obj): flags.append("Revenue") if obj.risk_brand: flags.append("Brand") - if obj.risk_message: - flags.append("Message") if obj.risk_ai: flags.append("AI") return flags @@ -634,11 +632,6 @@ class NimbusReviewSerializer(serializers.ModelSerializer): allow_null=False, error_messages={"null": NimbusConstants.ERROR_REQUIRED_QUESTION}, ) - risk_message = serializers.BooleanField( - required=True, - allow_null=False, - error_messages={"null": NimbusConstants.ERROR_REQUIRED_QUESTION}, - ) risk_ai = serializers.BooleanField( required=True, allow_null=False, diff --git a/experimenter/experimenter/experiments/changelog_utils.py b/experimenter/experimenter/experiments/changelog_utils.py index 30e5c74bd4..0f23a3646c 100644 --- a/experimenter/experimenter/experiments/changelog_utils.py +++ b/experimenter/experimenter/experiments/changelog_utils.py @@ -1,6 +1,7 @@ import json import uuid +from django.contrib.auth.models import User from django.contrib.postgres.fields import ArrayField from django.core.exceptions import FieldDoesNotExist from django.db import models @@ -59,6 +60,7 @@ class NimbusExperimentChangeLogSerializer(serializers.ModelSerializer): many=True, read_only=True, slug_field="email" ) tags = serializers.SlugRelatedField(many=True, read_only=True, slug_field="name") + message_reviewer = serializers.SlugRelatedField(read_only=True, slug_field="email") class Meta: model = NimbusExperiment @@ -148,6 +150,16 @@ def get_formatted_change_object(field_name, field_diff, changelog, timestamp): old_value = json.dumps(old_value, indent=2) new_value = json.dumps(new_value, indent=2) + # elif ( + # isinstance(field_instance, models.ForeignKey) + # and field_instance.related_model is User + # ): + # values = { + # User.objects. + # } + # old_value = "dingus" + # new_value = "dongus" + elif isinstance(field_instance, (models.JSONField, ArrayField)): event_name = ChangeEventType.DETAILED.name if old_value is not None: diff --git a/experimenter/experimenter/experiments/constants.py b/experimenter/experimenter/experiments/constants.py index 6ec8f5ad17..291ffb0d86 100644 --- a/experimenter/experimenter/experiments/constants.py +++ b/experimenter/experimenter/experiments/constants.py @@ -203,6 +203,8 @@ class ApplicationConfig: #: If None, this application will not support Firefox Labs. firefox_labs: FirefoxLabs | None = field(default=None) + messaging_features: list[str] = field(default=None) + def get_kinto_collection_for_experiment(self, experiment: NimbusExperiment) -> str: if self.kinto_collections_by_feature_id is not None: return self.get_kinto_collection_for_feature_ids( @@ -295,6 +297,42 @@ def kinto_collections(self) -> set[str]: FirefoxLabs.Groups.NEWTAB_WIDGETS: Version.FIREFOX_151, }, ), + # https://searchfox.org/firefox-main/source/browser/components/asrouter/modules/MessagingExperimentConstants.sys.mjs + messaging_features=[ + "cfr", + "infobar", + "moments-page", + "pbNewtab", + "spotlight", + "featureCallout", + "fxms_bmb_button", + "fxms-message", + "fxms-message-1", + "fxms-message-2", + "fxms-message-3", + "fxms-message-4", + "fxms-message-5", + "fxms-message-6", + "fxms-message-7", + "fxms-message-8", + "fxms-message-9", + "fxms-message-10", + "fxms-message-11", + "fxms-message-12", + "fxms-message-13", + "fxms-message-14", + "fxms-message-15", + "fxms-message-16", + "fxms-message-17", + "fxms-message-18", + "fxms-message-19", + "fxms-message-20", + "fxms-message-21", + "fxms-message-22", + "fxms-message-23", + "fxms-message-24", + "fxms-message-25", + ] ) APPLICATION_CONFIG_FENIX = ApplicationConfig( diff --git a/experimenter/experimenter/experiments/migrations/0338_nimbusexperiment_message_reviewer.py b/experimenter/experimenter/experiments/migrations/0338_nimbusexperiment_message_reviewer.py new file mode 100644 index 0000000000..2fdcfacc16 --- /dev/null +++ b/experimenter/experimenter/experiments/migrations/0338_nimbusexperiment_message_reviewer.py @@ -0,0 +1,57 @@ +import django.db.models.deletion +from django.conf import settings +from django.db import migrations, models + + +def create_permissions(apps, schema_editor): + ContentType = apps.get_model("contenttypes", "ContentType") + NimbusExperiment = apps.get_model("experiments", "NimbusExperiment") + Group = apps.get_model("auth", "Group") + Permission = apps.get_model("auth", "Permission") + + content_type = ContentType.objects.get_for_model(NimbusExperiment) + + omc = Group.objects.create(name="OMC") + can_perform_message_review = Permission.objects.create( + codename="can_perform_message_review", + name="Can perform message review", + content_type=content_type, + ) + omc.permissions.add(can_perform_message_review) + + +def delete_permissions(apps, schema_editor): + ContentType = apps.get_model("contenttypes", "ContentType") + NimbusExperiment = apps.get_model("experiments", "NimbusExperiment") + Group = apps.get_model("auth", "Group") + Permission = apps.get_model("auth", "Permission") + + content_type = ContentType.objects.get_for_model(NimbusExperiment) + + Group.objects.filter(name="OMC").delete() + Permission.objects.filter( + content_type=content_type, codename="can_perform_message_review" + ).delete() + + +class Migration(migrations.Migration): + dependencies = [ + ("experiments", "0337_nimbusexperiment_sizing_data"), + ("contenttypes", "0002_remove_content_type_name"), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ] + + operations = [ + migrations.AddField( + model_name="nimbusexperiment", + name="message_reviewer", + field=models.ForeignKey( + blank=True, + default=None, + null=True, + on_delete=django.db.models.deletion.SET_NULL, + to=settings.AUTH_USER_MODEL, + ), + ), + migrations.RunPython(create_permissions, delete_permissions), + ] diff --git a/experimenter/experimenter/experiments/models.py b/experimenter/experimenter/experiments/models.py index f013920921..bb08beceaf 100644 --- a/experimenter/experimenter/experiments/models.py +++ b/experimenter/experimenter/experiments/models.py @@ -423,6 +423,7 @@ def default_firefox_version_parsed(): risk_message = models.BooleanField( "Is a Message Risk Flag", default=None, blank=True, null=True ) + message_reviewer = models.ForeignKey(User, default=None, blank=True, null=True, on_delete=models.SET_NULL) risk_ai = models.BooleanField( "Is an AI Risk Flag", default=None, blank=True, null=True ) @@ -2922,6 +2923,16 @@ def notification_emails(self): ) return list({email for email in emails if email}) + @property + def is_messaging_experiment(self): + application_config = self.application_config + return ( + application_config.messaging_features + and self.feature_configs.filter( + slug__in=application_config.messaging_features + ).exists() + ) + class NimbusBranch(models.Model): experiment = models.ForeignKey( diff --git a/experimenter/experimenter/features/__init__.py b/experimenter/experimenter/features/__init__.py index 882b6425d3..4fff71a56c 100644 --- a/experimenter/experimenter/features/__init__.py +++ b/experimenter/experimenter/features/__init__.py @@ -1,4 +1,5 @@ import json +import logging import re from collections.abc import Iterable from dataclasses import dataclass @@ -30,6 +31,7 @@ FeatureVariableType.BOOLEAN: bool, } +logger = logging.getLogger() @dataclass class Feature: @@ -122,6 +124,7 @@ def _read_manifest( def _load_features(cls): features = [] version_re = re.compile(r"^v(?P\d+)\.(?P\d+)\.(?P\d+)") + i = 0 for application in NimbusConstants.APPLICATION_CONFIGS.values(): application_dir: Path = settings.FEATURE_MANIFESTS_PATH / application.slug @@ -139,11 +142,15 @@ def _load_features(cls): application_yaml_path = child / "experimenter.yaml" if application_yaml_path.exists(): + i+=1 + logging.info(f"Loading manifest {i} {application_yaml_path}") features.extend( cls._read_manifest( application, application_yaml_path, version ) ) + logging.info(f"Loaded manifest {i} {application_yaml_path}") + return features diff --git a/experimenter/experimenter/features/management/commands/load_feature_configs.py b/experimenter/experimenter/features/management/commands/load_feature_configs.py index 0593927344..e9fed81b92 100644 --- a/experimenter/experimenter/features/management/commands/load_feature_configs.py +++ b/experimenter/experimenter/features/management/commands/load_feature_configs.py @@ -48,10 +48,14 @@ def handle(self, *args, **options): # When we are ingesting versioned Features, we want to update the # NimbusFeatureConfig objects with the most up-to-date description. updated: set[tuple[str, str]] = set() - for feature in itertools.chain( + ALL_FEATURES = list(itertools.chain( Features.unversioned(), sorted(Features.versioned(), key=lambda f: f.version, reverse=True), - ): + )) + N = len(ALL_FEATURES) + for (i, feature) in enumerate(ALL_FEATURES): + logger.info(f"processing feature {i}/{N}") + key = (feature.application_slug, feature.slug) if key in updated: # We have already processes the unversioned feature OR a @@ -210,10 +214,11 @@ def handle(self, *args, **options): schema.save(update_fields=dirty_fields) logger.info( - f"Feature Loaded: {feature.application_slug}/{feature.slug} " + f"Feature {i}/{N} Loaded: {feature.application_slug}/{feature.slug} " f"(version {feature.version})" ) + logger.info("attempting bulk create of {len(schemas_to_create)} schemas") NimbusVersionedSchema.objects.bulk_create(schemas_to_create) logger.info("Features Updated") diff --git a/experimenter/experimenter/nimbus_ui/constants.py b/experimenter/experimenter/nimbus_ui/constants.py index 5b1b3cddad..b1c4de6323 100644 --- a/experimenter/experimenter/nimbus_ui/constants.py +++ b/experimenter/experimenter/nimbus_ui/constants.py @@ -35,8 +35,8 @@ class NimbusUIConstants: ERROR_ROLLOUT_REENABLE_REQUIRES_CURRENT_PHASE = ( "Cannot duplicate the final phase because this rollout has no current phase." ) + MESSAGE_CONSULT_URL = "https://mozilla-hub.atlassian.net/wiki/spaces/FPS/pages/2542436381/OMC+Team+2026+-+Onboarding+Messaging+Communication#OMC-Intake---Requesting-work%2FCode-Reviews%2FConsultation" - RISK_MESSAGE_URL = "https://mozilla-hub.atlassian.net/wiki/spaces/FIREFOX/pages/208308555/Message+Consult+Creation" REVIEW_URL = "https://experimenter.info/getting-started/for-reviewers" VALIDATING_EXPERIMENTS_URL = ( "https://experimenter.info/data-analysis/validating-experiments/" @@ -193,7 +193,7 @@ class NimbusUIConstants: } OVERVIEW_PAGE_LINKS = { "risk_link": "https://mana.mozilla.org/wiki/display/FIREFOX/Pref-Flip+and+Add-On+Experiments#PrefFlipandAddOnExperiments-Doesthishavehighrisktothebrand?", - "message_consult_link": "https://mozilla-hub.atlassian.net/wiki/spaces/FIREFOX/pages/208308555/Message+Consult+Creation", + "message_consult_link": "https://mozilla-hub.atlassian.net/wiki/spaces/FPS/pages/2542436381/OMC+Team+2026+-+Onboarding+Messaging+Communication#OMC-Intake---Requesting-work%2FCode-Reviews%2FConsultation", "revenue_risk_link": "https://experimenter.info/workflow/risk-mitigation#vp-sign-off", "partner_related_risk_link": "https://experimenter.info/workflow/risk-mitigation#legal-sign-off", } diff --git a/experimenter/experimenter/nimbus_ui/forms.py b/experimenter/experimenter/nimbus_ui/forms.py index 060e95a8b7..8cfd62e352 100644 --- a/experimenter/experimenter/nimbus_ui/forms.py +++ b/experimenter/experimenter/nimbus_ui/forms.py @@ -392,13 +392,6 @@ class OverviewForm(NimbusChangeLogFormMixin, forms.ModelForm): widget=InlineRadioSelect, coerce=lambda x: x == "True", ) - risk_message = forms.TypedChoiceField( - required=False, - choices=YES_NO_CHOICES, - widget=InlineRadioSelect, - coerce=lambda x: x == "True", - ) - public_description = forms.CharField( required=False, widget=forms.Textarea(attrs={"class": "form-control", "rows": 3}) ) @@ -430,7 +423,6 @@ class Meta: "risk_partner_related", "risk_revenue", "risk_brand", - "risk_message", "risk_ai", ] @@ -446,7 +438,6 @@ def __init__(self, *args, **kwargs): data=self.data or None, instance=self.instance, ) - def is_valid(self): return super().is_valid() and self.documentation_links.is_valid() @@ -722,6 +713,10 @@ class NimbusBranchesForm(NimbusChangeLogFormMixin, forms.ModelForm): required=False, widget=forms.CheckboxInput(attrs={"class": "form-check-input"}) ) is_first_run = forms.BooleanField(required=False, widget=forms.HiddenInput()) + message_review = forms.BooleanField( + required=False, + widget=forms.CheckboxInput(attrs={"class": "form-check-input"}), + ) update_on_change_fields = ( "equal_branch_ratio", @@ -729,6 +724,7 @@ class NimbusBranchesForm(NimbusChangeLogFormMixin, forms.ModelForm): "is_firefox_labs_opt_in", "is_localized", "is_rollout", + "message_review", ) class Meta: @@ -817,6 +813,14 @@ def __init__(self, *args, **kwargs): self.was_labs_opt_in = self.instance.is_firefox_labs_opt_in + if not self.instance.is_messaging_experiment: + self.fields.pop("message_review") + else: + if not self.request.user.has_perm("experiments.can_perform_message_review"): + self.fields["message_review"].disabled = True + + self.fields["message_review"].initial = self.instance.message_reviewer_id is not None + @property def errors(self): errors = super().errors @@ -845,6 +849,24 @@ def clean(self): @transaction.atomic def save(self, *args, **kwargs): self.branches.save() + + if ( + self.instance.is_messaging_experiment + and self.request.user.has_perm("experiments.can_perform_message_review") + ): + message_review = self.cleaned_data.pop("message_review", None) + + if ( + self.instance.message_reviewer_id is None + and message_review is True + ): + self.instance.message_reviewer_id = self.request.user.id + elif ( + self.instance.message_reviewer_id is not None + and message_review is False + ): + self.instance.message_reviewer_id = None + experiment = super().save(*args, **kwargs) if experiment.is_rollout: diff --git a/experimenter/experimenter/nimbus_ui/static/js/review_controls.js b/experimenter/experimenter/nimbus_ui/static/js/review_controls.js index 89906ad8a9..7c73b1e6f5 100644 --- a/experimenter/experimenter/nimbus_ui/static/js/review_controls.js +++ b/experimenter/experimenter/nimbus_ui/static/js/review_controls.js @@ -7,10 +7,12 @@ window.showRecommendation = function () { recommendationMessage.classList.remove("d-none"); }; window.toggleSubmitButton = function () { - const checkbox1 = document.getElementById("checkbox-1"); - const checkbox2 = document.getElementById("checkbox-2"); - const submitButton = document.getElementById("request-launch-button"); - submitButton.disabled = !(checkbox1.checked && checkbox2.checked); + const required = Array.from( + document.getElementById("recommendation-message") + .querySelectorAll('input[type="checkbox"][data-required-for-launch]') + ); + + document.getElementById("request-launch-button").disabled = required.some(el => !el.checked); }; window.updatePreviewURL = function () { diff --git a/experimenter/experimenter/nimbus_ui/templates/nimbus_experiments/detail.html b/experimenter/experimenter/nimbus_ui/templates/nimbus_experiments/detail.html index d48c4c6eed..565cb337a7 100644 --- a/experimenter/experimenter/nimbus_ui/templates/nimbus_experiments/detail.html +++ b/experimenter/experimenter/nimbus_ui/templates/nimbus_experiments/detail.html @@ -10,6 +10,7 @@
{% include "nimbus_experiments/invalid_pages_warning.html" %} {% include "nimbus_experiments/audience_overlap_warnings.html" %} + {% include "nimbus_experiments/message_review_warning.html" %} {% if not invalid_pages or not experiment.is_draft %} {% include "nimbus_experiments/launch_controls.html" %} diff --git a/experimenter/experimenter/nimbus_ui/templates/nimbus_experiments/edit_branches.html b/experimenter/experimenter/nimbus_ui/templates/nimbus_experiments/edit_branches.html index 03db65b9a5..4dae07517f 100644 --- a/experimenter/experimenter/nimbus_ui/templates/nimbus_experiments/edit_branches.html +++ b/experimenter/experimenter/nimbus_ui/templates/nimbus_experiments/edit_branches.html @@ -59,8 +59,8 @@

Branches

{{ form.is_holdback }} +
{{ NimbusUIConstants.HOLDBACK_HELP_TEXT }}
-

{{ NimbusUIConstants.HOLDBACK_HELP_TEXT }}

{% for error in form.is_holdback.errors %}
{{ error }}
{% endfor %} {% for error in validation_errors.is_holdback %}
{{ error }}
{% endfor %}
@@ -100,6 +100,34 @@

Branches

{% endfor %} + {% if experiment.is_messaging_experiment %} +
+
+
+ {{ form.message_review }} + + {% if experiment.message_reviewer is None and not perms.experiments.can_perform_message_review %} +
+ You lack the necessary permissions to perform the message review. Please ask a member of OMC to perform the review. +
+ {% endif %} +
+ This process is optional, but is considered best practice in + order to ensure that end users do not have degraded experiences + due to multiple conflicting messages. +
+ {% if experiment.message_reviewer_id is not None %} +
The message review was completed by {{ experiment.message_reviewer }}.
+ {% endif %} +
+
+
+ {% endif %}
{{ form.branches.management_form }} {% for branch_form in form.branches %} @@ -220,7 +248,7 @@

Branches

{% endwith %}
-
+
{{ screenshot_form.description|add_error_class:"is-invalid" }}
-
-
- -
-
- {{ form.risk_message|add_error_class:"is-invalid" }} - {% for error in form.risk_message.errors %}
{{ error }}
{% endfor %} - {% for error in validation_errors.risk_message %}
{{ error }}
{% endfor %} -
-
diff --git a/experimenter/experimenter/nimbus_ui/templates/nimbus_experiments/launch_controls.html b/experimenter/experimenter/nimbus_ui/templates/nimbus_experiments/launch_controls.html index eb0b959d65..8c977b9265 100644 --- a/experimenter/experimenter/nimbus_ui/templates/nimbus_experiments/launch_controls.html +++ b/experimenter/experimenter/nimbus_ui/templates/nimbus_experiments/launch_controls.html @@ -113,35 +113,54 @@
- +
-
- - {% if experiment.can_publish_to_preview %} + {% if experiment.is_messaging_experiment and not experiment.message_reviewer_id %} +
+ + +
+ {% endif %} +
- {% endif %} + hx-swap="outerHTML" + disabled>Request Launch + {% if experiment.can_publish_to_preview %} + + {% endif %} +
@@ -158,19 +177,36 @@
- +
-
+ {% if experiment.is_messaging_experiment and not experiment.message_reviewer_id %} +
+ + +
+ {% endif %} {% if experiment.can_preview_to_review %}