split resolving jobs functionality into seperate action from comment (#22375)
This commit is contained in:
Andrew Williamson
GitHub
Родитель
6ca5b5932c
Коммит
8d1d7d471d
|
|
@ -1039,6 +1039,16 @@ class AUTO_REJECT_CONTENT_AFTER_DELAY_EXPIRED(_LOG):
|
|||
cinder_action = DECISION_ACTIONS.AMO_REJECT_VERSION_ADDON
|
||||
|
||||
|
||||
class RESOLVE_CINDER_JOB_WITH_NO_ACTION(_LOG):
|
||||
id = 191
|
||||
format = '{addon} cinder job resolved with no action .'
|
||||
short = _('Job Resolved as Ignore/Approve')
|
||||
keep = True
|
||||
review_queue = True
|
||||
hide_developer = True
|
||||
reviewer_review_action = True
|
||||
|
||||
|
||||
LOGS = [x for x in vars().values() if isclass(x) and issubclass(x, _LOG) and x != _LOG]
|
||||
# Make sure there's no duplicate IDs.
|
||||
assert len(LOGS) == len({log.id for log in LOGS})
|
||||
|
|
|
|||
|
|
@ -167,10 +167,9 @@
|
|||
{{ form.versions }}
|
||||
{{ form.versions.errors }}
|
||||
|
||||
<div class="review-actions-section data-toggle review-comments"
|
||||
data-value="{{ actions_comments|join(' ') }}">
|
||||
<div class="review-actions-section">
|
||||
<div class="review-actions-comments-reasons">
|
||||
<div class="review-actions-comments">
|
||||
<div class="review-actions-comments data-toggle review-comments" data-value="{{ actions_comments|join(' ') }}">
|
||||
<label for="id_comments">{{ form.comments.label }}</label>
|
||||
{{ form.comments }}
|
||||
{{ form.comments.errors }}
|
||||
|
|
|
|||
|
|
@ -383,7 +383,11 @@ class TestReviewForm(TestCase):
|
|||
)
|
||||
form = self.get_form()
|
||||
assert not form.is_bound
|
||||
data = {'action': 'comment', 'comments': 'lol', 'resolve_cinder_jobs': [job.id]}
|
||||
data = {
|
||||
'action': 'resolve_job',
|
||||
'comments': 'lol',
|
||||
'resolve_cinder_jobs': [job.id],
|
||||
}
|
||||
form = self.get_form(data=data)
|
||||
assert form.is_bound
|
||||
assert not form.is_valid()
|
||||
|
|
@ -426,7 +430,7 @@ class TestReviewForm(TestCase):
|
|||
form = self.get_form()
|
||||
assert not form.is_bound
|
||||
data = {
|
||||
'action': 'comment',
|
||||
'action': 'resolve_job',
|
||||
'comments': 'lol',
|
||||
'resolve_cinder_jobs': [job.id],
|
||||
'cinder_policies': [no_action_policy.id],
|
||||
|
|
|
|||
|
|
@ -949,6 +949,26 @@ class TestReviewHelper(TestReviewHelperBase):
|
|||
)
|
||||
assert expected == actions
|
||||
|
||||
def test_actions_resolve_cinder_jobs(self):
|
||||
self.grant_permission(self.user, 'Addons:Review')
|
||||
CinderJob.objects.create(
|
||||
target_addon=self.addon, resolvable_in_reviewer_tools=True
|
||||
)
|
||||
expected = [
|
||||
'reject_multiple_versions',
|
||||
'set_needs_human_review_multiple_versions',
|
||||
'reply',
|
||||
'resolve_job',
|
||||
'comment',
|
||||
]
|
||||
actions = list(
|
||||
self.get_review_actions(
|
||||
addon_status=amo.STATUS_APPROVED,
|
||||
file_status=amo.STATUS_APPROVED,
|
||||
).keys()
|
||||
)
|
||||
assert expected == actions
|
||||
|
||||
def test_set_file(self):
|
||||
self.file.update(datestatuschanged=yesterday)
|
||||
self.helper.handler.set_file(amo.STATUS_APPROVED, self.review_version.file)
|
||||
|
|
@ -1006,6 +1026,9 @@ class TestReviewHelper(TestReviewHelperBase):
|
|||
|
||||
def test_log_action_sets_policies_with_allow_policies(self):
|
||||
self.grant_permission(self.user, 'Addons:Review')
|
||||
cinder_job = CinderJob.objects.create(
|
||||
target_addon=self.addon, resolvable_in_reviewer_tools=True
|
||||
)
|
||||
self.helper = self.get_helper()
|
||||
data = {
|
||||
# ignored - the action doesn't allow_reasons
|
||||
|
|
@ -1026,16 +1049,17 @@ class TestReviewHelper(TestReviewHelperBase):
|
|||
uuid='z', default_cinder_action=DECISION_ACTIONS.AMO_IGNORE
|
||||
),
|
||||
],
|
||||
'resolve_cinder_jobs': [cinder_job],
|
||||
}
|
||||
self.helper.set_data(data)
|
||||
self.helper.handler.review_action = self.helper.actions.get('comment')
|
||||
self.helper.handler.log_action(amo.LOG.COMMENT_VERSION)
|
||||
self.helper.handler.review_action = self.helper.actions.get('resolve_job')
|
||||
self.helper.handler.log_action(amo.LOG.RESOLVE_CINDER_JOB_WITH_NO_ACTION)
|
||||
assert ReviewActionReasonLog.objects.count() == 0
|
||||
assert CinderPolicyLog.objects.count() == 2
|
||||
assert (
|
||||
ActivityLog.objects.get(action=amo.LOG.COMMENT_VERSION.id).details[
|
||||
'cinder_action'
|
||||
]
|
||||
ActivityLog.objects.get(
|
||||
action=amo.LOG.RESOLVE_CINDER_JOB_WITH_NO_ACTION.id
|
||||
).details['cinder_action']
|
||||
== 'AMO_IGNORE'
|
||||
)
|
||||
|
||||
|
|
@ -3296,6 +3320,9 @@ class TestReviewHelper(TestReviewHelperBase):
|
|||
AutoApprovalSummary.objects.create(
|
||||
version=self.review_version, verdict=amo.AUTO_APPROVED, weight=42
|
||||
)
|
||||
CinderJob.objects.create(
|
||||
target_addon=self.addon, resolvable_in_reviewer_tools=True
|
||||
)
|
||||
self.setup_data(amo.STATUS_APPROVED)
|
||||
self._notify_decision_called_everywhere_checkbox_shown(
|
||||
{
|
||||
|
|
@ -3319,7 +3346,7 @@ class TestReviewHelper(TestReviewHelperBase):
|
|||
'should_email': True,
|
||||
'cinder_action': DECISION_ACTIONS.AMO_DISABLE_ADDON,
|
||||
},
|
||||
'comment': {'should_email': False, 'cinder_action': None},
|
||||
'resolve_job': {'should_email': False, 'cinder_action': None},
|
||||
}
|
||||
)
|
||||
self.setup_data(amo.STATUS_APPROVED, file_status=amo.STATUS_DISABLED)
|
||||
|
|
@ -3338,7 +3365,7 @@ class TestReviewHelper(TestReviewHelperBase):
|
|||
'should_email': True,
|
||||
'cinder_action': DECISION_ACTIONS.AMO_DISABLE_ADDON,
|
||||
},
|
||||
'comment': {'should_email': False, 'cinder_action': None},
|
||||
'resolve_job': {'should_email': False, 'cinder_action': None},
|
||||
}
|
||||
)
|
||||
self.setup_data(amo.STATUS_DISABLED, file_status=amo.STATUS_DISABLED)
|
||||
|
|
@ -3352,7 +3379,7 @@ class TestReviewHelper(TestReviewHelperBase):
|
|||
'should_email': True,
|
||||
'cinder_action': DECISION_ACTIONS.AMO_APPROVE_VERSION,
|
||||
},
|
||||
'comment': {'should_email': False, 'cinder_action': None},
|
||||
'resolve_job': {'should_email': False, 'cinder_action': None},
|
||||
}
|
||||
)
|
||||
|
||||
|
|
@ -3363,6 +3390,9 @@ class TestReviewHelper(TestReviewHelperBase):
|
|||
AutoApprovalSummary.objects.create(
|
||||
version=self.review_version, verdict=amo.AUTO_APPROVED, weight=42
|
||||
)
|
||||
CinderJob.objects.create(
|
||||
target_addon=self.addon, resolvable_in_reviewer_tools=True
|
||||
)
|
||||
self.setup_data(amo.STATUS_APPROVED, channel=amo.CHANNEL_UNLISTED)
|
||||
self._notify_decision_called_everywhere_checkbox_shown(
|
||||
{
|
||||
|
|
@ -3386,7 +3416,7 @@ class TestReviewHelper(TestReviewHelperBase):
|
|||
'should_email': True,
|
||||
'cinder_action': DECISION_ACTIONS.AMO_DISABLE_ADDON,
|
||||
},
|
||||
'comment': {'should_email': False, 'cinder_action': None},
|
||||
'resolve_job': {'should_email': False, 'cinder_action': None},
|
||||
}
|
||||
)
|
||||
self.setup_data(amo.STATUS_DISABLED, file_status=amo.STATUS_DISABLED)
|
||||
|
|
@ -3408,7 +3438,7 @@ class TestReviewHelper(TestReviewHelperBase):
|
|||
'should_email': True,
|
||||
'cinder_action': DECISION_ACTIONS.AMO_APPROVE_VERSION,
|
||||
},
|
||||
'comment': {'should_email': False, 'cinder_action': None},
|
||||
'resolve_job': {'should_email': False, 'cinder_action': None},
|
||||
}
|
||||
)
|
||||
|
||||
|
|
|
|||
|
|
@ -2515,7 +2515,7 @@ class TestReview(ReviewBase):
|
|||
assert ActivityLog.objects.filter(action=comment_version.id).count() == 1
|
||||
|
||||
@mock.patch('olympia.reviewers.utils.resolve_job_in_cinder.delay')
|
||||
def test_comment_with_resolve_cinder_job(self, resolve_mock):
|
||||
def test_resolve_cinder_job(self, resolve_mock):
|
||||
cinder_job = CinderJob.objects.create(
|
||||
job_id='123', target_addon=self.addon, resolvable_in_reviewer_tools=True
|
||||
)
|
||||
|
|
@ -2534,23 +2534,18 @@ class TestReview(ReviewBase):
|
|||
response = self.client.post(
|
||||
self.url,
|
||||
{
|
||||
'action': 'comment',
|
||||
'comments': 'hello sailor',
|
||||
'action': 'resolve_job',
|
||||
'resolve_cinder_jobs': [cinder_job.id],
|
||||
'cinder_policies': [policy.id],
|
||||
},
|
||||
)
|
||||
assert response.status_code == 302
|
||||
|
||||
assert (
|
||||
ActivityLog.objects.filter(action=amo.LOG.COMMENT_VERSION.id).count() == 1
|
||||
)
|
||||
assert (
|
||||
ActivityLog.objects.filter(action=amo.LOG.COMMENT_VERSION.id)[0].details[
|
||||
'cinder_action'
|
||||
]
|
||||
== 'AMO_IGNORE'
|
||||
activity_log_qs = ActivityLog.objects.filter(
|
||||
action=amo.LOG.RESOLVE_CINDER_JOB_WITH_NO_ACTION.id
|
||||
)
|
||||
assert activity_log_qs.count() == 1
|
||||
assert activity_log_qs[0].details['cinder_action'] == 'AMO_IGNORE'
|
||||
resolve_mock.assert_called_once()
|
||||
|
||||
def test_reviewer_reply(self):
|
||||
|
|
@ -2658,7 +2653,7 @@ class TestReview(ReviewBase):
|
|||
str(author.get_role_display()),
|
||||
self.addon,
|
||||
)
|
||||
with self.assertNumQueries(55):
|
||||
with self.assertNumQueries(56):
|
||||
# FIXME: obviously too high, but it's a starting point.
|
||||
# Potential further optimizations:
|
||||
# - Remove trivial... and not so trivial duplicates
|
||||
|
|
@ -2686,42 +2681,43 @@ class TestReview(ReviewBase):
|
|||
# 17. version reviewer flags
|
||||
# 18. version reviewer flags (repeated)
|
||||
# 19. version autoapprovalsummary
|
||||
# 20. addonreusedguid
|
||||
# 21. blocklist
|
||||
# 22. unresolved DSA related abuse reports
|
||||
# 23. abuse reports count against user or addon
|
||||
# 24. low ratings count
|
||||
# 25. base version pk for comparison
|
||||
# 26. count of all versions in channel
|
||||
# 27. paginated list of versions in channel
|
||||
# 28. scanner results for paginated list of versions
|
||||
# 29. translations for paginated list of versions
|
||||
# 30. applications versions for paginated list of versions
|
||||
# 31. activity log for paginated list of versions
|
||||
# 32. files for paginated list of versions
|
||||
# 33. versionreviewer flags exists to find out if pending rejection
|
||||
# 34. count versions needing human review on other pages
|
||||
# 35. count versions needing human review by mad on other pages
|
||||
# 36. count versions pending rejection on other pages
|
||||
# 37. whiteboard
|
||||
# 38. reviewer subscriptions for listed
|
||||
# 39. reviewer subscriptions for unlisted
|
||||
# 40. config for motd
|
||||
# 41. release savepoint (?)
|
||||
# 42. count add-ons the user is a developer of
|
||||
# 43. config for site notice
|
||||
# 44. other add-ons with same guid
|
||||
# 45. translations for... (?! id=1)
|
||||
# 46. important activity log about the add-on
|
||||
# 47. user for the activity (from the ActivityLog foreignkey)
|
||||
# 48. user for the activity (from the ActivityLog arguments)
|
||||
# 49. add-on for the activity
|
||||
# 50. translation for the add-on for the activity
|
||||
# 51. select all versions in channel for versions dropdown widget
|
||||
# 52. reviewer reasons for the reason dropdown
|
||||
# 53. cinder policies for the policy dropdown
|
||||
# 54. select users by role for this add-on (?)
|
||||
# 55. unreviewed versions in other channel
|
||||
# 20. blocklist
|
||||
# 21. cinderjob exists
|
||||
# 22. addonreusedguid
|
||||
# 23. unresolved DSA related abuse reports
|
||||
# 24. abuse reports count against user or addon
|
||||
# 25. low ratings count
|
||||
# 26. base version pk for comparison
|
||||
# 27. count of all versions in channel
|
||||
# 28. paginated list of versions in channel
|
||||
# 29. scanner results for paginated list of versions
|
||||
# 30. translations for paginated list of versions
|
||||
# 31. applications versions for paginated list of versions
|
||||
# 32. activity log for paginated list of versions
|
||||
# 33. files for paginated list of versions
|
||||
# 34. versionreviewer flags exists to find out if pending rejection
|
||||
# 35. count versions needing human review on other pages
|
||||
# 36. count versions needing human review by mad on other pages
|
||||
# 37. count versions pending rejection on other pages
|
||||
# 38. whiteboard
|
||||
# 39. reviewer subscriptions for listed
|
||||
# 40. reviewer subscriptions for unlisted
|
||||
# 41. config for motd
|
||||
# 42. release savepoint (?)
|
||||
# 43. count add-ons the user is a developer of
|
||||
# 44. config for site notice
|
||||
# 45. other add-ons with same guid
|
||||
# 46. translations for... (?! id=1)
|
||||
# 47. important activity log about the add-on
|
||||
# 48. user for the activity (from the ActivityLog foreignkey)
|
||||
# 49. user for the activity (from the ActivityLog arguments)
|
||||
# 50. add-on for the activity
|
||||
# 51. translation for the add-on for the activity
|
||||
# 52. select all versions in channel for versions dropdown widget
|
||||
# 53. reviewer reasons for the reason dropdown
|
||||
# 54. cinder policies for the policy dropdown
|
||||
# 55. select users by role for this add-on (?)
|
||||
# 56. unreviewed versions in other channel
|
||||
response = self.client.get(self.url)
|
||||
assert response.status_code == 200
|
||||
doc = pq(response.content)
|
||||
|
|
@ -5389,7 +5385,7 @@ class TestReview(ReviewBase):
|
|||
results={'matchedRules': [customs_rule.name]},
|
||||
)
|
||||
|
||||
with self.assertNumQueries(56):
|
||||
with self.assertNumQueries(57):
|
||||
# See test_item_history_pagination() for more details about the
|
||||
# queries count. What's important here is that the extra versions
|
||||
# and scanner results don't cause extra queries.
|
||||
|
|
|
|||
|
|
@ -12,6 +12,7 @@ import markupsafe
|
|||
|
||||
import olympia.core.logger
|
||||
from olympia import amo
|
||||
from olympia.abuse.models import CinderJob
|
||||
from olympia.abuse.tasks import notify_addon_decision_to_cinder, resolve_job_in_cinder
|
||||
from olympia.access import acl
|
||||
from olympia.activity.models import ActivityLog
|
||||
|
|
@ -536,6 +537,13 @@ class ReviewHelper:
|
|||
)
|
||||
version_is_blocked = self.version and self.version.is_blocked
|
||||
|
||||
has_unresolved_abuse_report_jobs = (
|
||||
CinderJob.objects.for_addon(self.addon)
|
||||
.unresolved()
|
||||
.resolvable_in_reviewer_tools()
|
||||
.exists()
|
||||
)
|
||||
|
||||
# Special logic for availability of reject/approve multiple action:
|
||||
if version_is_unlisted:
|
||||
can_reject_multiple = is_appropriate_reviewer
|
||||
|
|
@ -828,17 +836,28 @@ class ReviewHelper:
|
|||
'requires_reasons': not is_static_theme,
|
||||
'resolves_abuse_reports': True,
|
||||
}
|
||||
actions['resolve_job'] = {
|
||||
'method': self.handler.resolve_job,
|
||||
'label': 'Resolve Jobs',
|
||||
'details': (
|
||||
'Allows abuse report jobs to be resovled without an action on the '
|
||||
'add-on or versions.'
|
||||
),
|
||||
'minimal': True,
|
||||
'available': is_reviewer and has_unresolved_abuse_report_jobs,
|
||||
'comments': False,
|
||||
'resolves_abuse_reports': True,
|
||||
'allows_policies': True,
|
||||
}
|
||||
actions['comment'] = {
|
||||
'method': self.handler.process_comment,
|
||||
'label': 'Comment / Resolve Jobs',
|
||||
'label': 'Comment',
|
||||
'details': (
|
||||
"Make a comment on this version. The developer won't be able to see "
|
||||
'this. Can be used to resolve jobs/appeals without action '
|
||||
'this.'
|
||||
),
|
||||
'minimal': True,
|
||||
'available': is_reviewer,
|
||||
'resolves_abuse_reports': True,
|
||||
'allows_policies': True,
|
||||
}
|
||||
return OrderedDict(
|
||||
((key, action) for key, action in actions.items() if action['available'])
|
||||
|
|
@ -1077,7 +1096,10 @@ class ReviewBase:
|
|||
|
||||
def process_comment(self):
|
||||
self.log_action(amo.LOG.COMMENT_VERSION)
|
||||
|
||||
def resolve_job(self):
|
||||
if self.data.get('resolve_cinder_jobs', ()):
|
||||
self.log_action(amo.LOG.RESOLVE_CINDER_JOB_WITH_NO_ACTION)
|
||||
self.notify_decision() # notify cinder
|
||||
|
||||
def approve_latest_version(self):
|
||||
|
|
|
|||
|
|
@ -1042,6 +1042,10 @@ form.review-form .data-toggle {
|
|||
width: 100%;
|
||||
}
|
||||
|
||||
.review-actions-policies-select {
|
||||
height: 100px;
|
||||
}
|
||||
|
||||
.review-actions-reasons-select ul {
|
||||
list-style: none;
|
||||
}
|
||||
|
|
|
|||
Загрузка…
Ссылка в новой задаче