Merge ~ilasc/launchpad:add-link-to-status-report-log into launchpad:master

Proposed by Ioana Lasc
Status: Merged
Approved by: Ioana Lasc
Approved revision: fbc03d6e33a5373713e73f79a884b27dbb187ee8
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: ~ilasc/launchpad:add-link-to-status-report-log
Merge into: launchpad:master
Diff against target: 103 lines (+33/-1)
4 files modified
lib/lp/code/interfaces/revisionstatus.py (+7/-0)
lib/lp/code/model/revisionstatus.py (+15/-0)
lib/lp/code/model/tests/test_revisionstatus.py (+8/-0)
lib/lp/code/templates/git-macros.pt (+3/-1)
Reviewer Review Type Date Requested Status
Colin Watson Approve
Review via email: mp+417156@code.launchpad.net

Commit message

Add link to latest internal log of status reports

Description of the change

This adds the link to the latest internally stored log on status reports.

We use the URL to the log as a fallback if report.url isn't set instead of defining a new column in the table and we only link to the internally-stored log when we don't have an external URL.

To post a comment you must log in.
Revision history for this message
Ioana Lasc (ilasc) wrote (last edit ):

This work needs a DB change: adding date_created to RevisionStatusArtifact and after that lands, the query here in `latestLog` will need to change. DB patch for the change: https://code.launchpad.net/~ilasc/launchpad/+git/launchpad/+merge/417161

It also requires unit tests after that.

Revision history for this message
Colin Watson (cjwatson) wrote :

It seems to me that it's confusing to have a method called "latestLog" that either returns a URL that doesn't come from a log artifact at all or the download URL of a log artifact, and in no case actually returns a log artifact.

How about instead writing the link tag in the template as follows:

    <a tal:attributes="href report/url|report/latest_log/download_url|nothing" tal:content="report/title" />

Then the report can have a `latest_log` property instead of a `latestLog` method (which was anomalously named - method names normally contain a verb), and can just return the artifact rather than picking out its download URL in Python. I think that's somewhat neater, and it feels better to me to put this sort of display logic in the template.

review: Needs Fixing
Revision history for this message
Ioana Lasc (ilasc) wrote :

Thanks Colin, seems this "href report/url | report/latest_log/download_url | nothing" doesn't have quite the expected behavior - it evaluates to no link when 'report/url' is null and 'download_url' is set.. will try more things around it tomorrow, couldn't spot the problem today.

Revision history for this message
Colin Watson (cjwatson) wrote :

Sorry, it looks like I misled you slightly here. Looking at the TALES documentation (https://pagetemplates.readthedocs.io/en/latest/tales.html), it says:

    If a traversal step fails, and no alternate expression has been specified, an error results. Otherwise, the alternate expression is evaluated.

What it means by "fails" here is roughly "raises an exception" - just returning None doesn't count as failing. `report/latest_log/download_url` will indeed raise an exception if `report.latest_log` is None, but `report/url` will just return None if the report doesn't have a URL.

I think the simplest fix, though it's a bit ugly, would be to change the template to this:

              <td tal:define="url python: report.url if report.url is not None else path('report/latest_log/download_url | nothing')">
                <a tal:attributes="href url" tal:content="report/title" />
              </td>

(Alternatively, you could add another property to `RevisionStatusReport` to encapsulate this. I was going to suggest that as an alternative but then realized I couldn't think of a suitable name for the property.)

Also, you'll need to rebase/merge this onto current master, as I touched some nearby template code recently.

Revision history for this message
Ioana Lasc (ilasc) wrote :

Thanks Colin, indeed that did the trick, updated and tested, it needs an approval vote before it can land.

Revision history for this message
Colin Watson (cjwatson) wrote :

Just a couple of minor tweaks to the test, but after that you can go ahead and land this. Thanks!

review: Approve
Revision history for this message
Ioana Lasc (ilasc) wrote :

Thanks Colin! Tweaks added and landing this.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/lib/lp/code/interfaces/revisionstatus.py b/lib/lp/code/interfaces/revisionstatus.py
2index b36c832..708cc40 100644
3--- a/lib/lp/code/interfaces/revisionstatus.py
4+++ b/lib/lp/code/interfaces/revisionstatus.py
5@@ -79,6 +79,8 @@ class IRevisionStatusReportView(Interface):
6 date_finished = exported(Datetime(
7 title=_("When the report has finished.")), readonly=False)
8
9+ latest_log = Attribute("The most recent log for this report.")
10+
11 @operation_parameters(
12 artifact_type=Choice(vocabulary=RevisionStatusArtifactType,
13 required=False))
14@@ -278,5 +280,10 @@ class IRevisionStatusArtifact(Interface):
15
16 repository = Attribute("The repository for this artifact.")
17
18+ download_url = Attribute("The download url for this artifact.")
19+
20+ date_created = Datetime(
21+ title=_("When the artifact was created."), readonly=True)
22+
23 def getFileByName(filename):
24 """Returns an artifact by name."""
25diff --git a/lib/lp/code/model/revisionstatus.py b/lib/lp/code/model/revisionstatus.py
26index 1f74af3..78fc85e 100644
27--- a/lib/lp/code/model/revisionstatus.py
28+++ b/lib/lp/code/model/revisionstatus.py
29@@ -11,6 +11,7 @@ import io
30 import os
31
32 import pytz
33+from storm.expr import Desc
34 from storm.locals import (
35 And,
36 DateTime,
37@@ -157,6 +158,16 @@ class RevisionStatusReport(StormBase):
38 artifacts = IStore(RevisionStatusArtifact).find(*clauses)
39 return [artifact.download_url for artifact in artifacts]
40
41+ @property
42+ def latest_log(self):
43+ log = IStore(RevisionStatusArtifact).find(
44+ RevisionStatusArtifact,
45+ RevisionStatusArtifact.report == self,
46+ RevisionStatusArtifact.artifact_type ==
47+ RevisionStatusArtifactType.LOG).order_by(
48+ Desc(RevisionStatusArtifact.date_created)).first()
49+ return log
50+
51
52 @implementer(IRevisionStatusReportSet)
53 class RevisionStatusReportSet:
54@@ -223,11 +234,15 @@ class RevisionStatusArtifact(StormBase):
55 artifact_type = DBEnum(name='type', allow_none=False,
56 enum=RevisionStatusArtifactType)
57
58+ date_created = DateTime(
59+ name='date_created', tzinfo=pytz.UTC, allow_none=True)
60+
61 def __init__(self, library_file, report, artifact_type):
62 super().__init__()
63 self.library_file = library_file
64 self.report = report
65 self.artifact_type = artifact_type
66+ self.date_created = UTC_NOW
67
68 @property
69 def download_url(self):
70diff --git a/lib/lp/code/model/tests/test_revisionstatus.py b/lib/lp/code/model/tests/test_revisionstatus.py
71index 33f9c3c..deb9f6d 100644
72--- a/lib/lp/code/model/tests/test_revisionstatus.py
73+++ b/lib/lp/code/model/tests/test_revisionstatus.py
74@@ -150,6 +150,14 @@ class TestRevisionStatusReport(TestCaseWithFactory):
75 IRevisionStatusReportSet).getByCIBuildAndTitle(build, "test")
76 self.assertEqual("test", report.title)
77
78+ def test_latest_log(self):
79+ report = self.factory.makeRevisionStatusReport()
80+ self.makeRevisionStatusArtifact(report=report)
81+ self.makeRevisionStatusArtifact(report=report)
82+ artifact3 = self.makeRevisionStatusArtifact(report=report)
83+ with person_logged_in(report.git_repository.owner):
84+ self.assertEqual(artifact3, report.latest_log)
85+
86
87 class TestRevisionStatusReportWebservice(TestCaseWithFactory):
88 layer = LaunchpadFunctionalLayer
89diff --git a/lib/lp/code/templates/git-macros.pt b/lib/lp/code/templates/git-macros.pt
90index ea6dd1d..262586b 100644
91--- a/lib/lp/code/templates/git-macros.pt
92+++ b/lib/lp/code/templates/git-macros.pt
93@@ -235,7 +235,9 @@
94 <tbody>
95 <tr tal:repeat="report batch">
96 <td class="icon" tal:content="structure report/image:icon" />
97- <td><a tal:attributes="href report/url|nothing" tal:content="report/title" /></td>
98+ <td tal:define="url python: report.url if report.url is not None else path('report/latest_log/download_url | nothing')">
99+ <a tal:attributes="href url" tal:content="report/title" />
100+ </td>
101 <td tal:content="report/result_summary" />
102 </tr>
103 </tbody>

Subscribers

People subscribed via source and target branches

to status/vote changes: