launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-review-queued into lp:launchpad.

Commit message:
Don't treat the case where a snap store upload is queued behind others for review as an error.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/snap-review-queued/+merge/370706
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snap-review-queued into lp:launchpad.

=== modified file 'lib/lp/snappy/browser/tests/test_snapbuild.py'
--- lib/lp/snappy/browser/tests/test_snapbuild.py	2019-05-22 14:57:45 +0000
+++ lib/lp/snappy/browser/tests/test_snapbuild.py	2019-07-29 11:22:08 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test snap package build views."""
@@ -25,6 +25,7 @@
 
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.buildmaster.enums import BuildStatus
+from lp.services.config import config
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.webapp import canonical_url
 from lp.snappy.interfaces.snapbuildjob import ISnapStoreUploadJobSource
@@ -119,6 +120,23 @@
                     text=re.compile(
                         r"^\s*Manage this package in the store\s*$")))))
 
+    def test_store_upload_status_completed_no_url(self):
+        # A package that has been uploaded to the store may lack a URL if
+        # the upload was queued behind others for manual review.
+        build = self.factory.makeSnapBuild(status=BuildStatus.FULLYBUILT)
+        job = getUtility(ISnapStoreUploadJobSource).create(build)
+        naked_job = removeSecurityProxy(job)
+        naked_job.job._status = JobStatus.COMPLETED
+        build_view = create_initialized_view(build, "+index")
+        self.assertThat(build_view(), soupmatchers.HTMLContains(
+            soupmatchers.Within(
+                soupmatchers.Tag(
+                    "store upload status", "li",
+                    attrs={"id": "store-upload-status"}),
+                soupmatchers.Tag(
+                    "store link", "a", attrs={"href": config.snappy.store_url},
+                    text=re.compile(r"^\s*Uploaded to the store\s*$")))))
+
     def test_store_upload_status_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	2019-06-20 16:33:19 +0000
+++ lib/lp/snappy/model/snapstoreclient.py	2019-07-29 11:22:08 +0000
@@ -1,4 +1,4 @@
-# Copyright 2016-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2016-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Communication with the snap store."""
@@ -348,6 +348,17 @@
                 # our upload has been successful
                 if response_data['code'] == 'need_manual_review':
                     return response_data["url"], response_data["revision"]
+                # The review-queued state is a little odd.  It shows up as a
+                # processing error of sorts, and it doesn't contain a URL or
+                # a revision; on the other hand, it means that there's no
+                # point waiting any longer because a manual review might
+                # take an arbitrary amount of time.  We'll just return
+                # (None, None) to indicate that we have no information but
+                # that it's OK to continue.
+                if (response_data["code"] == "processing_error" and
+                    any(error["code"] == "review-queued"
+                        for error in response_data["errors"])):
+                    return None, None
                 error_message = "\n".join(
                     error["message"] for error in response_data["errors"])
                 error_messages = []

=== modified file 'lib/lp/snappy/templates/snapbuild-index.pt'
--- lib/lp/snappy/templates/snapbuild-index.pt	2018-10-09 10:35:44 +0000
+++ lib/lp/snappy/templates/snapbuild-index.pt	2019-07-29 11:22:08 +0000
@@ -199,6 +199,20 @@
             <input type="submit" name="field.actions.upload" value="Retry" />
           </form>
         </tal:failed-release>
+        <tal:uploaded
+            condition="context/store_upload_status/enumvalue:UPLOADED">
+          <tal:comment condition="nothing">
+            If the package is in the uploaded state, we normally have a URL
+            (rendered above), but we might not if the upload was queued for
+            manual review behind other uploads of the same package.  Linking
+            to the front page of the store in that case isn't great, but
+            it's the best we can reasonably do.
+          </tal:comment>
+          <a tal:condition="not: context/store_upload_url"
+             tal:attributes="href modules/lp.services.config/config/snappy/store_url">
+            Uploaded to the store
+          </a>
+        </tal:uploaded>
       </li>
       <li id="store-upload-status"
           tal:condition="python:

=== modified file 'lib/lp/snappy/tests/test_snapbuildjob.py'
--- lib/lp/snappy/tests/test_snapbuildjob.py	2019-06-24 11:21:10 +0000
+++ lib/lp/snappy/tests/test_snapbuildjob.py	2019-07-29 11:22:08 +0000
@@ -497,6 +497,30 @@
         self.assertWebhookDeliveries(
             snapbuild, ["Pending", "Failed to upload"], logger)
 
+    def test_run_scan_review_queued(self):
+        # A run that finds that the store has queued the package behind
+        # others for manual review completes, but without recording a store
+        # URL or revision.
+        logger = self.useFixture(FakeLogger())
+        snapbuild = self.makeSnapBuild()
+        self.assertContentEqual([], snapbuild.store_upload_jobs)
+        job = SnapStoreUploadJob.create(snapbuild)
+        client = FakeSnapStoreClient()
+        client.upload.result = self.status_url
+        client.checkStatus.result = (None, None)
+        self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
+        with dbuser(config.ISnapStoreUploadJobSource.dbuser):
+            run_isolated_jobs([job])
+        self.assertEqual([((snapbuild,), {})], client.upload.calls)
+        self.assertEqual([((self.status_url,), {})], client.checkStatus.calls)
+        self.assertContentEqual([job], snapbuild.store_upload_jobs)
+        self.assertIsNone(job.store_url)
+        self.assertIsNone(job.store_revision)
+        self.assertIsNone(job.error_message)
+        self.assertEqual([], pop_notifications())
+        self.assertWebhookDeliveries(
+            snapbuild, ["Pending", "Uploaded"], logger)
+
     def test_run_release(self):
         # A run configured to automatically release the package to certain
         # channels does so.

=== modified file 'lib/lp/snappy/tests/test_snapstoreclient.py'
--- lib/lp/snappy/tests/test_snapstoreclient.py	2019-06-20 16:33:19 +0000
+++ lib/lp/snappy/tests/test_snapstoreclient.py	2019-07-29 11:22:08 +0000
@@ -1,4 +1,4 @@
-# Copyright 2016-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2016-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for communication with the snap store."""
@@ -642,6 +642,24 @@
             self.client.checkStatus(status_url))
 
     @responses.activate
+    def test_checkStatus_review_queued(self):
+        status_url = "http://sca.example/dev/api/snaps/1/builds/1/status";
+        responses.add(
+            "GET", status_url,
+            json={
+                "errors": [
+                    {"code": "review-queued",
+                     "message": (
+                         "Waiting for previous upload(s) to complete their "
+                         "review process."),
+                     }],
+                "code": "processing_error",
+                "processed": True,
+                "can_release": False,
+                })
+        self.assertEqual((None, None), self.client.checkStatus(status_url))
+
+    @responses.activate
     def test_listChannels(self):
         self._addChannelsResponse()
         self.assertEqual(self.channels, self.client.listChannels())