launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/branch-unscan-api into lp:launchpad.

Commit message:
Replace unscan-branch.py with a Branch.unscan() webservice call.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/branch-unscan-api/+merge/201704

A year ago I added an unscan-branch.py script to work around around bug #904683 and bug #1018460. This branch replaces it with a Branch.unscan() webservice call, so we don't have to poke webops for it every time.
-- 
https://code.launchpad.net/~wgrant/launchpad/branch-unscan-api/+merge/201704
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/branch-unscan-api into lp:launchpad.

=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py	2013-02-21 05:43:21 +0000
+++ lib/lp/code/interfaces/branch.py	2014-01-15 01:40:26 +0000
@@ -1048,6 +1048,15 @@
             with the `IBranchNamespacePolicy`.
         """
 
+    @operation_parameters(
+        rescan=Bool(
+            title=_("Scan the branch after resetting its scan data."),
+            default=True))
+    @export_write_operation()
+    @operation_for_version('devel')
+    def unscan(rescan=True):
+        """Reset this branch's scan data and optionally request a scan."""
+
 
 class IBranchEditableAttributes(Interface):
     """IBranch attributes that can be edited.

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2013-11-22 01:11:26 +0000
+++ lib/lp/code/model/branch.py	2014-01-15 01:40:26 +0000
@@ -1153,6 +1153,17 @@
         else:
             raise AssertionError("No pull URL for %r" % (self, ))
 
+    def unscan(self, rescan=True):
+        from lp.code.model.branchjob import BranchScanJob
+        old_scanned_id = self.last_scanned_id
+        Store.of(self).find(BranchRevision, branch=self).remove()
+        self.last_scanned = self.last_scanned_id = None
+        self.revision_count = 0
+        if rescan:
+            job = BranchScanJob.create(self)
+            job.celeryRunOnCommit()
+        return (self.last_mirrored_id, old_scanned_id)
+
     def requestMirror(self):
         """See `IBranch`."""
         if self.branch_type in (BranchType.REMOTE, BranchType.HOSTED):

=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py	2013-11-21 03:31:46 +0000
+++ lib/lp/code/model/tests/test_branch.py	2014-01-15 01:40:26 +0000
@@ -10,13 +10,14 @@
     datetime,
     timedelta,
     )
+import json
 
 from bzrlib.branch import Branch
 from bzrlib.bzrdir import BzrDir
 from bzrlib.revision import NULL_REVISION
 from pytz import UTC
-import simplejson
 from sqlobject import SQLObjectNotFound
+from storm.exceptions import LostObjectError
 from storm.locals import Store
 from testtools import ExpectedException
 from testtools.matchers import (
@@ -25,6 +26,7 @@
     )
 import transaction
 from zope.component import getUtility
+from zope.security.interfaces import Unauthorized
 from zope.security.proxy import removeSecurityProxy
 
 from lp import _
@@ -138,12 +140,15 @@
 from lp.services.osutils import override_environ
 from lp.services.propertycache import clear_property_cache
 from lp.services.webapp.authorization import check_permission
-from lp.services.webapp.interfaces import IOpenLaunchBag
+from lp.services.webapp.interfaces import (
+    IOpenLaunchBag,
+    OAuthPermission,
+    )
 from lp.testing import (
     admin_logged_in,
     ANONYMOUS,
+    api_url,
     celebrity_logged_in,
-    launchpadlib_for,
     login,
     login_person,
     logout,
@@ -153,11 +158,9 @@
     TestCaseWithFactory,
     time_counter,
     WebServiceTestCase,
-    ws_object,
     )
 from lp.testing.factory import LaunchpadObjectFactory
 from lp.testing.layers import (
-    AppServerLayer,
     CeleryBranchWriteJobLayer,
     CeleryBzrsyncdJobLayer,
     DatabaseFunctionalLayer,
@@ -165,6 +168,7 @@
     LaunchpadZopelessLayer,
     ZopelessAppServerLayer,
     )
+from lp.testing.pages import webservice_for_person
 
 
 def create_knit(test_case):
@@ -3250,7 +3254,7 @@
     def test_setMergeQueueConfig(self):
         """Test Branch.setMergeQueueConfig."""
         branch = self.factory.makeBranch()
-        config = simplejson.dumps({
+        config = json.dumps({
             'path': '/',
             'test': 'make test',
             })
@@ -3272,56 +3276,111 @@
                 config)
 
 
+class TestBranchUnscan(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_unscan(self):
+        # Unscanning a branch resets the scan data, including the
+        # BranchRevisions, last_scanned_id and revision_count.
+        branch = self.factory.makeAnyBranch()
+        self.factory.makeRevisionsForBranch(branch=branch)
+        head = branch.getBranchRevision(revision_id=branch.last_scanned_id)
+        self.assertEqual(5, head.sequence)
+        self.assertEqual(5, branch.revision_count)
+
+        with person_logged_in(branch.owner):
+            self.assertEqual(
+                (head.revision.revision_id, head.revision.revision_id),
+                branch.unscan())
+        transaction.commit()
+
+        self.assertIs(None, branch.last_scanned_id)
+        self.assertEqual(0, branch.revision_count)
+        self.assertRaises(LostObjectError, getattr, head, 'sequence')
+
+    def test_rescan(self):
+        branch = self.factory.makeAnyBranch()
+        self.assertEqual(
+            0, Store.of(branch).find(BranchJob, branch=branch).count())
+        with person_logged_in(branch.owner):
+            branch.unscan(rescan=True)
+        self.assertEqual(
+            1, Store.of(branch).find(BranchJob, branch=branch).count())
+
+    def test_no_rescan(self):
+        branch = self.factory.makeAnyBranch()
+        self.assertEqual(
+            0, Store.of(branch).find(BranchJob, branch=branch).count())
+        with person_logged_in(branch.owner):
+            branch.unscan(rescan=False)
+        self.assertEqual(
+            0, Store.of(branch).find(BranchJob, branch=branch).count())
+
+    def test_security(self):
+        branch = self.factory.makeAnyBranch()
+
+        # Random users can't unscan a branch.
+        with person_logged_in(self.factory.makePerson()):
+            self.assertRaises(Unauthorized, getattr, branch, 'unscan')
+
+        # But the owner can.
+        with person_logged_in(branch.owner):
+            branch.unscan()
+
+        # And so can commercial-admins (and maybe registry too,
+        # eventually).
+        with person_logged_in(
+                getUtility(ILaunchpadCelebrities).commercial_admin):
+            branch.unscan()
+
+
 class TestWebservice(TestCaseWithFactory):
     """Tests for the webservice."""
 
-    layer = AppServerLayer
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestWebservice, self).setUp()
+        self.branch_db = self.factory.makeBranch()
+        self.branch_url = api_url(self.branch_db)
+        self.webservice = webservice_for_person(
+            self.branch_db.owner, permission=OAuthPermission.WRITE_PUBLIC)
 
     def test_set_merge_queue(self):
-        """Test that the merge queue can be set properly."""
-        with person_logged_in(ANONYMOUS):
-            db_queue = self.factory.makeBranchMergeQueue()
-            db_branch = self.factory.makeBranch()
-            launchpad = launchpadlib_for('test', db_branch.owner,
-                service_root=self.layer.appserver_root_url('api'))
-
-        branch = ws_object(launchpad, db_branch)
-        queue = ws_object(launchpad, db_queue)
-        branch.merge_queue = queue
-        branch.lp_save()
-
-        branch2 = ws_object(launchpad, db_branch)
-        self.assertEqual(branch2.merge_queue, queue)
-
-    def test_set_configuration(self):
-        """Test the mutator for setting configuration."""
-        with person_logged_in(ANONYMOUS):
-            db_branch = self.factory.makeBranch()
-            launchpad = launchpadlib_for('test', db_branch.owner,
-                service_root=self.layer.appserver_root_url('api'))
-
-        configuration = simplejson.dumps({'test': 'make check'})
-
-        branch = ws_object(launchpad, db_branch)
-        branch.merge_queue_config = configuration
-        branch.lp_save()
-
-        branch2 = ws_object(launchpad, db_branch)
-        self.assertEqual(branch2.merge_queue_config, configuration)
+        """Test that the merge queue and config can be set properly."""
+        with person_logged_in(ANONYMOUS):
+            queue_db = self.factory.makeBranchMergeQueue()
+            queue_url = api_url(queue_db)
+
+        config = json.dumps({'test': 'make check'})
+        self.webservice.patch(
+            self.branch_url, "application/json",
+            json.dumps({
+                "merge_queue_link": queue_url,
+                "merge_queue_config": config,
+                }),
+            api_version='devel')
+        with person_logged_in(ANONYMOUS):
+            self.assertEqual(self.branch_db.merge_queue, queue_db)
+            self.assertEqual(self.branch_db.merge_queue_config, config)
 
     def test_transitionToInformationType(self):
         """Test transitionToInformationType() API arguments."""
-        product = self.factory.makeProduct()
-        self.factory.makeCommercialSubscription(product)
-        with person_logged_in(product.owner):
-            product.setBranchSharingPolicy(
-                BranchSharingPolicy.PUBLIC_OR_PROPRIETARY)
-            db_branch = self.factory.makeBranch(product=product)
-            launchpad = launchpadlib_for('test', db_branch.owner,
-                service_root=self.layer.appserver_root_url('api'))
-
-        branch = ws_object(launchpad, db_branch)
-        branch.transitionToInformationType(information_type='Proprietary')
-
-        updated_branch = ws_object(launchpad, db_branch)
-        self.assertEqual('Proprietary', updated_branch.information_type)
+        self.webservice.named_post(
+            self.branch_url, 'transitionToInformationType',
+            information_type='Private Security', api_version='devel')
+        with admin_logged_in():
+            self.assertEqual(
+                'Private Security', self.branch_db.information_type.title)
+
+    def test_unscan(self):
+        """Test unscan() API call."""
+        with admin_logged_in():
+            self.assertEqual(
+                0, len(list(getUtility(IBranchScanJobSource).iterReady())))
+        self.webservice.named_post(
+            self.branch_url, 'unscan', rescan=True, api_version='devel')
+        with admin_logged_in():
+            self.assertEqual(
+                1, len(list(getUtility(IBranchScanJobSource).iterReady())))

=== removed file 'lib/lp/code/scripts/tests/test_unscanbranch.py'
--- lib/lp/code/scripts/tests/test_unscanbranch.py	2012-10-10 05:48:24 +0000
+++ lib/lp/code/scripts/tests/test_unscanbranch.py	1970-01-01 00:00:00 +0000
@@ -1,34 +0,0 @@
-# Copyright 2012 Canonical Ltd.  This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-from storm.exceptions import LostObjectError
-from storm.store import Store
-
-from lp.code.model.branchjob import BranchJob
-from lp.code.scripts.unscanbranch import UnscanBranchScript
-from lp.services.log.logger import DevNullLogger
-from lp.testing import TestCaseWithFactory
-from lp.testing.layers import ZopelessDatabaseLayer
-
-
-class TestUnscanBranchScript(TestCaseWithFactory):
-
-    layer = ZopelessDatabaseLayer
-
-    def test_unscanbranch_script(self):
-        branch = self.factory.makeAnyBranch()
-        self.factory.makeRevisionsForBranch(branch=branch)
-        head = branch.getBranchRevision(revision_id=branch.last_scanned_id)
-        self.assertEqual(5, head.sequence)
-        self.assertEqual(5, branch.revision_count)
-        self.assertEqual(
-            1, Store.of(branch).find(BranchJob, branch=branch).count())
-
-        UnscanBranchScript(
-            "unscan-branch", test_args=['--rescan', branch.displayname],
-            logger=DevNullLogger()).main()
-
-        self.assertIs(None, branch.last_scanned_id)
-        self.assertRaises(LostObjectError, getattr, head, 'sequence')
-        self.assertEqual(
-            2, Store.of(branch).find(BranchJob, branch=branch).count())

=== removed file 'lib/lp/code/scripts/unscanbranch.py'
--- lib/lp/code/scripts/unscanbranch.py	2013-06-20 05:50:00 +0000
+++ lib/lp/code/scripts/unscanbranch.py	1970-01-01 00:00:00 +0000
@@ -1,67 +0,0 @@
-# Copyright 2012 Canonical Ltd.  This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-__metaclass__ = type
-__all__ = [
-    'UnscanBranchScript',
-    ]
-
-
-import transaction
-from zope.component import getUtility
-
-from lp.code.interfaces.branchlookup import IBranchLookup
-from lp.code.model.branchjob import BranchScanJob
-from lp.code.model.branchrevision import BranchRevision
-from lp.services.database.interfaces import IStore
-from lp.services.scripts.base import LaunchpadScript
-
-
-class UnscanBranchScript(LaunchpadScript):
-    """Unscan branches.
-
-    Resets the database scan data (eg. BranchRevision records and
-    last_scanned_id) for a set of branches, and optionally requests a
-    rescan.
-
-    Mostly useful for working around performance bugs in the branch scanner
-    that don't affect fresh branches.
-    """
-
-    description = __doc__
-    usage = "%prog <branch URL>..."
-
-    def add_my_options(self):
-        self.parser.add_option(
-            "--rescan", dest="rescan", action="store_true", default=False,
-            help="Request a rescan of the branches after unscanning them.")
-
-    def main(self):
-        if len(self.args) == 0:
-            self.parser.error("Wrong number of arguments.")
-
-        for url in self.args:
-            branch = getUtility(IBranchLookup).getByUrl(url)
-            if branch is None:
-                self.logger.error(
-                    "Could not find branch at %s" % url)
-                continue
-            self.unscan(branch)
-
-    def unscan(self, branch):
-        self.logger.info(
-            "Unscanning %s (last scanned id: %s)", branch.displayname,
-            branch.last_scanned_id)
-        self.logger.debug("Purging BranchRevisions.")
-        IStore(BranchRevision).find(BranchRevision, branch=branch).remove()
-
-        self.logger.debug("Resetting scan data.")
-        branch.last_scanned = branch.last_scanned_id = None
-        branch.revision_count = 0
-
-        if self.options.rescan:
-            self.logger.debug("Requesting rescan.")
-            job = BranchScanJob.create(branch)
-            job.celeryRunOnCommit()
-
-        transaction.commit()

=== removed file 'scripts/unscan-branch.py'
--- scripts/unscan-branch.py	2012-10-10 05:48:24 +0000
+++ scripts/unscan-branch.py	1970-01-01 00:00:00 +0000
@@ -1,12 +0,0 @@
-#!/usr/bin/python -S
-#
-# Copyright 2012 Canonical Ltd.  This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-import _pythonpath
-
-from lp.code.scripts.unscanbranch import UnscanBranchScript
-
-
-if __name__ == '__main__':
-    UnscanBranchScript("unscan-branch", dbuser='branchscanner').run()