Merge lp:~james-w/launchpad/code-import-request into lp:launchpad

[James Westby]

ICodeImport.requestImport() is now available over the API.

Hi,

This branch adds ICodeImport.requestImport() over the API.

It first adds the method on the interface and model, with associated tests, then exports it over the
webservice with a test that calls the method using that. Lastly it ports the browser code to use
the method to reduce duplication.

The most interesting thing here is the exception handling.

To make a nice API for scripting it doesn't error if it is already queued, as the outcome is the same,
but the browser code needs to know this, so it passes a parameter to find out.

Catching the exceptions in the browser code isn't too pretty, but seemed like the best way to balance
the needs of the two APIs.

The exceptions that can be thrown during webservice access are both marked to trigger a 400 response.
This isn't ideal, as scripts will have a tough time distinguishing them, but choosing arbitrary status codes
to do that is equally ugly. The docstring at least explains what can happen and tells you how to check
for it.

Thanks,

James

[James Westby (james-w)]

Hi,

This adds requestImport() to ICodeImport to allow you to request
that an import happens ASAP.

Thanks,

James

[Michael Hudson-Doyle (mwhudson)]

Subject to a few test cleanups mentioned on skype, this is fine.

review: Approve

=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2010-03-25 15:28:49 +0000
+++ lib/lp/code/browser/branch.py	2010-04-14 11:34:34 +0000
@@ -81,10 +81,12 @@
 from lp.code.browser.branchmergeproposal import (
     latest_proposals_for_each_branch)
 from lp.code.enums import (
-    BranchLifecycleStatus, BranchType, CodeImportJobState,
+    BranchLifecycleStatus, BranchType,
     CodeImportResultStatus, CodeImportReviewStatus, RevisionControlSystems,
     UICreatableBranchType)
-from lp.code.errors import InvalidBranchMergeProposal
+from lp.code.errors import (
+    CodeImportAlreadyRequested, CodeImportAlreadyRunning,
+    CodeImportNotInReviewedState, InvalidBranchMergeProposal)
 from lp.code.interfaces.branch import (
     BranchCreationForbidden, BranchExists, IBranch,
     user_has_special_branch_access)
@@ -1317,26 +1319,23 @@
 
     @action('Import Now', name='request')
     def request_import_action(self, action, data):
-        if self.context.code_import.import_job is None:
+        try:
+            self.context.code_import.requestImport(
+                self.user, error_if_already_requested=True)
+            self.request.response.addNotification(
+                "Import will run as soon as possible.")
+        except CodeImportNotInReviewedState:
             self.request.response.addNotification(
                 "This import is no longer being updated automatically.")
-        elif (self.context.code_import.import_job.state !=
-              CodeImportJobState.PENDING):
-            assert (self.context.code_import.import_job.state ==
-                    CodeImportJobState.RUNNING)
+        except CodeImportAlreadyRunning:
             self.request.response.addNotification(
                 "The import is already running.")
-        elif self.context.code_import.import_job.requesting_user is not None:
-            user = self.context.code_import.import_job.requesting_user
+        except CodeImportAlreadyRequested, e:
+            user = e.requesting_user
             adapter = queryAdapter(user, IPathAdapter, 'fmt')
             self.request.response.addNotification(
                 structured("The import has already been requested by %s." %
                            adapter.link(None)))
-        else:
-            getUtility(ICodeImportJobWorkflow).requestJob(
-                self.context.code_import.import_job, self.user)
-            self.request.response.addNotification(
-                "Import will run as soon as possible.")
 
     @property
     def prefix(self):

=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml	2010-04-12 14:51:11 +0000
+++ lib/lp/code/configure.zcml	2010-04-14 11:34:34 +0000
@@ -789,7 +789,8 @@
                        getImportDetailsForDisplay"/>
     <require
        permission="launchpad.AnyPerson"
-       attributes="tryFailingImportAgain"/>
+       attributes="tryFailingImportAgain
+                   requestImport"/>
     <require
        permission="launchpad.Edit"
        attributes="updateFromData"/>

=== modified file 'lib/lp/code/errors.py'
--- lib/lp/code/errors.py	2010-04-05 21:38:40 +0000
+++ lib/lp/code/errors.py	2010-04-14 11:34:34 +0000
@@ -8,6 +8,9 @@
     'BadBranchMergeProposalSearchContext',
     'BadStateTransition',
     'BranchMergeProposalExists',
+    'CodeImportAlreadyRequested',
+    'CodeImportAlreadyRunning',
+    'CodeImportNotInReviewedState',
     'ClaimReviewFailed',
     'InvalidBranchMergeProposal',
     'ReviewNotPending',
@@ -68,3 +71,23 @@
 
 class UnknownBranchTypeError(Exception):
     """Raised when the user specifies an unrecognized branch type."""
+
+
+class CodeImportNotInReviewedState(Exception):
+    """Raised when the user requests an import of a non-automatic import."""
+
+    webservice_error(400)
+
+
+class CodeImportAlreadyRequested(Exception):
+    """Raised when the user requests an import that is already requested."""
+
+    def __init__(self, msg, requesting_user):
+        super(CodeImportAlreadyRequested, self).__init__(msg)
+        self.requesting_user = requesting_user
+
+
+class CodeImportAlreadyRunning(Exception):
+    """Raised when the user requests an import that is already running."""
+
+    webservice_error(400)

=== modified file 'lib/lp/code/interfaces/codeimport.py'
--- lib/lp/code/interfaces/codeimport.py	2010-04-01 20:58:42 +0000
+++ lib/lp/code/interfaces/codeimport.py	2010-04-14 11:34:34 +0000
@@ -25,7 +25,8 @@
 from lp.code.interfaces.branch import IBranch
 
 from lazr.restful.declarations import (
-    export_as_webservice_entry, exported)
+    call_with, export_as_webservice_entry, exported, export_write_operation,
+    REQUEST_USER)
 from lazr.restful.fields import ReferenceChoice
 
 
@@ -177,6 +178,33 @@
         :param user: the user who is requesting the import be tried again.
         """
 
+    @call_with(requester=REQUEST_USER)
+    @export_write_operation()
+    def requestImport(requester, error_if_already_requested=False):
+        """Request that an import be tried soon.
+
+        This method will schedule an import to happen soon for this branch.
+
+        The import must be in the Reviewed state, if not then a
+        CodeImportNotInReviewedState error will be thrown. If using the
+        API then a status code of 400 will result.
+
+        If the import is already running then a CodeImportAlreadyRunning
+        error will be thrown. If using the API then a status code of
+        400 will result.
+
+        The two cases can be distinguished over the API by seeing if the
+        exception names appear in the body of the response.
+
+        If used over the API and the request has already been made then this
+        method will silently do nothing.
+        If called internally then the error_if_already_requested parameter
+        controls whether a CodeImportAlreadyRequested exception will be
+        thrown in that situation.
+
+        :return: None
+        """
+
 
 class ICodeImportSet(Interface):
     """Interface representing the set of code imports."""

=== modified file 'lib/lp/code/interfaces/webservice.py'
--- lib/lp/code/interfaces/webservice.py	2010-04-01 23:04:10 +0000
+++ lib/lp/code/interfaces/webservice.py	2010-04-14 11:34:34 +0000
@@ -5,7 +5,9 @@
 
 # The exceptions are imported so that they can produce the special
 # status code defined by webservice_error when they are raised.
-from lp.code.errors import BranchMergeProposalExists
+from lp.code.errors import (
+    BranchMergeProposalExists, CodeImportAlreadyRunning,
+    CodeImportNotInReviewedState)
 from lp.code.interfaces.branch import (
     IBranch, IBranchSet, BranchCreatorNotMemberOfOwnerTeam,
     BranchCreatorNotOwner, BranchExists)

=== modified file 'lib/lp/code/model/codeimport.py'
--- lib/lp/code/model/codeimport.py	2010-04-01 20:58:42 +0000
+++ lib/lp/code/model/codeimport.py	2010-04-14 11:34:34 +0000
@@ -37,6 +37,9 @@
 from lp.code.enums import (
     BranchType, CodeImportJobState, CodeImportResultStatus,
     CodeImportReviewStatus, RevisionControlSystems)
+from lp.code.errors import (
+    CodeImportAlreadyRequested, CodeImportAlreadyRunning,
+    CodeImportNotInReviewedState)
 from lp.code.interfaces.codeimport import ICodeImport, ICodeImportSet
 from lp.code.interfaces.codeimportevent import ICodeImportEventSet
 from lp.code.interfaces.codeimportjob import ICodeImportJobWorkflow
@@ -196,6 +199,27 @@
             {'review_status': CodeImportReviewStatus.REVIEWED}, user)
         getUtility(ICodeImportJobWorkflow).requestJob(self.import_job, user)
 
+    def requestImport(self, requester, error_if_already_requested=False):
+        """See `ICodeImport`."""
+        if self.import_job is None: # not in automatic mode
+           raise CodeImportNotInReviewedState("This code import is %s, and "
+                   "must be Reviewed for you to call requestImport."
+                   % self.review_status.name)
+        if (self.import_job.state != CodeImportJobState.PENDING):
+            assert (self.import_job.state == CodeImportJobState.RUNNING)
+            # Already running
+            raise CodeImportAlreadyRunning("This code import is already "
+                    "running.")
+        elif self.import_job.requesting_user is not None:
+            if error_if_already_requested:
+                raise CodeImportAlreadyRequested("This code import has "
+                    "already been requested to run.",
+                    self.import_job.requesting_user)
+        else:
+            getUtility(ICodeImportJobWorkflow).requestJob(
+                self.import_job, requester)
+        return None
+
 
 class CodeImportSet:
     """See `ICodeImportSet`."""

=== modified file 'lib/lp/code/model/tests/test_codeimport.py'
--- lib/lp/code/model/tests/test_codeimport.py	2010-04-01 20:58:42 +0000
+++ lib/lp/code/model/tests/test_codeimport.py	2010-04-14 11:34:34 +0000
@@ -12,6 +12,11 @@
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
+from canonical.launchpad.testing.codeimporthelpers import (
+    make_running_import)
+from lp.code.errors import (
+    CodeImportAlreadyRequested, CodeImportAlreadyRunning,
+    CodeImportNotInReviewedState)
 from lp.code.model.codeimport import CodeImportSet
 from lp.code.model.codeimportevent import CodeImportEvent
 from lp.code.model.codeimportjob import CodeImportJob, CodeImportJobSet
@@ -620,5 +625,67 @@
             requester, code_import.import_job.requesting_user)
 
 
+class TestRequestImport(TestCaseWithFactory):
+    """Tests for `ICodeImport.requestImport`."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        # We have to be logged in to request imports
+        TestCaseWithFactory.setUp(self, user='[e-mail]')
+
+    def test_requestsJob(self):
+        code_import = self.factory.makeCodeImport(
+            git_repo_url=self.factory.getUniqueURL())
+        requester = self.factory.makePerson()
+        old_date = code_import.import_job.date_due
+        code_import.requestImport(requester)
+        self.assertEqual(requester, code_import.import_job.requesting_user)
+        self.assertTrue(code_import.import_job.date_due <= old_date)
+
+    def test_noop_if_already_requested(self):
+        code_import = self.factory.makeCodeImport(
+            git_repo_url=self.factory.getUniqueURL())
+        requester = self.factory.makePerson()
+        code_import.requestImport(requester)
+        old_date = code_import.import_job.date_due
+        code_import.requestImport(requester)
+        # The checks don't matter so much, it's more that we don't get
+        # an exception.
+        self.assertEqual(requester, code_import.import_job.requesting_user)
+        self.assertEqual(old_date, code_import.import_job.date_due)
+
+    def test_optional_error_if_already_requested(self):
+        code_import = self.factory.makeCodeImport(
+            git_repo_url=self.factory.getUniqueURL())
+        requester = self.factory.makePerson()
+        code_import.requestImport(requester)
+        old_date = code_import.import_job.date_due
+        e = self.assertRaises(
+            CodeImportAlreadyRequested, code_import.requestImport, requester,
+            error_if_already_requested=True)
+        self.assertEqual(requester, e.requesting_user)
+
+    def test_exception_on_disabled(self):
+        # get an SVN request, which isn't reviewed by default
+        code_import = self.factory.makeCodeImport(
+            svn_branch_url=self.factory.getUniqueURL())
+        requester = self.factory.makePerson()
+        # which leads to an exception if we try and ask for an import
+        self.assertRaises(
+            CodeImportNotInReviewedState, code_import.requestImport,
+            requester)
+
+    def test_exception_if_already_running(self):
+        code_import = self.factory.makeCodeImport(
+            git_repo_url=self.factory.getUniqueURL())
+        code_import = make_running_import(factory=self.factory,
+            code_import=code_import)
+        requester = self.factory.makePerson()
+        self.assertRaises(
+            CodeImportAlreadyRunning, code_import.requestImport,
+            requester)
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/code/stories/webservice/xx-code-import.txt'
--- lib/lp/code/stories/webservice/xx-code-import.txt	2010-04-13 14:56:00 +0000
+++ lib/lp/code/stories/webservice/xx-code-import.txt	2010-04-14 11:34:34 +0000
@@ -228,3 +228,25 @@
     None
     >>> print representation['date_last_successful']
     None
+
+
+== Requesting an Import ==
+
+You can request that an approved, working import happen soon over the
+API using the requestImport() method.
+
+    >>> login(ANONYMOUS)
+    >>> git_import = factory.makeProductCodeImport(
+    ...     registrant=person, product=product, branch_name='git-import',
+    ...     git_repo_url=factory.getUniqueURL())
+    >>> git_import_url = (
+    ...    '/' + git_import.branch.unique_name + '/+code-import')
+    >>> logout()
+    >>> import_webservice = webservice_for_person(
+    ...     person, permission=OAuthPermission.WRITE_PUBLIC)
+    >>> response = import_webservice.named_post(
+    ...     git_import_url, 'requestImport')
+    >>> print response.status
+    200
+    >>> print response.jsonBody()
+    None