813319 - Reviewer tools need to catch SigningErrors

Status: RESOLVED FIXED

(Marketplace Graveyard :: Reviewer Tools, defect, P4)

Description

[krupa raj[:krupa]]

traceback details: 
Stacktrace (most recent call last):

  File "django/core/handlers/base.py", line 111, in get_response
    response = callback(request, *callback_args, **callback_kwargs)
  File "django/db/transaction.py", line 209, in inner
    return func(*args, **kwargs)
  File "amo/decorators.py", line 33, in wrapper
    return func(request, *args, **kw)
  File "amo/decorators.py", line 65, in wrapper
    return f(request, *args, **kw)
  File "addons/decorators.py", line 33, in wrapper
    return f(request, addon, *args, **kw)
  File "mkt/reviewers/views.py", line 265, in app_review
    return _review(request, addon)
  File "mkt/reviewers/views.py", line 205, in _review
    form.helper.process()
  File "mkt/reviewers/utils.py", line 397, in process
    return self.actions[action]['method']()
  File "mkt/reviewers/utils.py", line 136, in process_public
    self.process_public_immediately()
  File "mkt/reviewers/utils.py", line 171, in process_public_immediately
    self.addon.sign_if_packaged(self.version.pk)
  File "mkt/webapps/models.py", line 679, in sign_if_packaged
    return packaged.sign(version_pk, reviewer=reviewer)
  File "nuggets/celeryutils.py", line 35, in wrapped
    return fun(*args, **kw)
  File "lib/crypto/packaged.py", line 135, in sign
    sign_app(file_obj.file_path, path)
  File "lib/crypto/packaged.py", line 44, in sign_app
    raise SigningError("Archive extraction failed. Bad archive?")

sentry: http://sentry.dmz.phx1.mozilla.com/marketplace-dev/group/3203/

Comment 1

[Wil Clouser [:clouserw]]

We should catch this and log it or something.  Shouldn't be oopsing.

Comment 2

[Andy McKay]

Traceback fixed for this instance.

I think we have to abort the push to public if it can't be signed correctly. Logging would leave us in a bad state, a public packaged app that's unsigned. I think the change is to make the reviewer tools catch SigningErrors and raise an appropriate message, so it's not a 500, but it's not public either.

Comment 3

[Rob Hudson [:robhudson]]

My plan next week:

* Catch SigningError (and look for any other errors that make sense to catch in the view or reviewer.utils.
* Log the error
* Return nice message to the reviewers (vs 500 "Oops")
* We already have `commit_on_success` so that any error won't make the app public.

Comment 4

[Rob Hudson [:robhudson]]

https://github.com/mozilla/zamboni/commit/8d2599c