launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

Thiago F. Pappacena has proposed merging ~pappacena/launchpad:gitref-and-mp-pages-without-oops-on-failure into launchpad:master.

Commit message:
Avoiding crashes on MP and gitrefs pages when something goes wrong on git code hosting

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1923895 in Launchpad itself: "OOPS on MP pages when Turnip is not fully available"
  https://bugs.launchpad.net/launchpad/+bug/1923895

For more details, see:
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/401248
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:gitref-and-mp-pages-without-oops-on-failure into launchpad:master.

diff --git a/lib/lp/code/browser/branchmergeproposal.py b/lib/lp/code/browser/branchmergeproposal.py
index 1662539..659a5c9 100644
--- a/lib/lp/code/browser/branchmergeproposal.py
+++ b/lib/lp/code/browser/branchmergeproposal.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Views, navigation and actions for BranchMergeProposals."""
@@ -88,6 +88,7 @@ from lp.code.errors import (
     BranchMergeProposalExists,
     ClaimReviewFailed,
     DiffNotFound,
+    GitRepositoryScanFault,
     InvalidBranchMergeProposal,
     WrongBranchMergeProposal,
     )
@@ -133,6 +134,12 @@ from lp.services.webapp.interfaces import ILaunchBag
 from lp.services.webapp.menu import NavigationMenu
 
 
+GIT_HOSTING_ERROR_MSG = (
+    "There was an error fetching revisions from git servers. "
+    "Please, try again in some minutes. If the problem persists, "
+    "contact Launchpad support.")
+
+
 def latest_proposals_for_each_branch(proposals):
     """Returns the most recent merge proposals for any particular branch.
 
@@ -359,10 +366,14 @@ class BranchMergeProposalActionNavigationMenu(NavigationMenu,
 
 class UnmergedRevisionsMixin:
     """Provides the methods needed to show unmerged revisions."""
+    # This is set at self.unlanded_revisions, and should be used at view level
+    # as self.commit_infos_message (see git-macros.pt).
+    _unlanded_revisions_message = None
 
     @cachedproperty
     def unlanded_revisions(self):
         """Return the unlanded revisions from the source branch."""
+        self._unlanded_revisions_message = ''
         with reduced_timeout(1.0, webapp_max=5.0):
             try:
                 return self.context.getUnlandedSourceBranchRevisions()
@@ -374,6 +385,16 @@ class UnmergedRevisionsMixin:
                         self.context.merge_source.identity,
                         self.context.merge_target.identity))
                 return []
+            except GitRepositoryScanFault as e:
+                log.exception("Error scanning git repository: %s" % e)
+                self._unlanded_revisions_message = GIT_HOSTING_ERROR_MSG
+                return []
+
+    @property
+    def commit_infos_message(self):
+        if self._unlanded_revisions_message is None:
+            self.unlanded_revisions
+        return self._unlanded_revisions_message
 
     @property
     def pending_updates(self):
@@ -664,6 +685,7 @@ class BranchMergeProposalView(LaunchpadFormView, UnmergedRevisionsMixin,
         merge_proposal = self.context
         with reduced_timeout(1.0, webapp_max=5.0):
             try:
+                self._conversation_message = ''
                 groups = list(merge_proposal.getRevisionsSinceReviewStart())
             except TimeoutError:
                 log.exception(
@@ -673,6 +695,10 @@ class BranchMergeProposalView(LaunchpadFormView, UnmergedRevisionsMixin,
                         merge_proposal.merge_source.identity,
                         merge_proposal.merge_target.identity))
                 groups = []
+            except GitRepositoryScanFault as e:
+                log.exception("Error scanning git repository: %s" % e)
+                self._conversation_message = GIT_HOSTING_ERROR_MSG
+                groups = []
         source = merge_proposal.merge_source
         if IBranch.providedBy(source):
             source = DecoratedBranch(source)
@@ -710,6 +736,12 @@ class BranchMergeProposalView(LaunchpadFormView, UnmergedRevisionsMixin,
         self._populate_previewdiffs(comments)
         return CodeReviewConversation(comments)
 
+    @property
+    def conversation_message(self):
+        if self._conversation_message is None:
+            self.conversation
+        return self._conversation_message
+
     def _populate_previewdiffs(self, comments):
         """Lookup and populate caches for 'previewdiff_id'.
 
diff --git a/lib/lp/code/browser/gitref.py b/lib/lp/code/browser/gitref.py
index ff01a72..89577c9 100644
--- a/lib/lp/code/browser/gitref.py
+++ b/lib/lp/code/browser/gitref.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Git reference views."""
@@ -41,7 +41,10 @@ from lp.code.browser.branchmergeproposal import (
 from lp.code.browser.sourcepackagerecipelisting import HasRecipesMenuMixin
 from lp.code.browser.widgets.gitref import GitRefWidget
 from lp.code.enums import GitRepositoryType
-from lp.code.errors import InvalidBranchMergeProposal
+from lp.code.errors import (
+    GitRepositoryScanFault,
+    InvalidBranchMergeProposal,
+    )
 from lp.code.interfaces.branchmergeproposal import IBranchMergeProposal
 from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference
 from lp.code.interfaces.gitref import IGitRef
@@ -110,6 +113,10 @@ class GitRefContextMenu(ContextMenu, HasRecipesMenuMixin, HasSnapsMenuMixin):
 
 class GitRefView(LaunchpadView, HasSnapsViewMixin):
 
+    # This is set at self.commit_infos, and should be accessed by the view
+    # as self.commit_info_message.
+    _commit_info_message = None
+
     @property
     def label(self):
         return self.context.display_name
@@ -219,9 +226,27 @@ class GitRefView(LaunchpadView, HasSnapsViewMixin):
 
     @cachedproperty
     def commit_infos(self):
-        return self.context.getLatestCommits(
-            extended_details=True, user=self.user, handle_timeout=True,
-            logger=log)
+        try:
+            self._commit_info_message = ''
+            return self.context.getLatestCommits(
+                extended_details=True, user=self.user, handle_timeout=True,
+                logger=log)
+        except GitRepositoryScanFault as e:
+            log.error("There was an error fetching git commit info: %s" % e)
+            self._commit_info_message = (
+                "There was an error while fetching commit information from "
+                "code hosting service. Please, try again in some minutes. "
+                "If the problem persists, contact Launchpad support.")
+            return []
+        except Exception as e:
+            log.error("There was an error scanning %s: (%s) %s" %
+                      (self.context, e.__class__, e))
+            raise
+
+    def commit_infos_message(self):
+        if self._commit_info_message is None:
+            self.commit_infos
+        return self._commit_info_message
 
     @property
     def recipes_link(self):
diff --git a/lib/lp/code/browser/tests/test_branchmergeproposal.py b/lib/lp/code/browser/tests/test_branchmergeproposal.py
index 7e9e308..c320022 100644
--- a/lib/lp/code/browser/tests/test_branchmergeproposal.py
+++ b/lib/lp/code/browser/tests/test_branchmergeproposal.py
@@ -2,7 +2,7 @@
 # NOTE: The first line above must stay first; do not move the copyright
 # notice to the top.  See http://www.python.org/dev/peps/pep-0263/.
 #
-# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Unit tests for BranchMergeProposals."""
@@ -75,7 +75,10 @@ from lp.code.enums import (
     BranchMergeProposalStatus,
     CodeReviewVote,
     )
-from lp.code.errors import InvalidBranchMergeProposal
+from lp.code.errors import (
+    GitRepositoryScanFault,
+    InvalidBranchMergeProposal,
+    )
 from lp.code.interfaces.branchjob import IBranchScanJobSource
 from lp.code.interfaces.branchmergeproposal import (
     IBranchMergeProposal,
@@ -154,7 +157,7 @@ class GitHostingClientMixin:
 
     def setUp(self):
         super(GitHostingClientMixin, self).setUp()
-        self.useFixture(GitHostingFixture())
+        self.git_hosting_fixture = self.useFixture(GitHostingFixture())
 
 
 class TestBranchMergeProposalContextMenu(TestCaseWithFactory):
@@ -293,6 +296,23 @@ class TestBranchMergeProposalMergedViewGit(
     def getBranchRevisionValues(self, branch):
         return {'merged_revision_id': branch.commit_sha1}
 
+    def test_mp_page_with_git_hosting_issues(self):
+        mp = self.makeBranchMergeProposal()
+        self.git_hosting_fixture.getLog.result = None
+        self.git_hosting_fixture.getLog.failure = GitRepositoryScanFault(":-(")
+
+        canonical_url(mp)
+        browser = self.getUserBrowser(canonical_url(mp))
+        error_msg = (
+            "There was an error fetching revisions from git servers. "
+            "Please, try again in some minutes. If the problem persists, "
+            "contact Launchpad support.")
+        self.assertThat(
+            browser.contents, HTMLContains(Tag(
+                'error msg', "div",
+                text=error_msg,
+                attrs={'class': "pending-update"})))
+
 
 class TestBranchMergeProposalAddVoteView(TestCaseWithFactory):
     """Test the AddVote view."""
@@ -1956,7 +1976,7 @@ class TestBranchMergeProposalBrowserView(BrowserTestCase):
         # There is of course the permissions aspect involved here that you can
         # do a local merge using only read permissions on the source branch.
         team = self.factory.makeTeam()
-        source_owner = self.factory.makePerson()
+        self.factory.makePerson()
         other_user = self.factory.makePerson()
         fooix = self.factory.makeProduct(name="fooix")
         repository = self.factory.makeGitRepository(
diff --git a/lib/lp/code/browser/tests/test_gitref.py b/lib/lp/code/browser/tests/test_gitref.py
index d49b343..6c25673 100644
--- a/lib/lp/code/browser/tests/test_gitref.py
+++ b/lib/lp/code/browser/tests/test_gitref.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Unit tests for GitRefView."""
@@ -23,6 +23,7 @@ from testtools.matchers import (
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
+from lp.code.errors import GitRepositoryScanFault
 from lp.code.interfaces.gitjob import IGitRefScanJobSource
 from lp.code.interfaces.gitrepository import IGitRepositorySet
 from lp.code.tests.helpers import GitHostingFixture
@@ -40,6 +41,7 @@ from lp.testing import (
     TestCaseWithFactory,
     )
 from lp.testing.dbuser import dbuser
+from lp.testing.fakemethod import FakeMethod
 from lp.testing.layers import (
     DatabaseFunctionalLayer,
     LaunchpadFunctionalLayer,
@@ -149,6 +151,37 @@ class TestGitRefView(BrowserTestCase):
     def test_rendering_non_ascii(self):
         self._test_rendering("\N{SNOWMAN}")
 
+    def test_rendering_githost_failure(self):
+        [ref] = self.factory.makeGitRefs(paths=["refs/heads/branch"])
+        log = self.makeCommitLog()
+        self.hosting_fixture.getLog.result = list(reversed(log))
+        self.scanRef(ref, log[-1])
+
+        # Make code hosting fail.
+        self.hosting_fixture.getLog = FakeMethod(
+            failure=GitRepositoryScanFault(":-("))
+        with admin_logged_in():
+            url = canonical_url(ref)
+        # Visiting ref page should not crash, and we should be able to see
+        # the error message telling us that git hosting is having problems.
+        browser = self.getUserBrowser(url, ref.owner)
+
+        recent_commits_tag = soupmatchers.Tag(
+            "recent commits", "div", attrs={"id": "recent-commits"})
+        error_msg = (
+            "There was an error while fetching commit information from code "
+            "hosting service. Please, try again in some minutes. If the "
+            "problem persists, contact Launchpad support.")
+        self.assertThat(
+            browser.contents,
+            soupmatchers.HTMLContains(
+                soupmatchers.Within(
+                    recent_commits_tag,
+                    soupmatchers.Tag(
+                        'error msg', "div",
+                        text=error_msg,
+                        attrs={'class': "pending-update"}))))
+
     def test_clone_instructions(self):
         [ref] = self.factory.makeGitRefs(paths=["refs/heads/branch"])
         username = ref.owner.name
diff --git a/lib/lp/code/templates/branchmergeproposal-index.pt b/lib/lp/code/templates/branchmergeproposal-index.pt
index d8eb68f..551ab1c 100644
--- a/lib/lp/code/templates/branchmergeproposal-index.pt
+++ b/lib/lp/code/templates/branchmergeproposal-index.pt
@@ -132,6 +132,9 @@
 
     <div id="conversation"
          tal:content="structure view/conversation/@@+render"/>
+     <div tal:condition="view/conversation_message" class="pending-update">
+      <p tal:content="view/conversation_message" />
+    </div>
   </div>
 
   <tal:logged-in condition="view/user">
diff --git a/lib/lp/code/templates/git-macros.pt b/lib/lp/code/templates/git-macros.pt
index 0c93657..c080c1a 100644
--- a/lib/lp/code/templates/git-macros.pt
+++ b/lib/lp/code/templates/git-macros.pt
@@ -144,6 +144,9 @@
     }
   </style>
   <dl class="commit">
+    <div tal:condition="view/commit_infos_message" class="pending-update">
+      <p tal:content="view/commit_infos_message" />
+    </div>
     <tal:commit repeat="commit_info commit_infos">
       <metal:commit-text use-macro="ref/@@+macros/commit-text"/>
     </tal:commit>