launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

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 +0000
> +++ lib/lp/snappy/browser/tests/test_snapbuild.py	2018-02-16 21:53:15 +0000
> @@ -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.py	2017-06-14 10:25:23 +0000
> +++ lib/lp/snappy/model/snapstoreclient.py	2018-02-16 21:53:15 +0000
> @@ -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.pt	2017-04-03 12:35:03 +0000
> +++ lib/lp/snappy/templates/snapbuild-index.pt	2018-02-16 21:53:15 +0000
> @@ -177,7 +177,15 @@
>          <tal:failed-upload
>              condition="context/store_upload_status/enumvalue:FAILEDTOUPLOAD">
>            Store upload failed:
> -          <tal:error-message replace="context/store_upload_error_message" />
> +          <tal:error-message
> +            condition="not: context/store_upload_error_messages"
> +            replace="context/store_upload_error_message" />
> +          <ul id="store-upload-error-messages">
> +            <li tal:repeat="error context/store_upload_error_messages">
> +              <span tal:replace="error/message"/>
> +              <a tal:condition="error/link" tal:attributes="href error/link">What does this mean?</a>

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).

> +            </li>
> +          </ul>
>            <form action="" method="POST">
>              <input type="submit" name="field.actions.upload" value="Retry" />
>            </form>
> 
> === modified file 'lib/lp/snappy/tests/test_snapbuildjob.py'
> --- lib/lp/snappy/tests/test_snapbuildjob.py	2017-06-29 18:06:03 +0000
> +++ lib/lp/snappy/tests/test_snapbuildjob.py	2018-02-16 21:53:15 +0000
> @@ -31,7 +31,7 @@
>      )
>  from lp.snappy.interfaces.snapstoreclient import (
>      BadRefreshResponse,
> -    BadScanStatusResponse,
> +    ScanFailedResponse,

Sort imports.

>      ISnapStoreClient,
>      ReleaseFailedResponse,
>      UnauthorizedUploadResponse,


-- 
https://code.launchpad.net/~maxiberta/launchpad/lp-1729580/+merge/337825
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.