launchpad-reviewers team mailing list archive

[Ioana Lasc <ioana.lasc@xxxxxxxxxxxxx>]

Ioana Lasc has proposed merging ~ilasc/launchpad:remove-old-git-notify-path into launchpad:master.

Commit message:
Remove notify code path used prior to repack

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~ilasc/launchpad/+git/launchpad/+merge/399281
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~ilasc/launchpad:remove-old-git-notify-path into launchpad:master.

diff --git a/lib/lp/code/browser/tests/test_branchmergeproposal.py b/lib/lp/code/browser/tests/test_branchmergeproposal.py
index e8a22f4..7e9e308 100644
--- a/lib/lp/code/browser/tests/test_branchmergeproposal.py
+++ b/lib/lp/code/browser/tests/test_branchmergeproposal.py
@@ -1609,7 +1609,9 @@ class TestBranchMergeProposalView(TestCaseWithFactory):
         self.assertFalse(view.pending_diff)
         git_api = GitAPI(None, None)
         self.assertIsNone(
-            git_api.notify(bmp.source_git_repository.getInternalPath()))
+            git_api.notify(bmp.source_git_repository.getInternalPath(),
+                           {'loose_object_count': 5, 'pack_count': 2},
+                           {'uid': bmp.source_git_repository.owner.id}))
         self.assertTrue(view.pending_diff)
 
     def test_description_is_meta_description(self):
diff --git a/lib/lp/code/interfaces/gitapi.py b/lib/lp/code/interfaces/gitapi.py
index 985865d..ab11a79 100644
--- a/lib/lp/code/interfaces/gitapi.py
+++ b/lib/lp/code/interfaces/gitapi.py
@@ -50,7 +50,7 @@ class IGitAPI(Interface):
                 this repository, otherwise False.
         """
 
-    def notify(translated_path, statistics=None, auth_params=None):
+    def notify(translated_path, statistics, auth_params):
         """Notify of a change to the repository at 'translated_path'.
 
         :param translated_path: The translated path to the repository.  (We
diff --git a/lib/lp/code/model/tests/test_gitrepository.py b/lib/lp/code/model/tests/test_gitrepository.py
index e79c4e0..ce3b938 100644
--- a/lib/lp/code/model/tests/test_gitrepository.py
+++ b/lib/lp/code/model/tests/test_gitrepository.py
@@ -874,7 +874,10 @@ class TestGitRepositoryDeletion(TestCaseWithFactory):
 
     def test_related_GitJobs_deleted(self):
         # A repository with an associated job will delete those jobs.
-        GitAPI(None, None).notify(self.repository.getInternalPath())
+        with person_logged_in(self.repository.owner):
+            GitAPI(None, None).notify(self.repository.getInternalPath(),
+                                  {'loose_object_count': 5, 'pack_count': 2},
+                                  {'uid': self.repository.owner.id})
         store = Store.of(self.repository)
         self.repository.destroySelf()
         # Need to commit the transaction to fire off the constraint checks.
@@ -1471,7 +1474,11 @@ class TestGitRepositoryPendingUpdates(TestCaseWithFactory):
         # clears that flag.
         git_api = GitAPI(None, None)
         repository = self.factory.makeGitRepository()
-        self.assertIsNone(git_api.notify(repository.getInternalPath()))
+        with person_logged_in(repository.owner):
+            self.assertIsNone(git_api.notify(
+                repository.getInternalPath(),
+                {'loose_object_count': 5, 'pack_count': 2},
+                {'uid': repository.owner.id}))
         self.assertTrue(repository.pending_updates)
         [job] = list(getUtility(IGitRefScanJobSource).iterReady())
         with dbuser("branchscanner"):
diff --git a/lib/lp/code/xmlrpc/git.py b/lib/lp/code/xmlrpc/git.py
index 403d36d..5931e94 100644
--- a/lib/lp/code/xmlrpc/git.py
+++ b/lib/lp/code/xmlrpc/git.py
@@ -485,30 +485,17 @@ class GitAPI(LaunchpadXMLRPCView):
         getUtility(IGitRefScanJobSource).create(
             removeSecurityProxy(repository))
 
-    def notify(self, translated_path, statistics=None, auth_params=None):
+    def notify(self, translated_path, statistics, auth_params):
         """See `IGitAPI`."""
         logger = self._getLogger()
-        if statistics is None:
-            logger.info("Request received: notify('%s')", translated_path)
-        else:
-            logger.info("Request received: notify('%s', '%d', '%d')",
+        logger.info("Request received: notify('%s', '%d', '%d')",
                     translated_path, statistics.get('loose_object_count'),
                     statistics.get('pack_count'))
-        if auth_params is not None:
-            requester_id = _get_requester_id(auth_params)
-            result = run_with_login(
-                requester_id, self._notify,
-                translated_path, statistics, auth_params)
-            if isinstance(result, xmlrpc_client.Fault):
-                logger.error("notify failed: %r", result)
-            else:
-                logger.info("notify succeeded: %s" % result)
-            return result
-        else:
-            # XXX ilasc 2021-02-12: This else block will be removed
-            # once turnip always passes statistics and auth_params
-            result = self._notify(
-                None, translated_path, statistics, auth_params)
+
+        requester_id = _get_requester_id(auth_params)
+        result = run_with_login(
+            requester_id, self._notify,
+            translated_path, statistics, auth_params)
         if isinstance(result, xmlrpc_client.Fault):
             logger.error("notify failed: %r", result)
         else:
diff --git a/lib/lp/code/xmlrpc/tests/test_git.py b/lib/lp/code/xmlrpc/tests/test_git.py
index e226038..ea38315 100644
--- a/lib/lp/code/xmlrpc/tests/test_git.py
+++ b/lib/lp/code/xmlrpc/tests/test_git.py
@@ -2071,7 +2071,10 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
         # The notify call creates a GitRefScanJob.
         repository = self.factory.makeGitRepository()
         self.assertIsNone(self.assertDoesNotFault(
-            None, "notify", repository.getInternalPath()))
+            None, "notify",
+            repository.getInternalPath(),
+            {'loose_object_count': 5, 'pack_count': 2},
+            {'uid': repository.owner.id}))
         job_source = getUtility(IGitRefScanJobSource)
         [job] = list(job_source.iterReady())
         self.assertEqual(repository, job.repository)
@@ -2079,7 +2082,11 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
     def test_notify_missing_repository(self):
         # A notify call on a non-existent repository returns a fault and
         # does not create a job.
-        self.assertFault(faults.NotFound, None, "notify", "10000")
+        requester_owner = self.factory.makePerson()
+
+        self.assertFault(faults.NotFound, None, "notify", "10000",
+                         {'loose_object_count': 5, 'pack_count': 2},
+                         {'uid': requester_owner.id})
         job_source = getUtility(IGitRefScanJobSource)
         self.assertEqual([], list(job_source.iterReady()))
 
@@ -2089,7 +2096,10 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
             repository = self.factory.makeGitRepository(
                 information_type=InformationType.PRIVATESECURITY)
             path = repository.getInternalPath()
-        self.assertIsNone(self.assertDoesNotFault(None, "notify", path))
+            self.assertIsNone(self.assertDoesNotFault(
+                None, "notify", path,
+                {'loose_object_count': 5, 'pack_count': 2},
+                {'uid': repository.owner.id}))
         job_source = getUtility(IGitRefScanJobSource)
         [job] = list(job_source.iterReady())
         self.assertEqual(repository, job.repository)