Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't flag add-ons for growth threshold if hotness is negative #23031

Merged
merged 2 commits into from
Jan 30, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions src/olympia/addons/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 7 additions & 5 deletions src/olympia/addons/tests/test_tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
22 changes: 10 additions & 12 deletions src/olympia/reviewers/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
26 changes: 26 additions & 0 deletions src/olympia/reviewers/tests/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
)
Loading