From 4e75d2ffa1a8c6706a87c97f8586f572955f1e87 Mon Sep 17 00:00:00 2001 From: Andrew Williamson Date: Thu, 7 May 2026 16:58:26 +0100 Subject: [PATCH] cache existing Blocks so reversing delayed block actions doesn't fail --- src/olympia/abuse/actions.py | 16 ++++++++++++++-- src/olympia/abuse/tests/test_actions.py | 22 ++++++++++++++++++++++ src/olympia/blocklist/models.py | 3 ++- src/olympia/blocklist/tests/test_utils.py | 8 ++++---- src/olympia/blocklist/utils.py | 5 ++--- 5 files changed, 44 insertions(+), 10 deletions(-) diff --git a/src/olympia/abuse/actions.py b/src/olympia/abuse/actions.py index 852505d2c24d..37e3de37662f 100644 --- a/src/olympia/abuse/actions.py +++ b/src/olympia/abuse/actions.py @@ -19,7 +19,7 @@ from olympia.amo.templatetags.jinja_helpers import absolutify from olympia.amo.utils import send_mail from olympia.bandwagon.models import Collection -from olympia.blocklist.models import BlocklistSubmission, BlockType +from olympia.blocklist.models import Block, BlocklistSubmission, BlockType from olympia.blocklist.utils import delete_versions_from_blocks, save_versions_to_blocks from olympia.constants.abuse import DECISION_ACTIONS from olympia.constants.permissions import ADDONS_HIGH_IMPACT_APPROVE @@ -742,6 +742,18 @@ def __init__(self, decision): f'{self.__class__.__name__} requires delay_days to be set' ) + @classmethod + def get_existing_blocks_from_decision(cls, decision): + """Get existing blocks for the decision's target and cache them on the decision. + We're caching them on the decision to avoid hitting the database multiple times + when reversing a decision, and because get_blocks_from_guids uses replicas, so + the data can be stale when we're running in a transaction""" + if not hasattr(decision, '_existing_blocks'): + decision._existing_blocks = Block.get_blocks_from_guids( + [decision.target.guid] + ) + return decision._existing_blocks + def process_action(self, release_hold=False): versions_qs = self.versions_block_will_affect # if this is a followup action, and the primary action is rejecting specific @@ -795,7 +807,7 @@ def reverse_action(cls, original_decision): ) ) delete_versions_from_blocks( - [guid], + cls.get_existing_blocks_from_decision(original_decision), BlocklistSubmission( input_guids=guid, action=BlocklistSubmission.ACTIONS.DELETE, diff --git a/src/olympia/abuse/tests/test_actions.py b/src/olympia/abuse/tests/test_actions.py index 8ed345473d19..4078e63c6462 100644 --- a/src/olympia/abuse/tests/test_actions.py +++ b/src/olympia/abuse/tests/test_actions.py @@ -2285,6 +2285,28 @@ def test_approve_appeal_success_followup(self): ) self._test_approve_appeal_or_override(ContentActionTargetAppealApprove) + def test_approve_appeal_success_followup_with_multiple_followups(self): + self.past_negative_decision.update( + appeal_job=self.cinder_job, action=DECISION_ACTIONS.AMO_DISABLE_ADDON + ) + ContentDecisionFollowupAction.objects.create( + decision=self.past_negative_decision, + action=self.takedown_decision_action, + action_date=datetime.now(), + ) + # These follow-up actions are redundant, but shouldn't cause errors. + ContentDecisionFollowupAction.objects.create( + decision=self.past_negative_decision, + action=DECISION_ACTIONS.AMO_FU_DELAY_LONG_SOFT_BLOCK_ADDON, + action_date=datetime.now(), + ) + ContentDecisionFollowupAction.objects.create( + decision=self.past_negative_decision, + action=DECISION_ACTIONS.AMO_FU_DELAY_LONG_HARD_BLOCK_ADDON, + action_date=datetime.now(), + ) + self._test_approve_appeal_or_override(ContentActionTargetAppealApprove) + def test_approve_override_success_followup(self): self.decision.update(override_of=self.past_negative_decision) self.past_negative_decision.update(action=DECISION_ACTIONS.AMO_DISABLE_ADDON) diff --git a/src/olympia/blocklist/models.py b/src/olympia/blocklist/models.py index 6240e354b26b..81c2ff26e5fa 100644 --- a/src/olympia/blocklist/models.py +++ b/src/olympia/blocklist/models.py @@ -536,7 +536,8 @@ def delete_block_objects(self): for guids_chunk in chunked(all_guids_to_block, 100): # This function will remove BlockVersions and delete the Block if empty - delete_versions_from_blocks(guids_chunk, self) + blocks = Block.get_blocks_from_guids(guids_chunk) + delete_versions_from_blocks(blocks, self) self.save() self.update(signoff_state=self.SIGNOFF_STATES.PUBLISHED) diff --git a/src/olympia/blocklist/tests/test_utils.py b/src/olympia/blocklist/tests/test_utils.py index 00074903160f..917e0d1f6ba4 100644 --- a/src/olympia/blocklist/tests/test_utils.py +++ b/src/olympia/blocklist/tests/test_utils.py @@ -430,7 +430,7 @@ def test_activity_attributed_to_user_deleting_the_block(self): addon = addon_factory() version_factory(addon=addon) user = user_factory() - block_factory(guid=addon.guid, updated_by=self.task_user) + block = block_factory(guid=addon.guid, updated_by=self.task_user) changed_version_ids = list( addon.versions.values_list('pk', flat=True).order_by('pk') ) @@ -445,7 +445,7 @@ def test_activity_attributed_to_user_deleting_the_block(self): signoff_state=BlocklistSubmission.SIGNOFF_STATES.PUBLISHED, ) ActivityLog.objects.all().delete() - delete_versions_from_blocks([addon.guid], submission) + delete_versions_from_blocks([block], submission) for version in addon.versions.all(): assert not version.is_blocked @@ -468,7 +468,7 @@ def test_activity_attributed_to_user_deleting_the_block_with_signoff(self): version_factory(addon=addon) user = user_factory() signoffer = user_factory() - block_factory(guid=addon.guid, updated_by=self.task_user) + block = block_factory(guid=addon.guid, updated_by=self.task_user) changed_version_ids = list( addon.versions.values_list('pk', flat=True).order_by('pk') ) @@ -484,7 +484,7 @@ def test_activity_attributed_to_user_deleting_the_block_with_signoff(self): signoff_by=signoffer, ) ActivityLog.objects.all().delete() - delete_versions_from_blocks([addon.guid], submission) + delete_versions_from_blocks([block], submission) for version in addon.versions.all(): assert not version.is_blocked diff --git a/src/olympia/blocklist/utils.py b/src/olympia/blocklist/utils.py index c4c91a04744a..8a49f9881020 100644 --- a/src/olympia/blocklist/utils.py +++ b/src/olympia/blocklist/utils.py @@ -257,12 +257,11 @@ def save_versions_to_blocks(guids, submission): return blocks -def delete_versions_from_blocks(guids, submission): - from .models import Block, BlockVersion +def delete_versions_from_blocks(blocks, submission): + from .models import BlockVersion modified_datetime = datetime.now() - blocks = Block.get_blocks_from_guids(guids) should_override_block_metadata = not submission.preserve_block_metadata for block in blocks: if not block.id: