launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #28343
[Merge] ~cjwatson/launchpad:revision-status-report-overall-icon into launchpad:master
Colin Watson has proposed merging ~cjwatson/launchpad:revision-status-report-overall-icon into launchpad:master.
Commit message:
Fix overall summary of revision status reports for a commit
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/419494
The set of revision status reports for a commit aren't well summarized by a boolean succeeded/failed icon that only goes green when all jobs have succeeded: some or all of the jobs might be skipped, or they might still be in progress. Replace this with a more nuanced set of rules as follows:
* If all jobs were skipped, show the `yes-gray` icon.
* Otherwise, if all jobs either succeeded or were skipped, show the `yes` icon.
* Otherwise, if any jobs failed or were cancelled, show the `no` icon.
* Otherwise, show the `processing` icon.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:revision-status-report-overall-icon into launchpad:master.
diff --git a/lib/lp/code/browser/branchmergeproposal.py b/lib/lp/code/browser/branchmergeproposal.py
index 064e37b..4e89019 100644
--- a/lib/lp/code/browser/branchmergeproposal.py
+++ b/lib/lp/code/browser/branchmergeproposal.py
@@ -75,6 +75,7 @@ from lp.app.browser.tales import DateTimeFormatterAPI
from lp.code.adapters.branch import BranchMergeProposalNoPreviewDiffDelta
from lp.code.browser.codereviewcomment import CodeReviewDisplayComment
from lp.code.browser.decorations import DecoratedBranch
+from lp.code.browser.revisionstatus import HasRevisionStatusReportsMixin
from lp.code.browser.widgets.gitref import GitRefWidget
from lp.code.enums import (
BranchMergeProposalStatus,
@@ -126,7 +127,6 @@ from lp.services.webapp import (
stepthrough,
)
from lp.services.webapp.authorization import check_permission
-from lp.services.webapp.batching import BatchNavigator
from lp.services.webapp.breadcrumb import Breadcrumb
from lp.services.webapp.escaping import structured
from lp.services.webapp.interfaces import ILaunchBag
@@ -634,7 +634,8 @@ class CodeReviewNewRevisions:
class BranchMergeProposalView(LaunchpadFormView, UnmergedRevisionsMixin,
BranchMergeProposalRevisionIdMixin,
BranchMergeProposalStatusMixin,
- DiffRenderingMixin):
+ DiffRenderingMixin,
+ HasRevisionStatusReportsMixin):
"""A basic view used for the index page."""
schema = ClaimButton
@@ -665,11 +666,6 @@ class BranchMergeProposalView(LaunchpadFormView, UnmergedRevisionsMixin,
self.request.response.addErrorNotification(str(e))
self.next_url = canonical_url(self.context)
- def getStatusReports(self, commit_sha1):
- reports = self.context.getStatusReports(
- commit_sha1)
- return BatchNavigator(reports, self.request)
-
@property
def comment_location(self):
"""Location of page for commenting on this proposal."""
diff --git a/lib/lp/code/browser/gitref.py b/lib/lp/code/browser/gitref.py
index 4cb3871..fedff3e 100644
--- a/lib/lp/code/browser/gitref.py
+++ b/lib/lp/code/browser/gitref.py
@@ -40,6 +40,7 @@ from lp.charms.browser.hascharmrecipes import (
from lp.code.browser.branchmergeproposal import (
latest_proposals_for_each_branch,
)
+from lp.code.browser.revisionstatus import HasRevisionStatusReportsMixin
from lp.code.browser.sourcepackagerecipelisting import HasRecipesMenuMixin
from lp.code.browser.widgets.gitref import GitRefWidget
from lp.code.enums import GitRepositoryType
@@ -66,7 +67,6 @@ from lp.services.webapp import (
Link,
)
from lp.services.webapp.authorization import check_permission
-from lp.services.webapp.batching import BatchNavigator
from lp.services.webapp.escaping import structured
from lp.snappy.browser.hassnaps import (
HasSnapsMenuMixin,
@@ -118,7 +118,8 @@ class GitRefContextMenu(
return Link("+new-recipe", text, enabled=enabled, icon="add")
-class GitRefView(LaunchpadView, HasSnapsViewMixin, HasCharmRecipesViewMixin):
+class GitRefView(LaunchpadView, HasSnapsViewMixin, HasCharmRecipesViewMixin,
+ HasRevisionStatusReportsMixin):
# This is set at self.commit_infos, and should be accessed by the view
# as self.commit_info_message.
@@ -276,10 +277,6 @@ class GitRefView(LaunchpadView, HasSnapsViewMixin, HasCharmRecipesViewMixin):
'<a href="+recipes">%s recipes</a> using this branch.',
count).escapedtext
- def getStatusReports(self, commit_sha1):
- reports = self.context.getStatusReports(commit_sha1)
- return BatchNavigator(reports, self.request)
-
class GitRefRegisterMergeProposalSchema(Interface):
"""The schema to define the form for registering a new merge proposal."""
diff --git a/lib/lp/code/browser/revisionstatus.py b/lib/lp/code/browser/revisionstatus.py
index e93276c..39574eb 100644
--- a/lib/lp/code/browser/revisionstatus.py
+++ b/lib/lp/code/browser/revisionstatus.py
@@ -1,12 +1,51 @@
# Copyright 2022 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
+from lp.code.enums import RevisionStatusResult
from lp.code.interfaces.revisionstatus import IRevisionStatusArtifact
from lp.services.librarian.browser import FileNavigationMixin
from lp.services.webapp import Navigation
+from lp.services.webapp.batching import BatchNavigator
class RevisionStatusArtifactNavigation(Navigation, FileNavigationMixin):
"""Traversal to +files/${filename}."""
usedfor = IRevisionStatusArtifact
+
+
+class HasRevisionStatusReportsMixin:
+
+ def getStatusReports(self, commit_sha1):
+ reports = self.context.getStatusReports(commit_sha1)
+ return BatchNavigator(reports, self.request)
+
+ def getOverallIcon(self, repository, commit_sha1):
+ """Show an appropriate icon at the top of the report."""
+ icon_template = (
+ '<img width="14" height="14" alt="%(title)s" '
+ 'title="%(title)s" src="%(src)s" />')
+ reports = repository.getStatusReports(commit_sha1)
+ if all(
+ report.result == RevisionStatusResult.SKIPPED
+ for report in reports):
+ title = "Skipped"
+ source = "/@@/yes-gray"
+ elif all(
+ report.result in (
+ RevisionStatusResult.SUCCEEDED,
+ RevisionStatusResult.SKIPPED)
+ for report in reports):
+ title = "Succeeded"
+ source = "/@@/yes"
+ elif any(
+ report.result in (
+ RevisionStatusResult.FAILED,
+ RevisionStatusResult.CANCELLED)
+ for report in reports):
+ title = "Failed"
+ source = "/@@/no"
+ else:
+ title = "In progress"
+ source = "/@@/processing"
+ return icon_template % {"title": title, "src": source}
diff --git a/lib/lp/code/browser/tests/test_gitref.py b/lib/lp/code/browser/tests/test_gitref.py
index 18a94f1..85864ea 100644
--- a/lib/lp/code/browser/tests/test_gitref.py
+++ b/lib/lp/code/browser/tests/test_gitref.py
@@ -15,6 +15,7 @@ import soupmatchers
from storm.store import Store
from testtools.matchers import (
Equals,
+ MatchesListwise,
Not,
)
from zope.component import getUtility
@@ -267,6 +268,184 @@ class TestGitRefView(BrowserTestCase):
# on the page: reports_section[0] and reports_section[1]
self.assertEqual(2, len(reports_section))
+ def test_revisionStatusReports_all_skipped(self):
+ self.useFixture(FakeLogger())
+ [ref] = self.factory.makeGitRefs()
+ log = self.makeCommitLog()
+ for _ in range(2):
+ self.factory.makeRevisionStatusReport(
+ user=ref.repository.owner, git_repository=ref.repository,
+ commit_sha1=log[0]["sha1"],
+ result=RevisionStatusResult.SKIPPED)
+ self.hosting_fixture.getLog.result = list(log)
+ self.scanRef(ref, log[-1], blobs={".launchpad.yaml": ""})
+ view = create_initialized_view(
+ ref, "+index", principal=ref.repository.owner)
+ with person_logged_in(ref.repository.owner):
+ contents = view.render()
+ reports_section = find_tags_by_class(contents, "status-reports-table")
+ with person_logged_in(ref.repository.owner):
+ self.assertThat(
+ reports_section[0].div,
+ soupmatchers.Tag(
+ "overall icon", "img",
+ attrs={
+ "width": "14", "height": "14",
+ "src": "/@@/yes-gray", "title": "Skipped",
+ }))
+ self.assertThat(
+ reports_section[0].table.find_all("tr"),
+ MatchesListwise([
+ soupmatchers.Tag(
+ "icon", "img",
+ attrs={"src": "/@@/yes-gray", "title": "Skipped"})
+ for _ in range(2)]))
+
+ def test_revisionStatusReports_all_succeeded_or_skipped(self):
+ self.useFixture(FakeLogger())
+ [ref] = self.factory.makeGitRefs()
+ log = self.makeCommitLog()
+ for result in (
+ RevisionStatusResult.SUCCEEDED, RevisionStatusResult.SUCCEEDED,
+ RevisionStatusResult.SKIPPED):
+ self.factory.makeRevisionStatusReport(
+ user=ref.repository.owner, git_repository=ref.repository,
+ commit_sha1=log[0]["sha1"], result=result)
+ self.hosting_fixture.getLog.result = list(log)
+ self.scanRef(ref, log[-1], blobs={".launchpad.yaml": ""})
+ view = create_initialized_view(
+ ref, "+index", principal=ref.repository.owner)
+ with person_logged_in(ref.repository.owner):
+ contents = view.render()
+ reports_section = find_tags_by_class(contents, "status-reports-table")
+ with person_logged_in(ref.repository.owner):
+ self.assertThat(
+ reports_section[0].div,
+ soupmatchers.Tag(
+ "overall icon", "img",
+ attrs={
+ "width": "14", "height": "14",
+ "src": "/@@/yes", "title": "Succeeded",
+ }))
+ self.assertThat(
+ reports_section[0].table.find_all("tr"),
+ MatchesListwise([
+ soupmatchers.Tag(
+ "icon", "img", attrs={"src": src, "title": title})
+ for src, title in (
+ ("/@@/yes", "Succeeded"),
+ ("/@@/yes", "Succeeded"),
+ ("/@@/yes-gray", "Skipped"),
+ )]))
+
+ def test_revisionStatusReports_failed(self):
+ self.useFixture(FakeLogger())
+ [ref] = self.factory.makeGitRefs()
+ log = self.makeCommitLog()
+ for result in (
+ RevisionStatusResult.SUCCEEDED, RevisionStatusResult.FAILED):
+ self.factory.makeRevisionStatusReport(
+ user=ref.repository.owner, git_repository=ref.repository,
+ commit_sha1=log[0]["sha1"], result=result)
+ self.hosting_fixture.getLog.result = list(log)
+ self.scanRef(ref, log[-1], blobs={".launchpad.yaml": ""})
+ view = create_initialized_view(
+ ref, "+index", principal=ref.repository.owner)
+ with person_logged_in(ref.repository.owner):
+ contents = view.render()
+ reports_section = find_tags_by_class(contents, "status-reports-table")
+ with person_logged_in(ref.repository.owner):
+ self.assertThat(
+ reports_section[0].div,
+ soupmatchers.Tag(
+ "overall icon", "img",
+ attrs={
+ "width": "14", "height": "14",
+ "src": "/@@/no", "title": "Failed",
+ }))
+ self.assertThat(
+ reports_section[0].table.find_all("tr"),
+ MatchesListwise([
+ soupmatchers.Tag(
+ "icon", "img", attrs={"src": src, "title": title})
+ for src, title in (
+ ("/@@/yes", "Succeeded"),
+ ("/@@/no", "Failed"),
+ )]))
+
+ def test_revisionStatusReports_cancelled(self):
+ self.useFixture(FakeLogger())
+ [ref] = self.factory.makeGitRefs()
+ log = self.makeCommitLog()
+ for result in (
+ RevisionStatusResult.SUCCEEDED,
+ RevisionStatusResult.CANCELLED):
+ self.factory.makeRevisionStatusReport(
+ user=ref.repository.owner, git_repository=ref.repository,
+ commit_sha1=log[0]["sha1"], result=result)
+ self.hosting_fixture.getLog.result = list(log)
+ self.scanRef(ref, log[-1], blobs={".launchpad.yaml": ""})
+ view = create_initialized_view(
+ ref, "+index", principal=ref.repository.owner)
+ with person_logged_in(ref.repository.owner):
+ contents = view.render()
+ reports_section = find_tags_by_class(contents, "status-reports-table")
+ with person_logged_in(ref.repository.owner):
+ self.assertThat(
+ reports_section[0].div,
+ soupmatchers.Tag(
+ "overall icon", "img",
+ attrs={
+ "width": "14", "height": "14",
+ "src": "/@@/no", "title": "Failed",
+ }))
+ self.assertThat(
+ reports_section[0].table.find_all("tr"),
+ MatchesListwise([
+ soupmatchers.Tag(
+ "icon", "img",
+ attrs={"src": src, "title": title})
+ for src, title in (
+ ("/@@/yes", "Succeeded"),
+ ("/@@/build-failed", "Cancelled"),
+ )]))
+
+ def test_revisionStatusReports_all_waiting_or_running(self):
+ self.useFixture(FakeLogger())
+ [ref] = self.factory.makeGitRefs()
+ log = self.makeCommitLog()
+ for result in (
+ RevisionStatusResult.WAITING, RevisionStatusResult.RUNNING):
+ self.factory.makeRevisionStatusReport(
+ user=ref.repository.owner, git_repository=ref.repository,
+ commit_sha1=log[0]["sha1"], result=result)
+ self.hosting_fixture.getLog.result = list(log)
+ self.scanRef(ref, log[-1], blobs={".launchpad.yaml": ""})
+ view = create_initialized_view(
+ ref, "+index", principal=ref.repository.owner)
+ with person_logged_in(ref.repository.owner):
+ contents = view.render()
+ reports_section = find_tags_by_class(contents, "status-reports-table")
+ with person_logged_in(ref.repository.owner):
+ self.assertThat(
+ reports_section[0].div,
+ soupmatchers.Tag(
+ "overall icon", "img",
+ attrs={
+ "width": "14", "height": "14",
+ "src": "/@@/processing", "title": "In progress",
+ }))
+ self.assertThat(
+ reports_section[0].table.find_all("tr"),
+ MatchesListwise([
+ soupmatchers.Tag(
+ "icon", "img",
+ attrs={"src": src, "title": title})
+ for src, title in (
+ ("/@@/build-needed", "Waiting"),
+ ("/@@/processing", "Running"),
+ )]))
+
def test_clone_instructions(self):
[ref] = self.factory.makeGitRefs(paths=["refs/heads/branch"])
username = ref.owner.name
diff --git a/lib/lp/code/interfaces/gitref.py b/lib/lp/code/interfaces/gitref.py
index eb8066a..b8bf0ce 100644
--- a/lib/lp/code/interfaces/gitref.py
+++ b/lib/lp/code/interfaces/gitref.py
@@ -152,9 +152,6 @@ class IGitRefView(IHasMergeProposals, IHasRecipes, IPrivacy, IInformationType):
def getStatusReports(commit_sha1):
"""Get status reports for this repository at the given commit"""
- def getCommitStatus(commit_sha1):
- """Help to show red or green icon at the top of the commit."""
-
information_type = Attribute(
"The type of information contained in the repository containing this "
"reference.")
diff --git a/lib/lp/code/interfaces/gitrepository.py b/lib/lp/code/interfaces/gitrepository.py
index a541026..4edf4c9 100644
--- a/lib/lp/code/interfaces/gitrepository.py
+++ b/lib/lp/code/interfaces/gitrepository.py
@@ -741,6 +741,18 @@ class IGitRepositoryView(IHasRecipes):
macaroon.
"""
+ @operation_parameters(
+ commit_sha1=copy_field(IRevisionStatusReport["commit_sha1"]))
+ @scoped(AccessTokenScope.REPOSITORY_BUILD_STATUS.title)
+ @operation_returns_collection_of(Interface)
+ @export_read_operation()
+ @operation_for_version("devel")
+ def getStatusReports(commit_sha1):
+ """Retrieves the list of reports that exist for a commit.
+
+ :param commit_sha1: The commit sha1 for the report.
+ """
+
class IGitRepositoryModerateAttributes(Interface):
"""IGitRepository attributes that can be edited by more than one community.
@@ -1039,18 +1051,6 @@ class IGitRepositoryEdit(IWebhookTarget, IAccessTokenTarget):
:param result: The result of the new report.
"""
- @operation_parameters(
- commit_sha1=copy_field(IRevisionStatusReport["commit_sha1"]))
- @scoped(AccessTokenScope.REPOSITORY_BUILD_STATUS.title)
- @operation_returns_collection_of(Interface)
- @export_read_operation()
- @operation_for_version("devel")
- def getStatusReports(commit_sha1):
- """Retrieves the list of reports that exist for a commit.
-
- :param commit_sha1: The commit sha1 for the report.
- """
-
# XXX cjwatson 2015-01-19 bug=760849: "beta" is a lie to get WADL
# generation working. Individual attributes must set their version to
diff --git a/lib/lp/code/model/gitref.py b/lib/lp/code/model/gitref.py
index 08bb3bc..a3ef756 100644
--- a/lib/lp/code/model/gitref.py
+++ b/lib/lp/code/model/gitref.py
@@ -43,7 +43,6 @@ from lp.code.enums import (
BranchMergeProposalStatus,
GitObjectType,
GitRepositoryType,
- RevisionStatusResult,
)
from lp.code.errors import (
BranchMergeProposalExists,
@@ -183,13 +182,6 @@ class GitRefMixin:
def getStatusReports(self, commit_sha1):
return self.repository.getStatusReports(commit_sha1)
- def getCommitStatus(self, commit_sha1):
- """Help to show red or green icon at the top of the commit."""
- reports = self.repository.getStatusReports(commit_sha1)
- return all(
- report.result == RevisionStatusResult.SUCCEEDED
- for report in reports)
-
@property
def information_type(self):
"""See `IGitRef`."""
diff --git a/lib/lp/code/templates/git-macros.pt b/lib/lp/code/templates/git-macros.pt
index 997f96a..ea6dd1d 100644
--- a/lib/lp/code/templates/git-macros.pt
+++ b/lib/lp/code/templates/git-macros.pt
@@ -224,12 +224,12 @@
<dd class="subordinate collapsible status-reports-table"
tal:define="
commit_sha1 commit_info/sha1;
- status_result python:ref.getCommitStatus(commit_sha1);
+ status_result python:view.getOverallIcon(ref.repository, commit_sha1);
batchnav python:view.getStatusReports(commit_sha1);
batch python:batchnav.currentBatch()"
tal:condition="batch">
- <div class="sprite treeCollapsed">
- <span tal:replace="structure status_result/image:boolean" />
+ <div class="treeCollapsed">
+ <span class="icon" tal:replace="structure status_result" />
</div>
<table class="listing" tal:condition="batch">
<tbody>
