diff --git a/src/olympia/addons/models.py b/src/olympia/addons/models.py index 699cf74cdc34..7054b1ce9ded 100644 --- a/src/olympia/addons/models.py +++ b/src/olympia/addons/models.py @@ -57,7 +57,6 @@ ) from olympia.constants.browsers import BROWSERS from olympia.constants.categories import CATEGORIES_BY_ID -from olympia.constants.promoted import RECOMMENDED from olympia.constants.reviewers import REPUTATION_CHOICES from olympia.files.models import File from olympia.files.utils import extract_translations, resolve_i18n_message @@ -1571,9 +1570,9 @@ def promoted_groups(self, *, currently_approved=True): return None is_promoted = not currently_approved or promoted.approved_applications return promoted if is_promoted else None - + def check_permission(self, permission, *, truthy=True, currently_approved=True): - """ Does the addon have permissions? + """Does the addon have permissions? `currently_approved=True` means only returns True if self.current_version is approved for the current promotion & apps. @@ -1585,7 +1584,11 @@ def check_permission(self, permission, *, truthy=True, currently_approved=True): except PromotedAddon.DoesNotExist: return False is_promoted = not currently_approved or promoted.approved_applications - return promoted.check_permission(permission, truthy) if is_promoted else False + return ( + promoted.check_permission(permission, self.current_version, truthy) + if is_promoted + else False + ) @cached_property def promoted(self): @@ -1596,7 +1599,7 @@ def promoted(self): from olympia.promoted.models import PromotedTheme if self._is_recommended_theme(): - return PromotedTheme(addon=self, group_id=RECOMMENDED.id) + return PromotedTheme(addon=self) return None @cached_property @@ -1622,7 +1625,9 @@ def can_be_compatible_with_all_fenix_versions(self): versions (i.e. it's a recommended/line extension for Android).""" return ( self.promoted - and self.promoted.check_permission('can_be_compatible_with_all_fenix_versions') + and self.promoted.check_permission( + 'can_be_compatible_with_all_fenix_versions' + ) and amo.ANDROID in self.promoted.approved_applications ) diff --git a/src/olympia/addons/serializers.py b/src/olympia/addons/serializers.py index 4165db462b2a..8a54380d370d 100644 --- a/src/olympia/addons/serializers.py +++ b/src/olympia/addons/serializers.py @@ -964,10 +964,12 @@ def validate_user_id(self, value): class PromotedAddonGroupSerializer(AMOModelSerializer): GROUP_CHOICES = [(group.id, group.api_name) for group in PROMOTED_GROUPS] group_id = serializers.ChoiceField(choices=GROUP_CHOICES) + class Meta: model = PromotedAddonGroup fields = ('group_id',) + class PromotedAddonSerializer(AMOModelSerializer): apps = serializers.SerializerMethodField() categories = PromotedAddonGroupSerializer(many=True, source='promoted_groups') diff --git a/src/olympia/addons/tests/test_models.py b/src/olympia/addons/tests/test_models.py index 33f18b8759eb..a62993a968e2 100644 --- a/src/olympia/addons/tests/test_models.py +++ b/src/olympia/addons/tests/test_models.py @@ -43,7 +43,7 @@ from olympia.bandwagon.models import Collection from olympia.blocklist.models import Block, BlocklistSubmission from olympia.constants.categories import CATEGORIES -from olympia.constants.promoted import LINE, NOT_PROMOTED, RECOMMENDED, SPOTLIGHT +from olympia.constants.promoted import LINE, RECOMMENDED, SPOTLIGHT from olympia.devhub.models import RssKey from olympia.files.models import File from olympia.files.tests.test_models import UploadMixin @@ -1795,7 +1795,9 @@ def test_promoted(self): # If the group changes the approval for the current version isn't # valid. - PromotedAddonGroup.objects.filter(promoted_addon=promoted, group_id=LINE.id).update(group_id=SPOTLIGHT.id) + PromotedAddonGroup.objects.filter( + promoted_addon=promoted, group_id=LINE.id + ).update(group_id=SPOTLIGHT.id) del addon.promoted assert addon.promoted is None diff --git a/src/olympia/amo/tests/__init__.py b/src/olympia/amo/tests/__init__.py index ff89a8872837..d6459e654785 100644 --- a/src/olympia/amo/tests/__init__.py +++ b/src/olympia/amo/tests/__init__.py @@ -29,7 +29,6 @@ from django.utils.encoding import force_str from django.utils.html import escape -from olympia.constants.promoted import NOT_PROMOTED import pytest from rest_framework.reverse import reverse as drf_reverse from rest_framework.settings import api_settings @@ -56,6 +55,7 @@ from olympia.bandwagon.models import Collection from olympia.blocklist.models import Block, BlockType, BlockVersion from olympia.constants.categories import CATEGORIES +from olympia.constants.promoted import NOT_PROMOTED from olympia.files.models import File from olympia.promoted.models import ( PromotedAddon, @@ -586,9 +586,7 @@ def change_channel_for_addon(self, addon, listed): @classmethod def make_addon_promoted(cls, addon, group, approve_version=False): - obj, created = PromotedAddon.objects.update_or_create( - addon=addon - ) + obj, created = PromotedAddon.objects.update_or_create(addon=addon) if group == NOT_PROMOTED: PromotedAddonGroup.objects.filter(promoted_addon=obj).delete() else: @@ -734,7 +732,9 @@ def addon_factory(status=amo.STATUS_APPROVED, version_kw=None, file_kw=None, **k # Save 2. if promoted_group: promoted = PromotedAddon.objects.create(addon=addon) - PromotedAddonGroup.objects.create(promoted_addon=promoted, group_id=promoted_group.id) + PromotedAddonGroup.objects.create( + promoted_addon=promoted, group_id=promoted_group.id + ) if 'promotion_approved' not in version_kw: version_kw['promotion_approved'] = True diff --git a/src/olympia/devhub/forms.py b/src/olympia/devhub/forms.py index 3957923e9632..bce00f210278 100644 --- a/src/olympia/devhub/forms.py +++ b/src/olympia/devhub/forms.py @@ -867,9 +867,8 @@ def __init__(self, *args, **kwargs): # On Android, disable Fenix-pre GA version range unless the add-on is # recommended or line. - if ( - app == amo.ANDROID.id - and not addon.check_permission('can_be_compatible_with_all_fenix_versions') + if app == amo.ANDROID.id and not addon.check_permission( + 'can_be_compatible_with_all_fenix_versions' ): self.fields[ 'min' diff --git a/src/olympia/devhub/views.py b/src/olympia/devhub/views.py index a334ad809131..1a8c8bb4bd8e 100644 --- a/src/olympia/devhub/views.py +++ b/src/olympia/devhub/views.py @@ -1585,8 +1585,15 @@ def _submit_upload( # If we're not showing the generic submit notification warning, show # one specific to pre review if the developer would be affected because # of its promoted group. - if (channel == amo.CHANNEL_LISTED and addon.check_permission('listed_pre_review', currently_approved=False) or ( - channel == amo.CHANNEL_UNLISTED and addon.check_permission('unlisted_pre_review', currently_approved=False)) + if ( + channel == amo.CHANNEL_LISTED + and addon.check_permission('listed_pre_review', currently_approved=False) + or ( + channel == amo.CHANNEL_UNLISTED + and addon.check_permission( + 'unlisted_pre_review', currently_approved=False + ) + ) ): submit_notification_warning = get_config( 'submit_notification_warning_pre_review' diff --git a/src/olympia/hero/tests/test_models.py b/src/olympia/hero/tests/test_models.py index d26dc9f05f97..889a5547e384 100644 --- a/src/olympia/hero/tests/test_models.py +++ b/src/olympia/hero/tests/test_models.py @@ -41,10 +41,10 @@ def test_gradiant(self): assert ph.gradient == {'start': 'color-ink-80', 'end': 'color-pink-70'} def test_clean_requires_approved_can_primary_hero_group(self): - promoted = PromotedAddon.objects.create( - addon=addon_factory() - ) - PromotedAddonGroup.objects.create(promoted_addon=promoted, group_id=RECOMMENDED.id) + promoted = PromotedAddon.objects.create(addon=addon_factory()) + PromotedAddonGroup.objects.create( + promoted_addon=promoted, group_id=RECOMMENDED.id + ) ph = PrimaryHero.objects.create( promoted_addon=promoted, gradient_color='#C60184', @@ -64,7 +64,9 @@ def test_clean_requires_approved_can_primary_hero_group(self): ph.clean() # it raises if there's an error # change to a different group - PromotedAddonGroup.objects.filter(promoted_addon=ph.promoted_addon).update(group_id=STRATEGIC.id) + PromotedAddonGroup.objects.filter(promoted_addon=ph.promoted_addon).update( + group_id=STRATEGIC.id + ) ph.promoted_addon.approve_for_version(ph.promoted_addon.addon.current_version) ph.reload() ph.enabled = True @@ -78,7 +80,9 @@ def test_clean_requires_approved_can_primary_hero_group(self): ] # change to a different group that *can* be added as a primary hero - PromotedAddonGroup.objects.filter(promoted_addon=ph.promoted_addon).update(group_id=SPOTLIGHT.id) + PromotedAddonGroup.objects.filter(promoted_addon=ph.promoted_addon).update( + group_id=SPOTLIGHT.id + ) ph.promoted_addon.approve_for_version(ph.promoted_addon.addon.current_version) ph.reload() ph.enabled = True @@ -104,13 +108,11 @@ def test_clean_external_requires_homepage(self): def test_clean_gradient_and_image(self): # Currently, gradient is required and image isn't. - promoted = PromotedAddon.objects.create( - addon=addon_factory() - ) - PromotedAddonGroup.objects.create(promoted_addon=promoted, group_id=RECOMMENDED.id) - ph = PrimaryHero.objects.create( - promoted_addon=promoted + promoted = PromotedAddon.objects.create(addon=addon_factory()) + PromotedAddonGroup.objects.create( + promoted_addon=promoted, group_id=RECOMMENDED.id ) + ph = PrimaryHero.objects.create(promoted_addon=promoted) ph.promoted_addon.approve_for_version(ph.promoted_addon.addon.current_version) ph.reload() assert not ph.enabled @@ -131,10 +133,10 @@ def test_clean_gradient_and_image(self): ph.clean() # it raises if there's an error def test_clean_only_enabled(self): - promoted = PromotedAddon.objects.create( - addon=addon_factory() - ) - PromotedAddonGroup.objects.create(promoted_addon=promoted, group_id=RECOMMENDED.id) + promoted = PromotedAddon.objects.create(addon=addon_factory()) + PromotedAddonGroup.objects.create( + promoted_addon=promoted, group_id=RECOMMENDED.id + ) hero = PrimaryHero.objects.create( promoted_addon=promoted, gradient_color='#C60184', diff --git a/src/olympia/promoted/admin.py b/src/olympia/promoted/admin.py index aa2010720868..96f277119bce 100644 --- a/src/olympia/promoted/admin.py +++ b/src/olympia/promoted/admin.py @@ -57,6 +57,7 @@ def get_queryset(self, request): ) return qs + class PromotedAddonGroupInline(admin.TabularInline): model = PromotedAddonGroup fields = ('group_id',) @@ -75,6 +76,7 @@ def get_queryset(self, request): qs = qs.filter(promoted_addon=self.instance) return qs + class PromotedAddonAdmin(AMOModelAdmin): list_display = ( 'addon__name', diff --git a/src/olympia/promoted/models.py b/src/olympia/promoted/models.py index 52a3725f31ad..20187a4b2ed7 100644 --- a/src/olympia/promoted/models.py +++ b/src/olympia/promoted/models.py @@ -1,9 +1,9 @@ +from django.core.exceptions import ValidationError from django.db import models from django.dispatch import receiver from olympia.addons.models import Addon from olympia.amo.models import ManagerBase, ModelBase -from django.core.exceptions import ValidationError from olympia.constants.applications import APP_IDS, APP_USAGE, APPS_CHOICES from olympia.constants.promoted import ( NOT_PROMOTED, @@ -13,10 +13,12 @@ from olympia.reviewers.models import NeedsHumanReview from olympia.versions.models import Version + class PromotedAddonManager(ManagerBase): def get_queryset(self): return super().get_queryset().prefetch_related('promoted_groups') + class PromotedAddon(ModelBase): APPLICATION_CHOICES = ((None, 'All Applications'),) + APPS_CHOICES addon = models.OneToOneField( @@ -41,7 +43,7 @@ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) def __str__(self): - return f"{self.name} - {self.addon}" + return f'{self.name} - {self.addon}' @property def badged(self): @@ -57,34 +59,45 @@ def groups(self): Return the set of groups. """ return self.promoted_groups.all() - + @property def group_ids(self): """ Return the list of group ids. """ return list(self.groups.values_list('group_id', flat=True)) - + @property def not_promoted(self): """ Return whether the PromotedAddon is not promoted. """ return not self.groups - + def in_group(self, group): """ Return whether the PromotedAddon is part of the given group. """ return group.id in self.group_ids - - def check_permission(self, permission, truthy=True): + + def check_permission(self, permission, version=None, truthy=True): """ - Whether any promoted group the add-on is apart of has the associated permission. - When truthy is passed, checks for that boolean value. + Whether any promoted group the add-on is apart of has the associated permission. + + If version is passed, only returns True if the associated group is approved + for the given version. + + If truthy is passed, checks for that boolean value. """ - return any(group.check_permission(permission, truthy) for group in self.groups) - + return any( + group.check_permission(permission, truthy) + for group in self.groups + if not version + or PromotedApproval.objects.filter( + version=version, group_id=group.group_id + ).exists() + ) + @property def all_applications(self): application = APP_IDS.get(self.application_id) @@ -117,8 +130,9 @@ def _get_application_id_from_applications(self, apps): def approve_for_version(self, version, group_id=None): """Create PromotedApproval for current applications in the given promoted group. - - If group_id is passed, only approve group_id. otherwise, approve all in self.groups. + + If group_id is passed, only approve group_id. + Otherwise, approve all in self.groups. """ for app in self.all_applications: group_ids = [group_id] if group_id else self.group_ids @@ -154,11 +168,13 @@ def save(self, *args, **kwargs): due_date=due_date, reason=NeedsHumanReview.REASONS.ADDED_TO_PROMOTED_GROUP, ) - + class PromotedAddonGroup(ModelBase): GROUP_CHOICES = [(group.id, group.name) for group in PROMOTED_GROUPS] - promoted_addon = models.ForeignKey(PromotedAddon, on_delete=models.CASCADE, related_name='promoted_groups') + promoted_addon = models.ForeignKey( + PromotedAddon, on_delete=models.CASCADE, related_name='promoted_groups' + ) group_id = models.SmallIntegerField( choices=GROUP_CHOICES, verbose_name='Group', @@ -174,16 +190,25 @@ def badged(self): def check_permission(self, permission, truthy=True): result = getattr(PROMOTED_GROUPS_BY_ID[self.group_id], permission, False) return result if truthy else not result + class Meta: constraints = [ models.UniqueConstraint( fields=['group_id', 'promoted_addon'], name='promoted_addon_group_id' ) ] + def clean(self): - addon_groups = PromotedAddonGroup.objects.filter(promoted_addon=self.promoted_addon) - if addon_groups.filter(group_id=NOT_PROMOTED.id).exists() and addon_groups.len() > 1: - raise ValidationError('User cannot be promoted and not promoted at the same time.') + addon_groups = PromotedAddonGroup.objects.filter( + promoted_addon=self.promoted_addon + ) + if ( + addon_groups.filter(group_id=NOT_PROMOTED.id).exists() + and addon_groups.len() > 1 + ): + raise ValidationError( + 'User cannot be promoted and not promoted at the same time.' + ) super().clean() diff --git a/src/olympia/promoted/tasks.py b/src/olympia/promoted/tasks.py index a653cdb42e5f..52dbaf99581e 100644 --- a/src/olympia/promoted/tasks.py +++ b/src/olympia/promoted/tasks.py @@ -1,4 +1,5 @@ from django.db.models import Q + import olympia.core.logger from olympia import amo from olympia.addons.models import Addon diff --git a/src/olympia/promoted/tests/test_admin.py b/src/olympia/promoted/tests/test_admin.py index 64b3c3682a36..21a8920f58d6 100644 --- a/src/olympia/promoted/tests/test_admin.py +++ b/src/olympia/promoted/tests/test_admin.py @@ -522,9 +522,7 @@ def test_can_delete_when_primary_hero_too(self): assert PromotedApproval.objects.filter().exists() def test_updates_not_promoted_to_line(self): - item = PromotedAddon.objects.create( - addon=addon_factory() - ) + item = PromotedAddon.objects.create(addon=addon_factory()) detail_url = reverse(self.detail_url_name, args=(item.pk,)) user = user_factory(email='someone@mozilla.com') self.grant_permission(user, 'Discovery:Edit') @@ -542,4 +540,9 @@ def test_updates_not_promoted_to_line(self): item.reload() assert response.status_code == 200 - assert PromotedAddonGroup.objects.filter(promoted_addon=item, group_id=LINE.id).count() == 1 + assert ( + PromotedAddonGroup.objects.filter( + promoted_addon=item, group_id=LINE.id + ).count() + == 1 + ) diff --git a/src/olympia/promoted/tests/test_models.py b/src/olympia/promoted/tests/test_models.py index e6cc9d1f4833..dd6d8bcbc26b 100644 --- a/src/olympia/promoted/tests/test_models.py +++ b/src/olympia/promoted/tests/test_models.py @@ -16,10 +16,10 @@ def setUp(self): self.task_user = user_factory(pk=settings.TASK_USER_ID) def test_basic(self): - promoted_addon = PromotedAddon.objects.create( - addon=addon_factory() + promoted_addon = PromotedAddon.objects.create(addon=addon_factory()) + PromotedAddonGroup.objects.create( + promoted_addon=promoted_addon, group_id=promoted.LINE.id ) - PromotedAddonGroup.objects.create(promoted_addon=promoted_addon, group_id=promoted.LINE.id) assert promoted_addon.in_group(promoted.LINE) assert promoted_addon.application_id is None assert promoted_addon.all_applications == [ @@ -32,10 +32,10 @@ def test_basic(self): def test_is_approved_applications(self): addon = addon_factory() - promoted_addon = PromotedAddon.objects.create( - addon=addon + promoted_addon = PromotedAddon.objects.create(addon=addon) + PromotedAddonGroup.objects.create( + promoted_addon=promoted_addon, group_id=promoted.LINE.id ) - PromotedAddonGroup.objects.create(promoted_addon=promoted_addon, group_id=promoted.LINE.id) assert addon.promotedaddon # Just having the PromotedAddon instance isn't enough assert addon.promotedaddon.approved_applications == [] @@ -49,7 +49,9 @@ def test_is_approved_applications(self): ] # but not if it's for a different type of promotion - PromotedAddonGroup.objects.filter(promoted_addon=promoted_addon).update(group_id=promoted.SPOTLIGHT.id) + PromotedAddonGroup.objects.filter(promoted_addon=promoted_addon).update( + group_id=promoted.SPOTLIGHT.id + ) assert addon.promotedaddon.approved_applications == [] # unless that group has an approval too PromotedApproval.objects.create( @@ -63,7 +65,9 @@ def test_is_approved_applications(self): # for promoted groups that don't require pre-review though, there isn't # a per version approval, so a current_version is sufficient and all # applications are seen as approved. - PromotedAddonGroup.objects.filter(promoted_addon=promoted_addon).update(group_id=promoted.STRATEGIC.id) + PromotedAddonGroup.objects.filter(promoted_addon=promoted_addon).update( + group_id=promoted.STRATEGIC.id + ) assert addon.promotedaddon.approved_applications == [ applications.FIREFOX, applications.ANDROID, @@ -79,7 +83,9 @@ def test_auto_approves_addon_when_saved_for_immediate_approval(self): assert not PromotedApproval.objects.exists() # first test with a group.immediate_approval == False - PromotedAddonGroup.objects.create(promoted_addon=promo, group_id=promoted.RECOMMENDED.id) + PromotedAddonGroup.objects.create( + promoted_addon=promo, group_id=promoted.RECOMMENDED.id + ) promo.save() promo.addon.reload() assert promo.approved_applications == [] @@ -87,7 +93,9 @@ def test_auto_approves_addon_when_saved_for_immediate_approval(self): assert not promo.addon.promoted_groups() # then with a group thats immediate_approval == True - PromotedAddonGroup.objects.filter(promoted_addon=promo).update(group_id=promoted.SPOTLIGHT.id) + PromotedAddonGroup.objects.filter(promoted_addon=promo).update( + group_id=promoted.SPOTLIGHT.id + ) promo.save() promo.addon.reload() assert promo.approved_applications == [amo.FIREFOX] @@ -114,7 +122,9 @@ def test_addon_flagged_for_human_review_when_saved(self): assert not PromotedApproval.objects.exists() # first test with a group.flag_for_human_review == False - PromotedAddonGroup.objects.create(promoted_addon=promo, group_id=promoted.RECOMMENDED.id) + PromotedAddonGroup.objects.create( + promoted_addon=promo, group_id=promoted.RECOMMENDED.id + ) promo.save() promo.addon.reload() assert promo.approved_applications == [] @@ -131,7 +141,9 @@ def test_addon_flagged_for_human_review_when_saved(self): listed_ver.file.update(is_signed=True) unlisted_ver.file.update(is_signed=True) promo.addon.reload() - PromotedAddonGroup.objects.filter(promoted_addon=promo).update(group_id=promoted.NOTABLE.id) + PromotedAddonGroup.objects.filter(promoted_addon=promo).update( + group_id=promoted.NOTABLE.id + ) promo.save() promo.addon.reload() assert promo.approved_applications == [] # doesn't approve immediately @@ -150,7 +162,9 @@ def test_addon_flagged_for_human_review_when_saved(self): listed_ver.file.update(is_signed=False) unlisted_ver.file.update(is_signed=False) promo.addon.reload() - PromotedAddonGroup.objects.filter(promoted_addon=promo).update(group_id=promoted.NOTABLE.id) + PromotedAddonGroup.objects.filter(promoted_addon=promo).update( + group_id=promoted.NOTABLE.id + ) promo.save() promo.addon.reload() assert promo.approved_applications == [] # doesn't approve immediately @@ -167,7 +181,9 @@ def test_addon_flagged_for_human_review_when_saved(self): listed_ver.file.update(is_signed=True) unlisted_ver.file.update(is_signed=True) promo.addon.reload() - PromotedAddonGroup.objects.filter(promoted_addon=promo).update(group_id=promoted.NOTABLE.id) + PromotedAddonGroup.objects.filter(promoted_addon=promo).update( + group_id=promoted.NOTABLE.id + ) promo.save() promo.addon.reload() assert promo.approved_applications == [] # doesn't approve immediately @@ -191,10 +207,10 @@ def test_disabled_and_deleted_versions_flagged_for_human_review(self): file_kw={'status': amo.STATUS_DISABLED, 'is_signed': True} ) version = addon.find_latest_version(None, exclude=(), deleted=True) - promo = PromotedAddon.objects.create( - addon=addon, application_id=amo.FIREFOX.id + promo = PromotedAddon.objects.create(addon=addon, application_id=amo.FIREFOX.id) + PromotedAddonGroup.objects.create( + promoted_addon=promo, group_id=promoted.NOTABLE.id ) - PromotedAddonGroup.objects.create(promoted_addon=promo, group_id=promoted.NOTABLE.id) assert not promo.addon.promoted_groups() self.assertCloseToNow(version.reload().due_date, now=get_review_due_date()) assert version.needshumanreview_set.filter(is_active=True).count() == 1 @@ -239,10 +255,11 @@ def test_approve_for_addon(self): file_kw={'filename': 'webextension.xpi'}, ) ) - PromotedAddonGroup.objects.create(promoted_addon=promo, group_id=promoted.SPOTLIGHT.id) + PromotedAddonGroup.objects.create( + promoted_addon=promo, group_id=promoted.SPOTLIGHT.id + ) # SPOTLIGHT doesnt have special signing states so won't be resigned # approve_for_addon is called automatically - SPOTLIGHT has immediate_approval promo.addon.reload() assert promo.addon.promoted_groups().in_group(promoted.SPOTLIGHT) assert promo.addon.current_version.version == '0.123a' - diff --git a/src/olympia/promoted/tests/test_tasks.py b/src/olympia/promoted/tests/test_tasks.py index 5793b1be5d67..d9089d673fcd 100644 --- a/src/olympia/promoted/tests/test_tasks.py +++ b/src/olympia/promoted/tests/test_tasks.py @@ -103,14 +103,16 @@ def test_add_high_adu_extensions_to_notable(): extension_with_high_adu.reload().promoted_groups(currently_approved=False) == NOTABLE ) - assert ( - ignored_theme.reload().promoted_groups(currently_approved=False).not_promoted - ) + assert ignored_theme.reload().promoted_groups(currently_approved=False).not_promoted already_promoted.reload().promotedaddon.reload() assert already_promoted.promoted_groups(currently_approved=False).in_group(LINE) promoted_record_exists.reload().promotedaddon.reload() - assert promoted_record_exists.promoted_groups(currently_approved=False).in_group(NOTABLE) - assert unlisted_only_extension.promoted_groups(currently_approved=False).in_group(NOTABLE) + assert promoted_record_exists.promoted_groups(currently_approved=False).in_group( + NOTABLE + ) + assert unlisted_only_extension.promoted_groups(currently_approved=False).in_group( + NOTABLE + ) assert mixed_extension.promoted_groups(currently_approved=False).in_group(NOTABLE) assert deleted_extension.promoted_groups(currently_approved=False).in_group(NOTABLE) diff --git a/src/olympia/reviewers/models.py b/src/olympia/reviewers/models.py index 3d6449155adb..99fe072dd197 100644 --- a/src/olympia/reviewers/models.py +++ b/src/olympia/reviewers/models.py @@ -11,7 +11,6 @@ from extended_choices import Choices -from olympia.constants.promoted import PROMOTED_GROUPS_BY_ID import olympia.core.logger from olympia import activity, amo, core from olympia.abuse.models import AbuseReport, CinderPolicy @@ -20,6 +19,7 @@ from olympia.amo.models import ModelBase from olympia.amo.templatetags.jinja_helpers import absolutify from olympia.amo.utils import send_mail +from olympia.constants.promoted import PROMOTED_GROUPS_BY_ID from olympia.files.models import File, FileValidation from olympia.ratings.models import Rating from olympia.users.models import UserProfile @@ -148,8 +148,8 @@ def get_flags(addon, version): ] # add in the promoted group flags and return if promoted := addon.promoted_groups(currently_approved=False): - for promoted in promoted.groups: - group = PROMOTED_GROUPS_BY_ID[promoted.group_id] + for promo in promoted.groups: + group = PROMOTED_GROUPS_BY_ID[promo.group_id] flags.append((f'promoted-{group.api_name}', group.name)) return flags @@ -638,12 +638,15 @@ def check_is_promoted_prereview(cls, version): and ( ( version.channel == amo.CHANNEL_LISTED - and version.addon.check_permission('listed_pre_review', currently_approved=False) + and version.addon.check_permission( + 'listed_pre_review', currently_approved=False + ) ) or ( version.channel == amo.CHANNEL_UNLISTED - and version.addon.check_permission('unlisted_pre_review', currently_approved=False) - + and version.addon.check_permission( + 'unlisted_pre_review', currently_approved=False + ) ) ) ) diff --git a/src/olympia/reviewers/tests/test_models.py b/src/olympia/reviewers/tests/test_models.py index e3aabd73461e..bb234177657b 100644 --- a/src/olympia/reviewers/tests/test_models.py +++ b/src/olympia/reviewers/tests/test_models.py @@ -1199,19 +1199,27 @@ def test_check_is_promoted_prereview(self): promoted = PromotedAddon.objects.create(addon=self.addon) assert AutoApprovalSummary.check_is_promoted_prereview(self.version) is False - PromotedAddonGroup.objects.create(promoted_addon=promoted, group_id=RECOMMENDED.id) + PromotedAddonGroup.objects.create( + promoted_addon=promoted, group_id=RECOMMENDED.id + ) assert AutoApprovalSummary.check_is_promoted_prereview(self.version) is True - PromotedAddonGroup.objects.filter(promoted_addon=promoted).update(group_id=STRATEGIC.id) # STRATEGIC isn't prereview + PromotedAddonGroup.objects.filter(promoted_addon=promoted).update( + group_id=STRATEGIC.id + ) # STRATEGIC isn't prereview assert AutoApprovalSummary.check_is_promoted_prereview(self.version) is False - PromotedAddonGroup.objects.filter(promoted_addon=promoted).update(group_id=LINE.id) # LINE is though + PromotedAddonGroup.objects.filter(promoted_addon=promoted).update( + group_id=LINE.id + ) # LINE is though assert AutoApprovalSummary.check_is_promoted_prereview(self.version) is True self.version.update(channel=amo.CHANNEL_UNLISTED) # not for unlisted though assert AutoApprovalSummary.check_is_promoted_prereview(self.version) is False - PromotedAddonGroup.objects.filter(promoted_addon=promoted).update(group_id=NOTABLE.id) # NOTABLE is + PromotedAddonGroup.objects.filter(promoted_addon=promoted).update( + group_id=NOTABLE.id + ) # NOTABLE is assert AutoApprovalSummary.check_is_promoted_prereview(self.version) is True self.version.update(channel=amo.CHANNEL_LISTED) # and for listed too diff --git a/src/olympia/reviewers/tests/test_utils.py b/src/olympia/reviewers/tests/test_utils.py index 97ce8fdaf509..129140dfde4a 100644 --- a/src/olympia/reviewers/tests/test_utils.py +++ b/src/olympia/reviewers/tests/test_utils.py @@ -3085,7 +3085,9 @@ def test_autoapprove_fails_for_promoted(self): assert not self.addon.promoted_groups() # change to other type of promoted; same should happen - PromotedAddonGroup.objects.filter(promoted_addon=self.addon.promotedaddon).update(group_id=LINE.id) + PromotedAddonGroup.objects.filter( + promoted_addon=self.addon.promotedaddon + ).update(group_id=LINE.id) with self.assertRaises(AssertionError): self.test_nomination_to_public() assert not PromotedApproval.objects.filter( @@ -3094,7 +3096,9 @@ def test_autoapprove_fails_for_promoted(self): assert not self.addon.promoted_groups() # except for a group that doesn't require prereview - PromotedAddonGroup.objects.filter(promoted_addon=self.addon.promotedaddon).update(group_id=STRATEGIC.id) + PromotedAddonGroup.objects.filter( + promoted_addon=self.addon.promotedaddon + ).update(group_id=STRATEGIC.id) assert self.addon.promoted_groups().in_group(STRATEGIC) self.test_nomination_to_public() # But no promotedapproval though diff --git a/src/olympia/reviewers/utils.py b/src/olympia/reviewers/utils.py index 0da1acb64892..e37bf360e689 100644 --- a/src/olympia/reviewers/utils.py +++ b/src/olympia/reviewers/utils.py @@ -495,7 +495,12 @@ def get_actions(self): if version_is_unlisted: can_reject_multiple = is_appropriate_reviewer can_approve_multiple = is_appropriate_reviewer - elif self.content_review or promoted_groups and promoted_groups.check_permission('listed_pre_review') or is_static_theme: + elif ( + self.content_review + or promoted_groups + and promoted_groups.check_permission('listed_pre_review') + or is_static_theme + ): can_reject_multiple = ( addon_is_valid_and_version_is_listed and is_appropriate_reviewer ) @@ -733,7 +738,13 @@ def get_actions(self): 'available': ( self.version is not None and is_reviewer - and (not (promoted_groups and promoted_groups.check_permission('admin_review')) or is_appropriate_reviewer) + and ( + not ( + promoted_groups + and promoted_groups.check_permission('admin_review') + ) + or is_appropriate_reviewer + ) ), 'allows_reasons': not is_static_theme, 'requires_reasons': False, @@ -904,8 +915,14 @@ def set_promoted(self, versions=None): return channel = versions[0].channel if group and ( - (channel == amo.CHANNEL_LISTED and group.check_permission('listed_pre_review')) - or (channel == amo.CHANNEL_UNLISTED and group.check_permission('unlisted_pre_review')) + ( + channel == amo.CHANNEL_LISTED + and group.check_permission('listed_pre_review') + ) + or ( + channel == amo.CHANNEL_UNLISTED + and group.check_permission('unlisted_pre_review') + ) ): # These addons shouldn't be be attempted for auto approval anyway, # but double check that the cron job isn't trying to approve it. diff --git a/src/olympia/shelves/tests/test_views.py b/src/olympia/shelves/tests/test_views.py index d8717d8ea087..8aaa0e7cdfbd 100644 --- a/src/olympia/shelves/tests/test_views.py +++ b/src/olympia/shelves/tests/test_views.py @@ -69,15 +69,19 @@ def setUpTestData(cls): summary=None, ) - promoted = PromotedAddon.objects.create( - addon=addon_ext - ).approve_for_version(version=addon_ext.current_version) - PromotedAddonGroup.objects.create(promoted_addon=promoted, group_id=RECOMMENDED.id) - - promoted = PromotedAddon.objects.create( - addon=addon_theme - ).approve_for_version(version=addon_theme.current_version) - PromotedAddonGroup.objects.create(promoted_addon=promoted, group_id=RECOMMENDED.id) + promoted = PromotedAddon.objects.create(addon=addon_ext).approve_for_version( + version=addon_ext.current_version + ) + PromotedAddonGroup.objects.create( + promoted_addon=promoted, group_id=RECOMMENDED.id + ) + + promoted = PromotedAddon.objects.create(addon=addon_theme).approve_for_version( + version=addon_theme.current_version + ) + PromotedAddonGroup.objects.create( + promoted_addon=promoted, group_id=RECOMMENDED.id + ) user = UserProfile.objects.create(pk=settings.TASK_USER_ID) collection = collection_factory(author=user, slug='privacy-matters') diff --git a/src/olympia/versions/models.py b/src/olympia/versions/models.py index 2de8a39ec4c9..fe38d77884a3 100644 --- a/src/olympia/versions/models.py +++ b/src/olympia/versions/models.py @@ -1073,7 +1073,10 @@ def can_be_disabled_and_deleted(self): if self != self.addon.current_version or ( not (group := self.addon.promoted_groups()) - or not (group.badged and group.check_permission('listed_pre_review')) + or not ( + group.check_permission('badged') + and group.check_permission('listed_pre_review') + ) ): return True @@ -1091,7 +1094,7 @@ def can_be_disabled_and_deleted(self): .distinct()[:1] ) previous_approval = PromotedApproval.objects.filter( - group_id=group.id, version__in=previous_version + group_id__in=group.group_ids, version__in=previous_version ) return previous_approval.exists() @@ -1574,7 +1577,7 @@ def version_range_contains_forbidden_compatibility(self): # Recommended/line for Android are allowed to set compatibility in the # limited range. - if addon.check_permission('can_be_compatible_with_all_fenix_versions'): + if addon.can_be_compatible_with_all_fenix_versions: return False # Range is expressed as a [closed, open) interval. diff --git a/src/olympia/versions/tests/test_models.py b/src/olympia/versions/tests/test_models.py index 1fe80f12ec0f..907db6886334 100644 --- a/src/olympia/versions/tests/test_models.py +++ b/src/olympia/versions/tests/test_models.py @@ -408,17 +408,19 @@ def test_should_have_due_date(self): # Or if it's in a pre-review promoted group it will. recommended = addon_factory(**addon_kws).current_version promoted = PromotedAddon.objects.create(addon=recommended.addon) - PromotedAddonGroup.objects.create(promoted_addon=promoted, group_id=RECOMMENDED.id) + PromotedAddonGroup.objects.create( + promoted_addon=promoted, group_id=RECOMMENDED.id + ) NeedsHumanReview.objects.create( version=recommended, reason=NeedsHumanReview.REASONS.BELONGS_TO_PROMOTED_GROUP, ) # And not if it's a non-pre-review group - PromotedAddon.objects.create( - addon=addon_factory(**addon_kws) + PromotedAddon.objects.create(addon=addon_factory(**addon_kws)) + PromotedAddonGroup.objects.create( + promoted_addon=promoted, group_id=STRATEGIC.id ) - PromotedAddonGroup.objects.create(promoted_addon=promoted, group_id=STRATEGIC.id) # A disabled version with a developer reply developer_reply = addon_factory( @@ -1030,7 +1032,6 @@ def test_should_have_due_date_listed(self): # But if it has a NHR it will. nhr = version.needshumanreview_set.create( reason=NeedsHumanReview.REASONS.BELONGS_TO_PROMOTED_GROUP - ) assert version.should_have_due_date @@ -1328,14 +1329,18 @@ def test_unbadged_non_prereview_promoted_can_be_disabled_and_deleted(self): assert not addon.current_version.can_be_disabled_and_deleted() # STRATEGIC isn't pre-reviewd or badged, so it's okay though - PromotedAddonGroup.objects.filter(promoted_addon=addon.promotedaddon).update(group_id=STRATEGIC.id) + PromotedAddonGroup.objects.filter(promoted_addon=addon.promotedaddon).update( + group_id=STRATEGIC.id + ) addon.current_version.promoted_approvals.update(group_id=STRATEGIC.id) del addon.current_version.approved_for_groups assert addon.promoted_groups().in_group(STRATEGIC) assert addon.current_version.can_be_disabled_and_deleted() # SPOTLIGHT is pre-reviewed but not badged, so it's okay too - PromotedAddonGroup.objects.filter(promoted_addon=addon.promotedaddon).update(group_id=SPOTLIGHT.id) + PromotedAddonGroup.objects.filter(promoted_addon=addon.promotedaddon).update( + group_id=SPOTLIGHT.id + ) addon.current_version.promoted_approvals.update(group_id=SPOTLIGHT.id) assert addon.promoted_groups().in_group(SPOTLIGHT) assert addon.current_version.can_be_disabled_and_deleted() @@ -1345,10 +1350,12 @@ def test_can_be_disabled_and_deleted_querycount(self): version_factory(addon=addon) self.make_addon_promoted(addon, RECOMMENDED, approve_version=True) addon.reload() - with self.assertNumQueries(3): + with self.assertNumQueries(7): # 1. check the addon's promoted group # 2. check addon.current_version is approved for that group # 3. check the previous version is approved for that group + # 4. Prefetching the related PromotedAddonGroups + # 5. PromotedApproval fetch assert not addon.current_version.can_be_disabled_and_deleted() def test_is_blocked(self):