launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> 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
> @@ -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, "

This would be slightly more idiomatic without a comma after "Please", and perhaps with "a few" rather than "some".

> +    "contact Launchpad support.")

Can we arrange for this to have a link to `/launchpad/+addquestion`?

> +
> +
>  def latest_proposals_for_each_branch(proposals):
>      """Returns the most recent merge proposals for any particular branch.
>  
> @@ -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

Can this have a comment to indicate that it's being evaluated for the sake of perhaps updating `self._unlanded_revisions_message`?

> +        return self._unlanded_revisions_message
>  
>      @property
>      def pending_updates(self):
> @@ -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

Can this have a comment to indicate that it's being evaluated for the sake of perhaps updating `self._conversation_message`?

> +        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
> @@ -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

Can this have a comment to indicate that it's being evaluated for the sake of perhaps updating `self._commit_info_message`?

> +        return self._commit_info_message
>  
>      @property
>      def recipes_link(self):
> 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">

The opening `<div>` here is slightly misaligned (one too many spaces of indentation).

> +      <p tal:content="view/conversation_message" />
> +    </div>
>    </div>
>  
>    <tal:logged-in condition="view/user">


-- 
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/401248
Your team Launchpad code reviewers is subscribed to branch ~pappacena/launchpad:gitref-and-mp-pages-without-oops-on-failure.