From 95bd67ef0d23653c2c1daaf285d1ec8a8910c511 Mon Sep 17 00:00:00 2001 From: Jason Birchall Date: Wed, 9 Sep 2026 12:57:49 +0100 Subject: [PATCH 1/2] Don't check auto-approval restrictions for enterprise versions Skip the check entirely for enterprise: no flag, no history, no activity log. --- src/olympia/versions/models.py | 66 ++++++++++++----------- src/olympia/versions/tests/test_models.py | 29 ++++++++++ 2 files changed, 64 insertions(+), 31 deletions(-) diff --git a/src/olympia/versions/models.py b/src/olympia/versions/models.py index 3fb48dbb93ff..2ba0fde97d50 100644 --- a/src/olympia/versions/models.py +++ b/src/olympia/versions/models.py @@ -575,38 +575,42 @@ def from_upload( if is_mozilla_signed and addon.type != amo.ADDON_LPAPP: reviewer_flags_defaults['auto_approval_disabled'] = True - # Check if the approval should be restricted - checker = RestrictionChecker(upload=upload) - if not checker.is_auto_approval_allowed(): - flag = ( - 'auto_approval_disabled' - if channel == amo.CHANNEL_LISTED - else 'auto_approval_disabled_unlisted' - ) - reviewer_flags_defaults[flag] = True - failed_names = [cls.__name__ for cls in checker.failed_restrictions] - history_ids = [entry.pk for entry in checker.history_entries] - # The checker ran before the version existed, so it could not - # record it on the history rows itself; backfill it now. - if history_ids: - UserRestrictionHistory.objects.filter(pk__in=history_ids).update( - version=version + # Check if the approval should be restricted. Enterprise versions are + # exempt: their auto-approval can never be disabled (see + # AutoApprovalSummary.check_has_auto_approval_disabled()), so the flag + # is not set and nothing is recorded for them. + if channel != amo.CHANNEL_ENTERPRISE: + checker = RestrictionChecker(upload=upload) + if not checker.is_auto_approval_allowed(): + flag = ( + 'auto_approval_disabled' + if channel == amo.CHANNEL_LISTED + else 'auto_approval_disabled_unlisted' + ) + reviewer_flags_defaults[flag] = True + failed_names = [cls.__name__ for cls in checker.failed_restrictions] + history_ids = [entry.pk for entry in checker.history_entries] + # The checker ran before the version existed, so it could not + # record it on the history rows itself; backfill it now. + if history_ids: + UserRestrictionHistory.objects.filter(pk__in=history_ids).update( + version=version + ) + activity.log_create( + amo.LOG.DISABLE_AUTO_APPROVAL, + addon, + details={ + 'channel': version.channel, + 'comments': ( + f'{version.get_channel_display()} auto-approval ' + 'automatically disabled because of a restriction' + f' ({", ".join(failed_names)})' + ), + 'restrictions': failed_names, + 'restriction_history_ids': history_ids, + }, + user=get_task_user(), ) - activity.log_create( - amo.LOG.DISABLE_AUTO_APPROVAL, - addon, - details={ - 'channel': version.channel, - 'comments': ( - f'{version.get_channel_display()} auto-approval automatically ' - 'disabled because of a restriction' - f' ({", ".join(failed_names)})' - ), - 'restrictions': failed_names, - 'restriction_history_ids': history_ids, - }, - user=get_task_user(), - ) if reviewer_flags_defaults: AddonReviewerFlags.objects.update_or_create( diff --git a/src/olympia/versions/tests/test_models.py b/src/olympia/versions/tests/test_models.py index 7ce7ad5d0f23..2d72f79f220f 100644 --- a/src/olympia/versions/tests/test_models.py +++ b/src/olympia/versions/tests/test_models.py @@ -1978,6 +1978,35 @@ def test_enterprise_addon_enabled_waffle_switch(self): assert version assert version.channel == amo.CHANNEL_ENTERPRISE + @override_switch('enterprise-channel', active=True) + def test_auto_approval_not_restricted_for_enterprise(self): + # Enterprise versions are exempt from auto-approval restrictions: + # the disabled flag is ignored for them (see + # AutoApprovalSummary.check_has_auto_approval_disabled()), so the + # check is skipped entirely - no flag, no history, no activity log. + upload_enterprise = self.get_upload( + self.filename, channel=amo.CHANNEL_ENTERPRISE + ) + EmailUserRestriction.objects.create( + email_pattern=upload_enterprise.user.email, + restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL, + ) + version = Version.from_upload( + upload_enterprise, + self.addon, + amo.CHANNEL_ENTERPRISE, + selected_apps=[self.selected_app], + parsed_data=self.dummy_parsed_data, + ) + assert version + assert not AddonReviewerFlags.objects.filter(addon=self.addon).exists() + assert not UserRestrictionHistory.objects.exists() + assert ( + not ActivityLog.objects.for_addons(self.addon) + .filter(action=amo.LOG.DISABLE_AUTO_APPROVAL.id) + .exists() + ) + def test_addon_is_attached_to_upload_if_it_wasnt(self): assert self.upload_listed.addon is None version = Version.from_upload( From eef06d5d17f1915b80e53d0dc7a3b974bcfe7781 Mon Sep 17 00:00:00 2001 From: Jason Birchall Date: Wed, 9 Sep 2026 14:26:35 +0100 Subject: [PATCH 2/2] Flatten the enterprise guard into the existing condition Review feedback: as the RestrictionCheck is not costly, it's okay to fold it into the existing condition instead of adding an indentation level. --- src/olympia/versions/models.py | 61 +++++++++++++++++----------------- 1 file changed, 30 insertions(+), 31 deletions(-) diff --git a/src/olympia/versions/models.py b/src/olympia/versions/models.py index 2ba0fde97d50..41f03f9ebdb2 100644 --- a/src/olympia/versions/models.py +++ b/src/olympia/versions/models.py @@ -579,38 +579,37 @@ def from_upload( # exempt: their auto-approval can never be disabled (see # AutoApprovalSummary.check_has_auto_approval_disabled()), so the flag # is not set and nothing is recorded for them. - if channel != amo.CHANNEL_ENTERPRISE: - checker = RestrictionChecker(upload=upload) - if not checker.is_auto_approval_allowed(): - flag = ( - 'auto_approval_disabled' - if channel == amo.CHANNEL_LISTED - else 'auto_approval_disabled_unlisted' - ) - reviewer_flags_defaults[flag] = True - failed_names = [cls.__name__ for cls in checker.failed_restrictions] - history_ids = [entry.pk for entry in checker.history_entries] - # The checker ran before the version existed, so it could not - # record it on the history rows itself; backfill it now. - if history_ids: - UserRestrictionHistory.objects.filter(pk__in=history_ids).update( - version=version - ) - activity.log_create( - amo.LOG.DISABLE_AUTO_APPROVAL, - addon, - details={ - 'channel': version.channel, - 'comments': ( - f'{version.get_channel_display()} auto-approval ' - 'automatically disabled because of a restriction' - f' ({", ".join(failed_names)})' - ), - 'restrictions': failed_names, - 'restriction_history_ids': history_ids, - }, - user=get_task_user(), + checker = RestrictionChecker(upload=upload) + if channel != amo.CHANNEL_ENTERPRISE and not checker.is_auto_approval_allowed(): + flag = ( + 'auto_approval_disabled' + if channel == amo.CHANNEL_LISTED + else 'auto_approval_disabled_unlisted' + ) + reviewer_flags_defaults[flag] = True + failed_names = [cls.__name__ for cls in checker.failed_restrictions] + history_ids = [entry.pk for entry in checker.history_entries] + # The checker ran before the version existed, so it could not + # record it on the history rows itself; backfill it now. + if history_ids: + UserRestrictionHistory.objects.filter(pk__in=history_ids).update( + version=version ) + activity.log_create( + amo.LOG.DISABLE_AUTO_APPROVAL, + addon, + details={ + 'channel': version.channel, + 'comments': ( + f'{version.get_channel_display()} auto-approval automatically ' + 'disabled because of a restriction' + f' ({", ".join(failed_names)})' + ), + 'restrictions': failed_names, + 'restriction_history_ids': history_ids, + }, + user=get_task_user(), + ) if reviewer_flags_defaults: AddonReviewerFlags.objects.update_or_create(