Merge ~ines-almeida/launchpad:fix-infer-pro-enable-function into launchpad:master

[Ines Almeida]

Catch revision ID not well formatted exception in inferProEnable

Currently, the garbo job to populate the pro-enable value is failing in production with a `ValueError: Revision ID '<revision_id>' is not well-formed.`

Here we are catching that error and setting those to False by default.

Also took the chance to add more unit tests.

[Colin Watson (cjwatson)]

review: Needs Fixing

[Ines Almeida (ines-almeida)]

Sounds good, updated

[Colin Watson (cjwatson)]

review: Approve

diff --git a/lib/lp/code/model/branchhosting.py b/lib/lp/code/model/branchhosting.py
index 365a6c8..3be5857 100644
--- a/lib/lp/code/model/branchhosting.py
+++ b/lib/lp/code/model/branchhosting.py
@@ -31,6 +31,10 @@ class RequestExceptionWrapper(requests.RequestException):
     """A non-requests exception that occurred during a request."""
 
 
+class InvalidRevisionException(Exception):
+    """An exception thrown when a revision ID is not valid"""
+
+
 @implementer(IBranchHostingClient)
 class BranchHostingClient:
     """A client for the Bazaar Loggerhead API."""
@@ -94,7 +98,9 @@ class BranchHostingClient:
         attacks.
         """
         if rev is not None and "/" in rev:
-            raise ValueError("Revision ID '%s' is not well-formed." % rev)
+            raise InvalidRevisionException(
+                "Revision ID '%s' is not well-formed." % rev
+            )
 
     def getDiff(
         self, branch_id, new, old=None, context_lines=None, logger=None
diff --git a/lib/lp/code/model/tests/test_branchhosting.py b/lib/lp/code/model/tests/test_branchhosting.py
index 05931b2..76e7aa2 100644
--- a/lib/lp/code/model/tests/test_branchhosting.py
+++ b/lib/lp/code/model/tests/test_branchhosting.py
@@ -19,6 +19,7 @@ from zope.security.proxy import removeSecurityProxy
 
 from lp.code.errors import BranchFileNotFound, BranchHostingFault
 from lp.code.interfaces.branchhosting import IBranchHostingClient
+from lp.code.model.branchhosting import InvalidRevisionException
 from lp.services.job.interfaces.job import IRunnableJob, JobStatus
 from lp.services.job.model.job import Job
 from lp.services.job.runner import BaseRunnableJob, JobRunner
@@ -90,10 +91,14 @@ class TestBranchHostingClient(TestCase):
         self.assertRequest("+branch-id/123/diff/2/1?context_lines=4")
 
     def test_getDiff_bad_old_revision(self):
-        self.assertRaises(ValueError, self.client.getDiff, 123, "x/y", "1")
+        self.assertRaises(
+            InvalidRevisionException, self.client.getDiff, 123, "x/y", "1"
+        )
 
     def test_getDiff_bad_new_revision(self):
-        self.assertRaises(ValueError, self.client.getDiff, 123, "1", "x/y")
+        self.assertRaises(
+            InvalidRevisionException, self.client.getDiff, 123, "1", "x/y"
+        )
 
     def test_getDiff_failure(self):
         with self.mockRequests("GET", status=400):
@@ -144,7 +149,11 @@ class TestBranchHostingClient(TestCase):
 
     def test_getBlob_bad_revision(self):
         self.assertRaises(
-            ValueError, self.client.getBlob, 123, "file-name", rev="x/y"
+            InvalidRevisionException,
+            self.client.getBlob,
+            123,
+            "file-name",
+            rev="x/y",
         )
 
     def test_getBlob_failure(self):
diff --git a/lib/lp/snappy/model/snap.py b/lib/lp/snappy/model/snap.py
index 92dd911..c47ec1e 100644
--- a/lib/lp/snappy/model/snap.py
+++ b/lib/lp/snappy/model/snap.py
@@ -78,6 +78,7 @@ from lp.code.interfaces.gitrepository import (
 )
 from lp.code.model.branch import Branch
 from lp.code.model.branchcollection import GenericBranchCollection
+from lp.code.model.branchhosting import InvalidRevisionException
 from lp.code.model.branchnamespace import (
     BRANCH_POLICY_ALLOWED_TYPES,
     BRANCH_POLICY_REQUIRED_GRANTS,
@@ -1671,6 +1672,7 @@ class SnapSet:
             MissingSnapcraftYaml,
             CannotFetchSnapcraftYaml,
             CannotParseSnapcraftYaml,
+            InvalidRevisionException,
         ):
             pass
         else:
diff --git a/lib/lp/snappy/tests/test_snap.py b/lib/lp/snappy/tests/test_snap.py
index 0e6a31e..3cc6d7f 100644
--- a/lib/lp/snappy/tests/test_snap.py
+++ b/lib/lp/snappy/tests/test_snap.py
@@ -3696,6 +3696,47 @@ class TestSnapProcessors(TestCaseWithFactory):
         self.assertFalse(inferProEnable(refs[6]))  # Snap w/out snapcraft.yaml
         self.assertFalse(inferProEnable(None))  # Snap w/out ref or branch
 
+    def test_inferProEnable_branches(self):
+        """With a branch as a context, inferProEnable returns the inferred
+        values for pro-enable as expected (see test description above)
+        """
+        branches = [
+            removeSecurityProxy(self.factory.makeBranch()) for _ in range(7)
+        ]
+        blobs = {
+            branch.id: blob
+            for branch, blob in (
+                (branches[0], b"name: test-snap\n"),
+                (branches[1], b"name: test-snap\nbase: core\n"),
+                (branches[2], b"name: test-snap\nbase: core18\n"),
+                (branches[3], b"name: test-snap\nbuild-base: devel\n"),
+                (branches[4], b"name: core\ntype: base\n"),
+                (branches[5], b"name: core18\ntype: base\n"),
+            )
+        }
+        self.useFixture(
+            BranchHostingFixture()
+        ).getBlob = lambda branch_id, *args, **kwargs: blobs.get(branch_id)
+
+        inferProEnable = getUtility(ISnapSet).inferProEnable
+        self.assertTrue(inferProEnable(branches[0]))  # Snap w no base
+        self.assertTrue(inferProEnable(branches[1]))  # Snap w 'core' base
+        self.assertFalse(inferProEnable(branches[2]))  # Snap w 'core18' base
+        self.assertFalse(inferProEnable(branches[3]))  # Snap w only build-base
+        self.assertTrue(inferProEnable(branches[4]))  # 'core' snap itself
+        self.assertFalse(inferProEnable(branches[5]))  # 'core18' snap itself
+        self.assertFalse(inferProEnable(branches[6]))  # w/out snapcraft.yaml
+        self.assertFalse(inferProEnable(None))  # w/out ref or branch
+
+    def test_inferProEnable_bad_revision(self):
+        """A branch with an invalid revision ID (or an invalid last_scanned_id)
+        will have its pro-enable value set to False.
+        """
+        branch = self.factory.makeBranch()
+        branch.last_scanned_id = "bad/revision"
+        inferProEnable = getUtility(ISnapSet).inferProEnable
+        self.assertFalse(inferProEnable(removeSecurityProxy(branch)))
+
 
 class TestSnapWebservice(TestCaseWithFactory):
     layer = LaunchpadFunctionalLayer