[Launchpad-reviewers] [Merge] lp:~maxiberta/launchpad/lp-1729580 into lp:launchpad
The proposal to merge lp:~maxiberta/launchpad/lp-1729580 into lp:launchpad has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~maxiberta/launchpad/lp-1729580/+merge/337825 -- Your team Launchpad code reviewers is subscribed to branch lp:launchpad. ___ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-reviewers] [Merge] lp:~maxiberta/launchpad/lp-1729580 into lp:launchpad
Diff comments:
> === modified file 'lib/lp/snappy/browser/tests/test_snapbuild.py'
> --- lib/lp/snappy/browser/tests/test_snapbuild.py 2017-10-20 13:35:42
> +
> +++ lib/lp/snappy/browser/tests/test_snapbuild.py 2018-02-16 21:53:15
> +
> @@ -130,6 +130,38 @@
> text=re.compile(
> r"^\s*Store upload failed:\s+Scan failed.\s*$"
>
> +def test_store_upload_status_failed_with_extended_error_message(self):
> +build = self.factory.makeSnapBuild(status=BuildStatus.FULLYBUILT)
> +job = getUtility(ISnapStoreUploadJobSource).create(build)
> +naked_job = removeSecurityProxy(job)
> +naked_job.job._status = JobStatus.FAILED
> +naked_job.error_message = "This should not be shown."
> +naked_job.error_messages = [
> +{"message": "Scan failed.", "link": "link1"},
> +{"message": "Classic not allowed.", "link": "link2"}]
> +build_view = create_initialized_view(build, "+index")
> +built_view = build_view()
> +self.assertThat(built_view, Not(soupmatchers.HTMLContains(
> +soupmatchers.Tag(
> +"store upload status", "li",
> +attrs={"id": "store-upload-status"},
> +text=re.compile('.*This should not be shown.*')
> +self.assertThat(built_view, soupmatchers.HTMLContains(
> +soupmatchers.Within(
> +soupmatchers.Tag(
> +"store upload status", "li",
> +attrs={"id": "store-upload-status"}),
> +soupmatchers.Within(
> +soupmatchers.Tag(
> +"store upload error messages", "ul",
> +attrs={"id": "store-upload-error-messages"}),
> +soupmatchers.Within(
> +soupmatchers.Tag(
> +"store upload error message", "li"),
> +soupmatchers.Tag(
> +"store upload error link", "a",
> +text="What does this mean?"))
Done.
> +
> def test_store_upload_status_release_failed(self):
> build = self.factory.makeSnapBuild(status=BuildStatus.FULLYBUILT)
> job = getUtility(ISnapStoreUploadJobSource).create(build)
>
> === modified file 'lib/lp/snappy/model/snapstoreclient.py'
> --- lib/lp/snappy/model/snapstoreclient.py2017-06-14 10:25:23 +
> +++ lib/lp/snappy/model/snapstoreclient.py2018-02-16 21:53:15 +
> @@ -316,7 +316,11 @@
> elif "errors" in response_data:
> error_message = "\n".join(
> error["message"] for error in response_data["errors"])
> -raise ScanFailedResponse(error_message)
> +error_messages = [
> +{"message": error["message"], "link": error.get("link")}
> +for error in response_data["errors"]]
Good idea, done.
> +raise ScanFailedResponse(
> +error_message, messages=error_messages)
> elif not response_data["can_release"]:
> return response_data["url"], None
> else:
>
> === modified file 'lib/lp/snappy/templates/snapbuild-index.pt'
> --- lib/lp/snappy/templates/snapbuild-index.pt2017-04-03 12:35:03
> +
> +++ lib/lp/snappy/templates/snapbuild-index.pt2018-02-16 21:53:15
> +
> @@ -177,7 +177,15 @@
>condition="context/store_upload_status/enumvalue:FAILEDTOUPLOAD">
>Store upload failed:
> -
> + +condition="not: context/store_upload_error_messages"
> +replace="context/store_upload_error_message" />
> +
> +
> +
> + What does this mean?
Nice!
> +
> +
>
>
>
--
https://code.launchpad.net/~maxiberta/launchpad/lp-1729580/+merge/337825
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.
___
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-reviewers] [Merge] lp:~maxiberta/launchpad/lp-1729580 into lp:launchpad
Review: Approve
Diff comments:
> === modified file 'lib/lp/snappy/browser/tests/test_snapbuild.py'
> --- lib/lp/snappy/browser/tests/test_snapbuild.py 2017-10-20 13:35:42
> +
> +++ lib/lp/snappy/browser/tests/test_snapbuild.py 2018-02-16 21:53:15
> +
> @@ -14,7 +14,7 @@
> from pymacaroons import Macaroon
> import soupmatchers
> from storm.locals import Store
> -from testtools.matchers import StartsWith
> +from testtools.matchers import Not, StartsWith
LP style is to always write this like this, even if it would fit on a single
line:
from testtools.matchers import (
Not,
StartsWith,
)
> import transaction
> from zope.component import getUtility
> from zope.security.interfaces import Unauthorized
> @@ -130,6 +130,38 @@
> text=re.compile(
> r"^\s*Store upload failed:\s+Scan failed.\s*$"
>
> +def test_store_upload_status_failed_with_extended_error_message(self):
> +build = self.factory.makeSnapBuild(status=BuildStatus.FULLYBUILT)
> +job = getUtility(ISnapStoreUploadJobSource).create(build)
> +naked_job = removeSecurityProxy(job)
> +naked_job.job._status = JobStatus.FAILED
> +naked_job.error_message = "This should not be shown."
> +naked_job.error_messages = [
> +{"message": "Scan failed.", "link": "link1"},
> +{"message": "Classic not allowed.", "link": "link2"}]
> +build_view = create_initialized_view(build, "+index")
> +built_view = build_view()
> +self.assertThat(built_view, Not(soupmatchers.HTMLContains(
> +soupmatchers.Tag(
> +"store upload status", "li",
> +attrs={"id": "store-upload-status"},
> +text=re.compile('.*This should not be shown.*')
> +self.assertThat(built_view, soupmatchers.HTMLContains(
> +soupmatchers.Within(
> +soupmatchers.Tag(
> +"store upload status", "li",
> +attrs={"id": "store-upload-status"}),
> +soupmatchers.Within(
> +soupmatchers.Tag(
> +"store upload error messages", "ul",
> +attrs={"id": "store-upload-error-messages"}),
> +soupmatchers.Within(
> +soupmatchers.Tag(
> +"store upload error message", "li"),
> +soupmatchers.Tag(
> +"store upload error link", "a",
> +text="What does this mean?"))
Could you test the href attribute here too?
> +
> def test_store_upload_status_release_failed(self):
> build = self.factory.makeSnapBuild(status=BuildStatus.FULLYBUILT)
> job = getUtility(ISnapStoreUploadJobSource).create(build)
>
> === modified file 'lib/lp/snappy/model/snapstoreclient.py'
> --- lib/lp/snappy/model/snapstoreclient.py2017-06-14 10:25:23 +
> +++ lib/lp/snappy/model/snapstoreclient.py2018-02-16 21:53:15 +
> @@ -316,7 +316,11 @@
> elif "errors" in response_data:
> error_message = "\n".join(
> error["message"] for error in response_data["errors"])
> -raise ScanFailedResponse(error_message)
> +error_messages = [
> +{"message": error["message"], "link": error.get("link")}
> +for error in response_data["errors"]]
Pedantically, if link is absent from the original error, should it perhaps be
absent from the corresponding element of error_messages too?
> +raise ScanFailedResponse(
> +error_message, messages=error_messages)
> elif not response_data["can_release"]:
> return response_data["url"], None
> else:
>
> === modified file 'lib/lp/snappy/templates/snapbuild-index.pt'
> --- lib/lp/snappy/templates/snapbuild-index.pt2017-04-03 12:35:03
> +
> +++ lib/lp/snappy/templates/snapbuild-index.pt2018-02-16 21:53:15
> +
> @@ -177,7 +177,15 @@
>condition="context/store_upload_status/enumvalue:FAILEDTOUPLOAD">
>Store upload failed:
> -
> + +condition="not: context/store_upload_error_messages"
> +replace="context/store_upload_error_message" />
> +
> +
> +
> + What does this mean?
Could you render this using widget_help_link instead? It'd look a bit nicer
that way (in general, but especially in the case where the error message
doesn't end with a full stop).
> +
> +
>
>
>
>
> === modified file 'lib/lp/snappy/tests/test_snapbuildjob.py'
> --- lib/lp/snappy/tests/test_snapbuildjob.py 2017-06-29 18:06:03 +
> +++ lib/lp/snappy/tests/test_snapbuildjob.py
[Launchpad-reviewers] [Merge] lp:~maxiberta/launchpad/lp-1729580 into lp:launchpad
Maximiliano Bertacchini has proposed merging lp:~maxiberta/launchpad/lp-1729580
into lp:launchpad.
Commit message:
Expose extended error messages (with external link) for snap build jobs (LP:
#1729580).
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~maxiberta/launchpad/lp-1729580/+merge/337825
Expose extended error messages (with external link) for snap build jobs (LP:
#1729580).
--
Your team Launchpad code reviewers is requested to review the proposed merge of
lp:~maxiberta/launchpad/lp-1729580 into lp:launchpad.
=== modified file 'lib/lp/snappy/interfaces/snapbuild.py'
--- lib/lp/snappy/interfaces/snapbuild.py 2017-04-27 16:22:37 +
+++ lib/lp/snappy/interfaces/snapbuild.py 2018-02-15 22:12:44 +
@@ -40,6 +40,7 @@
Choice,
Datetime,
Int,
+Text,
TextLine,
)
@@ -213,6 +214,15 @@
"this snap build to the store."),
required=False, readonly=True))
+store_upload_error_messages = exported(Text(
+title=_("Store upload error messages"),
+description=_(
+"A list of dict(message, link) where message is an error "
+"description and link, if any, is an external link to extra "
+"details, from the last attempt to upload this snap build "
+"to the store."),
+required=False, readonly=True))
+
def getFiles():
"""Retrieve the build's `ISnapFile` records.
=== modified file 'lib/lp/snappy/interfaces/snapstoreclient.py'
--- lib/lp/snappy/interfaces/snapstoreclient.py 2017-06-14 10:25:23 +
+++ lib/lp/snappy/interfaces/snapstoreclient.py 2018-02-15 22:12:44 +
@@ -29,10 +29,12 @@
class SnapStoreError(Exception):
-def __init__(self, message="", detail=None, can_retry=False):
+def __init__(
+self, message="", detail=None, messages=None, can_retry=False):
super(SnapStoreError, self).__init__(message)
self.message = message
self.detail = detail
+self.messages = messages
self.can_retry = can_retry
=== modified file 'lib/lp/snappy/model/snapbuild.py'
--- lib/lp/snappy/model/snapbuild.py 2018-01-23 10:59:44 +
+++ lib/lp/snappy/model/snapbuild.py 2018-02-15 22:12:44 +
@@ -456,6 +456,11 @@
job = self.last_store_upload_job
return job and job.error_message
+@property
+def store_upload_error_messages(self):
+job = self.last_store_upload_job
+return job and job.error_messages
+
def scheduleStoreUpload(self):
"""See `ISnapBuild`."""
if not self.snap.can_upload_to_store:
=== modified file 'lib/lp/snappy/model/snapbuildjob.py'
--- lib/lp/snappy/model/snapbuildjob.py 2017-06-29 16:58:01 +
+++ lib/lp/snappy/model/snapbuildjob.py 2018-02-15 22:12:44 +
@@ -232,6 +232,16 @@
self.metadata["error_detail"] = detail
@property
+def error_messages(self):
+"""See `ISnapStoreUploadJob`."""
+return self.metadata.get("error_messages")
+
+@error_messages.setter
+def error_messages(self, messages):
+"""See `ISnapStoreUploadJob`."""
+self.metadata["error_messages"] = messages
+
+@property
def store_url(self):
"""See `ISnapStoreUploadJob`."""
return self.metadata.get("store_url")
@@ -326,6 +336,7 @@
self.attempt_count <= self.max_retries):
raise RetryableSnapStoreError(e.message, detail=e.detail)
self.error_message = str(e)
+self.error_messages = getattr(e, "messages", None)
self.error_detail = getattr(e, "detail", None)
if isinstance(e, UnauthorizedUploadResponse):
mailer = SnapBuildMailer.forUnauthorizedUpload(self.snapbuild)
=== modified file 'lib/lp/snappy/model/snapstoreclient.py'
--- lib/lp/snappy/model/snapstoreclient.py 2017-06-14 10:25:23 +
+++ lib/lp/snappy/model/snapstoreclient.py 2018-02-15 22:12:44 +
@@ -316,7 +316,11 @@
elif "errors" in response_data:
error_message = "\n".join(
error["message"] for error in response_data["errors"])
-raise ScanFailedResponse(error_message)
+error_messages = [
+{"message": error["message"], "link": error.get("link")}
+for error in response_data["errors"]]
+raise ScanFailedResponse(
+error_message, messages=error_messages)
elif not response_data["can_release"]:
return response_data["url"], None
else:
=== modified file 'lib/lp/snappy/tests/test_snapbuildjob.py'
--- lib/lp/snappy/tests/test_snapbuildjob.py 2017-06-29 18:06:03 +
+++ lib/lp/snappy/tests/test_snapbuildjob.py 2018-02-15 22:12:44 +
@@ -31,7 +31,7 @@
)
from lp.snappy.interfaces.snapstoreclient import (
BadRefreshResponse,
-BadScanStatusResponse,
+
