From ad4569bd97942175d9d27a3dc71007732ce2606d Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Thu, 30 Jan 2025 13:56:11 +0100 Subject: [PATCH] Don't flag add-ons for growth threshold if hotness is negative (#23031) * Don't flag add-ons for growth threshold if hotness is negative --- src/olympia/addons/tasks.py | 2 -- src/olympia/addons/tests/test_tasks.py | 12 +++++----- src/olympia/reviewers/models.py | 22 +++++++++--------- src/olympia/reviewers/tests/test_models.py | 26 ++++++++++++++++++++++ 4 files changed, 43 insertions(+), 19 deletions(-) diff --git a/src/olympia/addons/tasks.py b/src/olympia/addons/tasks.py index 46d69917ad40..848b8fa58990 100644 --- a/src/olympia/addons/tasks.py +++ b/src/olympia/addons/tasks.py @@ -540,8 +540,6 @@ def flag_high_hotness_according_to_review_tier(): base_qs = UsageTier.get_base_addons() hotness_per_tier_filters = Q() for usage_tier in usage_tiers: - # Note: get_growth_threshold_q_object can return an empty Q() object - # if the average hotness for that tier is exactly 0. hotness_per_tier_filters |= usage_tier.get_growth_threshold_q_object() if not hotness_per_tier_filters: return diff --git a/src/olympia/addons/tests/test_tasks.py b/src/olympia/addons/tests/test_tasks.py index 3b3d278991d8..7f62927badec 100644 --- a/src/olympia/addons/tests/test_tasks.py +++ b/src/olympia/addons/tests/test_tasks.py @@ -451,13 +451,15 @@ def test_flag_high_hotness_according_to_review_tier_threshold_check_negative(): upper_adu_threshold=1000, growth_threshold_before_flagging=10, ) - # Average hotness should be negative, -0.1. + # Average hotness should be negative, -0.2. # growth_threshold_before_flagging for that tier is 10 so we should flag - # add-ons with hotness above 0 + # add-ons with hotness above -0.1, but we explicitly only flag positive + # growth add-ons, so only the last one should be flagged. - addon_factory(average_daily_users=251, hotness=-0.1) - addon_factory(average_daily_users=251, hotness=-0.2) - addon = addon_factory(average_daily_users=251, hotness=0.0) + addon_factory(average_daily_users=251, hotness=-0.05) + addon_factory(average_daily_users=251, hotness=-0.85) + addon_factory(average_daily_users=251, hotness=0.0) + addon = addon_factory(average_daily_users=251, hotness=0.1) File.objects.update(is_signed=True) flag_high_hotness_according_to_review_tier() diff --git a/src/olympia/reviewers/models.py b/src/olympia/reviewers/models.py index 7461660e52e9..4ab23f617a26 100644 --- a/src/olympia/reviewers/models.py +++ b/src/olympia/reviewers/models.py @@ -832,28 +832,26 @@ def average_growth(self): self.get_base_addons() .filter(**self.get_tier_boundaries()) .aggregate(Avg('hotness', default=0)) - .get('hotness__avg') + .get('hotness__avg', 0) ) def get_growth_threshold(self): - """Return the growth threshold for that tier, or None. + """Return the growth threshold for that tier. The value is computed from the average growth (hotness) of add-ons in that tier plus the growth_threshold_before_flagging converted to - decimal.""" - return self.average_growth + self.growth_threshold_before_flagging / 100 + decimal. + + It has a floor of 0.""" + return max(0, self.average_growth + self.growth_threshold_before_flagging / 100) def get_growth_threshold_q_object(self): """Return Q object containing filters to apply to find add-ons over the computed growth threshold for that tier.""" - hotness_ceiling_before_flagging = self.get_growth_threshold() - if hotness_ceiling_before_flagging is not None: - return Q( - hotness__gt=hotness_ceiling_before_flagging, - **self.get_tier_boundaries(), - ) - else: - return Q() + return Q( + hotness__gt=self.get_growth_threshold(), + **self.get_tier_boundaries(), + ) class NeedsHumanReview(ModelBase): diff --git a/src/olympia/reviewers/tests/test_models.py b/src/olympia/reviewers/tests/test_models.py index 5df5678d2a13..1b757547b3b2 100644 --- a/src/olympia/reviewers/tests/test_models.py +++ b/src/olympia/reviewers/tests/test_models.py @@ -1919,6 +1919,16 @@ def test_get_growth_threshold(self): del self.tier.average_growth assert round(self.tier.get_growth_threshold(), ndigits=2) == 0.77 + def test_get_growth_threshold_zero_floor_instead_of_negative(self): + addon_factory(hotness=-0.4, average_daily_users=100) + addon_factory(hotness=-0.4, average_daily_users=999) + assert round(self.tier.get_growth_threshold(), ndigits=2) == 0.1 + + addon_factory(hotness=-0.9, average_daily_users=999) + addon_factory(hotness=-0.9, average_daily_users=999) + del self.tier.average_growth + assert round(self.tier.get_growth_threshold(), ndigits=2) == 0 # Not -0.15 + def test_get_growth_threshold_q_object(self): addon_factory(hotness=0.01, average_daily_users=100) addon_factory(hotness=0.01, average_daily_users=999) @@ -1928,3 +1938,19 @@ def test_get_growth_threshold_q_object(self): list(Addon.objects.filter(self.tier.get_growth_threshold_q_object())) == expected ) + + def test_get_growth_threshold_q_object_hotness_needs_to_be_higher_than(self): + addon_factory(hotness=0.5, average_daily_users=999) + expected = [addon_factory(hotness=0.501, average_daily_users=999)] + + # Override computed average growth to force the growth threshold to + # 0.5 (0.0 + 50/100) + self.tier.average_growth = 0.0 + assert self.tier.get_growth_threshold() == 0.5 + + # We filter on hotness_gt in get_growth_threshold_q_object() so the + # first add-on shouldn't be returned, only the second one. + assert ( + list(Addon.objects.filter(self.tier.get_growth_threshold_q_object())) + == expected + )