launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-build-better-error-messages into lp:launchpad.

Commit message:
Synthesise SnapBuild.store_upload_error_messages based on store_upload_error_message if necessary.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/snap-build-better-error-messages/+merge/356312

This allows deprecating store_upload_error_message rather than needing to check both (which build.snapcraft.io does in a slightly buggy way and thus sometimes fails to show error messages).
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snap-build-better-error-messages into lp:launchpad.

=== modified file 'lib/lp/snappy/browser/tests/test_snapbuild.py'
--- lib/lp/snappy/browser/tests/test_snapbuild.py	2018-03-22 10:08:37 +0000
+++ lib/lp/snappy/browser/tests/test_snapbuild.py	2018-10-09 10:40:56 +0000
@@ -129,9 +129,14 @@
         self.assertThat(build_view(), soupmatchers.HTMLContains(
             soupmatchers.Tag(
                 "store upload status", "li",
-                attrs={"id": "store-upload-status"},
-                text=re.compile(
-                    r"^\s*Store upload failed:\s+Scan failed.\s*$"))))
+                attrs={"id": "store-upload-status"}),
+            soupmatchers.Within(
+                soupmatchers.Tag(
+                    "store upload error messages", "ul",
+                    attrs={"id": "store-upload-error-messages"}),
+                soupmatchers.Tag(
+                    "store upload error message", "li",
+                    text=re.compile(r"^\s*Scan failed.\s*$")))))
 
     def test_store_upload_status_failed_with_extended_error_message(self):
         build = self.factory.makeSnapBuild(status=BuildStatus.FULLYBUILT)

=== modified file 'lib/lp/snappy/interfaces/snapbuild.py'
--- lib/lp/snappy/interfaces/snapbuild.py	2018-08-03 13:53:20 +0000
+++ lib/lp/snappy/interfaces/snapbuild.py	2018-10-09 10:40:56 +0000
@@ -228,7 +228,8 @@
         title=_("Store upload error message"),
         description=_(
             "The error message, if any, from the last attempt to upload "
-            "this snap build to the store."),
+            "this snap build to the store.  (Deprecated; use "
+            "store_upload_error_messages instead.)"),
         required=False, readonly=True))
 
     store_upload_error_messages = exported(List(

=== modified file 'lib/lp/snappy/model/snapbuild.py'
--- lib/lp/snappy/model/snapbuild.py	2018-10-04 12:54:04 +0000
+++ lib/lp/snappy/model/snapbuild.py	2018-10-09 10:40:56 +0000
@@ -477,7 +477,12 @@
     @property
     def store_upload_error_messages(self):
         job = self.last_store_upload_job
-        return job and job.error_messages or []
+        if job:
+            if job.error_messages:
+                return job.error_messages
+            elif job.error_message:
+                return [{"message": job.error_message}]
+        return []
 
     def scheduleStoreUpload(self):
         """See `ISnapBuild`."""

=== modified file 'lib/lp/snappy/templates/snapbuild-index.pt'
--- lib/lp/snappy/templates/snapbuild-index.pt	2018-03-22 10:08:37 +0000
+++ lib/lp/snappy/templates/snapbuild-index.pt	2018-10-09 10:40:56 +0000
@@ -177,9 +177,6 @@
         <tal:failed-upload
             condition="context/store_upload_status/enumvalue:FAILEDTOUPLOAD">
           Store upload failed:
-          <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"/>

=== modified file 'lib/lp/snappy/tests/test_snapbuild.py'
--- lib/lp/snappy/tests/test_snapbuild.py	2018-10-04 12:54:04 +0000
+++ lib/lp/snappy/tests/test_snapbuild.py	2018-10-09 10:40:56 +0000
@@ -455,6 +455,32 @@
             SnapBuildStoreUploadStatus.FAILEDTORELEASE,
             build.store_upload_status)
 
+    def test_store_upload_error_messages_no_job(self):
+        build = self.factory.makeSnapBuild(status=BuildStatus.FULLYBUILT)
+        self.assertEqual([], build.store_upload_error_messages)
+
+    def test_store_upload_error_messages_job_no_error(self):
+        build = self.factory.makeSnapBuild(status=BuildStatus.FULLYBUILT)
+        getUtility(ISnapStoreUploadJobSource).create(build)
+        self.assertEqual([], build.store_upload_error_messages)
+
+    def test_store_upload_error_messages_job_error_messages(self):
+        build = self.factory.makeSnapBuild(status=BuildStatus.FULLYBUILT)
+        job = getUtility(ISnapStoreUploadJobSource).create(build)
+        removeSecurityProxy(job).error_messages = [
+            {"message": "Scan failed.", "link": "link1"},
+            ]
+        self.assertEqual(
+            [{"message": "Scan failed.", "link": "link1"}],
+            build.store_upload_error_messages)
+
+    def test_store_upload_error_messages_job_error_message(self):
+        build = self.factory.makeSnapBuild(status=BuildStatus.FULLYBUILT)
+        job = getUtility(ISnapStoreUploadJobSource).create(build)
+        removeSecurityProxy(job).error_message = "Boom"
+        self.assertEqual(
+            [{"message": "Boom"}], build.store_upload_error_messages)
+
     def test_scheduleStoreUpload(self):
         # A build not previously uploaded to the store can be uploaded
         # manually.