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

always detect if there is an attachment for a decision #23138

Merged
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: 1 addition & 1 deletion src/olympia/abuse/actions.py
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@ def log_action(self, activity_log_action, *extra_args, extra_details=None):
activity_log__contentdecision__id=self.decision.id
).first():
attachment.update(activity_log=activity_log)
activity_log.attacmentlog = attachment # update fk
activity_log.attachmentlog = attachment # update fk
return activity_log

def process_action(self):
Expand Down
7 changes: 6 additions & 1 deletion src/olympia/abuse/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -1335,12 +1335,17 @@ def execute_action(self, *, release_hold=False):
return log_entry

def send_notifications(self):
from olympia.activity.models import AttachmentLog

if not self.action_date:
return

action_helper = self.get_action_helper()
cinder_job = self.originating_job
log_entry = self.activities.last()
has_attachment = AttachmentLog.objects.filter(
activity_log__contentdecisionlog__decision=self
).exists()

if cinder_job:
cinder_job.notify_reporters(action_helper)
Expand All @@ -1366,7 +1371,7 @@ def send_notifications(self):
'is_addon_disabled': details.get('is_addon_being_disabled')
or getattr(self.target, 'is_disabled', False),
'version_list': ', '.join(ver_str for ver_str in version_numbers),
'has_attachment': hasattr(log_entry, 'attachmentlog'),
'has_attachment': has_attachment,
'dev_url': absolutify(self.target.get_dev_url('versions'))
if self.addon_id
else None,
Expand Down
51 changes: 33 additions & 18 deletions src/olympia/abuse/tests/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -2886,23 +2886,6 @@ def test_report_to_cinder_approve_override_previous_decision_has_cinder_id(self)
expect_create_override_call=True,
)

def _check_notify_emails(self, decision, log_entry):
assert len(mail.outbox) == 1
assert mail.outbox[0].to == [decision.addon.authors.first().email]
assert str(log_entry.id) in mail.outbox[0].extra_headers['Message-ID']
assert 'days' not in mail.outbox[0].body
assert 'some review text' in mail.outbox[0].body
assert 'some policy text' not in mail.outbox[0].body
AttachmentLog.objects.create(
activity_log=log_entry,
file=ContentFile('Pseudo File', name='attachment.txt'),
)
decision.send_notifications()
assert 'An attachment was provided.' not in mail.outbox[0].body
assert 'To respond or view the file,' not in mail.outbox[0].body
assert 'An attachment was provided.' in mail.outbox[1].body
assert 'To respond or view the file,' in mail.outbox[1].body

Comment on lines -2889 to -2905
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, this is just refactoring because I realised _check_notify_emails was only used in one test, so it was confusing - I initially thought I could change here to test multiple tests. code has been cut&pasted into test_execute_action_with_action_date_already directly.

def _test_execute_action_ban_user_outcome(self, decision):
self.assertCloseToNow(decision.action_date)
self.assertCloseToNow(decision.user.reload().banned)
Expand Down Expand Up @@ -2970,12 +2953,19 @@ def test_execute_action_disable_addon_held(self):
alog = ActivityLog.objects.filter(
action=amo.LOG.HELD_ACTION_FORCE_DISABLE.id
).get()
# attachment is linked to the original decision activity log
AttachmentLog.objects.create(
activity_log=alog,
file=ContentFile('Pseudo File', name='attachment.txt'),
)
assert alog.contentdecisionlog_set.get().decision == decision
decision.send_notifications()
assert len(mail.outbox) == 0

decision.execute_action(release_hold=True)
self._test_execute_action_disable_addon_outcome(decision)
assert 'An attachment was provided.' in mail.outbox[0].body
assert 'To respond or view the file,' in mail.outbox[0].body

def test_execute_action_disable_addon(self):
addon = addon_factory(users=[user_factory()])
Expand All @@ -2988,6 +2978,8 @@ def test_execute_action_disable_addon(self):
decision.execute_action()
self._test_execute_action_disable_addon_outcome(decision)
assert '14 day(s)' not in mail.outbox[0].body
assert 'An attachment was provided.' not in mail.outbox[0].body
assert 'To respond or view the file,' not in mail.outbox[0].body

def _test_execute_action_reject_version_outcome(self, decision):
decision.send_notifications()
Expand Down Expand Up @@ -3024,12 +3016,19 @@ def test_execute_action_reject_version_held(self):
action=amo.LOG.HELD_ACTION_REJECT_VERSIONS.id
).get()
assert alog.contentdecisionlog_set.get().decision == decision
# attachment is linked to the original decision activity log
AttachmentLog.objects.create(
activity_log=alog,
file=ContentFile('Pseudo File', name='attachment.txt'),
)
decision.send_notifications()
assert len(mail.outbox) == 0
assert not version.needshumanreview_set.filter(is_active=True).exists()

decision.execute_action(release_hold=True)
self._test_execute_action_reject_version_outcome(decision)
assert 'An attachment was provided.' in mail.outbox[0].body
assert 'To respond or view the file,' in mail.outbox[0].body

def test_execute_action_reject_version(self):
addon = addon_factory(users=[user_factory()])
Expand Down Expand Up @@ -3061,6 +3060,8 @@ def test_execute_action_reject_version(self):
self._test_execute_action_reject_version_outcome(decision)
assert '14 day(s)' not in mail.outbox[0].body
assert not version.needshumanreview_set.filter(is_active=True).exists()
assert 'An attachment was provided.' not in mail.outbox[0].body
assert 'To respond or view the file,' not in mail.outbox[0].body

def _test_execute_action_reject_version_delayed_outcome(self, decision):
decision.send_notifications()
Expand Down Expand Up @@ -3405,7 +3406,21 @@ def test_execute_action_with_action_date_already(self):
process_mock.assert_not_called()
hold_mock.assert_not_called()
decision.send_notifications()
self._check_notify_emails(decision, log_entry)
assert len(mail.outbox) == 1
assert mail.outbox[0].to == [decision.addon.authors.first().email]
assert str(log_entry.id) in mail.outbox[0].extra_headers['Message-ID']
assert 'days' not in mail.outbox[0].body
assert 'some review text' in mail.outbox[0].body
assert 'some policy text' not in mail.outbox[0].body
AttachmentLog.objects.create(
activity_log=log_entry,
file=ContentFile('Pseudo File', name='attachment.txt'),
)
decision.send_notifications()
assert 'An attachment was provided.' not in mail.outbox[0].body
assert 'To respond or view the file,' not in mail.outbox[0].body
assert 'An attachment was provided.' in mail.outbox[1].body
assert 'To respond or view the file,' in mail.outbox[1].body

def test_get_target_review_url(self):
addon = addon_factory()
Expand Down
Loading