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

[Ioana Lasc]

Add link to latest internal log of status reports

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.

[Ioana Lasc (ilasc)]

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.

[Colin Watson (cjwatson)]

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

[Ioana Lasc (ilasc)]

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.

[Colin Watson (cjwatson)]

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.

[Ioana Lasc (ilasc)]

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

[Colin Watson (cjwatson)]

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

review: Approve

[Ioana Lasc (ilasc)]

Thanks Colin! Tweaks added and landing this.

diff --git a/lib/lp/code/interfaces/revisionstatus.py b/lib/lp/code/interfaces/revisionstatus.py
index b36c832..708cc40 100644
--- a/lib/lp/code/interfaces/revisionstatus.py
+++ b/lib/lp/code/interfaces/revisionstatus.py
@@ -79,6 +79,8 @@ class IRevisionStatusReportView(Interface):
     date_finished = exported(Datetime(
         title=_("When the report has finished.")), readonly=False)
 
+    latest_log = Attribute("The most recent log for this report.")
+
     @operation_parameters(
         artifact_type=Choice(vocabulary=RevisionStatusArtifactType,
                              required=False))
@@ -278,5 +280,10 @@ class IRevisionStatusArtifact(Interface):
 
     repository = Attribute("The repository for this artifact.")
 
+    download_url = Attribute("The download url for this artifact.")
+
+    date_created = Datetime(
+        title=_("When the artifact was created."), readonly=True)
+
     def getFileByName(filename):
         """Returns an artifact by name."""
diff --git a/lib/lp/code/model/revisionstatus.py b/lib/lp/code/model/revisionstatus.py
index 1f74af3..78fc85e 100644
--- a/lib/lp/code/model/revisionstatus.py
+++ b/lib/lp/code/model/revisionstatus.py
@@ -11,6 +11,7 @@ import io
 import os
 
 import pytz
+from storm.expr import Desc
 from storm.locals import (
     And,
     DateTime,
@@ -157,6 +158,16 @@ class RevisionStatusReport(StormBase):
         artifacts = IStore(RevisionStatusArtifact).find(*clauses)
         return [artifact.download_url for artifact in artifacts]
 
+    @property
+    def latest_log(self):
+        log = IStore(RevisionStatusArtifact).find(
+            RevisionStatusArtifact,
+            RevisionStatusArtifact.report == self,
+            RevisionStatusArtifact.artifact_type ==
+            RevisionStatusArtifactType.LOG).order_by(
+            Desc(RevisionStatusArtifact.date_created)).first()
+        return log
+
 
 @implementer(IRevisionStatusReportSet)
 class RevisionStatusReportSet:
@@ -223,11 +234,15 @@ class RevisionStatusArtifact(StormBase):
     artifact_type = DBEnum(name='type', allow_none=False,
                            enum=RevisionStatusArtifactType)
 
+    date_created = DateTime(
+        name='date_created', tzinfo=pytz.UTC, allow_none=True)
+
     def __init__(self, library_file, report, artifact_type):
         super().__init__()
         self.library_file = library_file
         self.report = report
         self.artifact_type = artifact_type
+        self.date_created = UTC_NOW
 
     @property
     def download_url(self):
diff --git a/lib/lp/code/model/tests/test_revisionstatus.py b/lib/lp/code/model/tests/test_revisionstatus.py
index 33f9c3c..deb9f6d 100644
--- a/lib/lp/code/model/tests/test_revisionstatus.py
+++ b/lib/lp/code/model/tests/test_revisionstatus.py
@@ -150,6 +150,14 @@ class TestRevisionStatusReport(TestCaseWithFactory):
             IRevisionStatusReportSet).getByCIBuildAndTitle(build, "test")
         self.assertEqual("test", report.title)
 
+    def test_latest_log(self):
+        report = self.factory.makeRevisionStatusReport()
+        self.makeRevisionStatusArtifact(report=report)
+        self.makeRevisionStatusArtifact(report=report)
+        artifact3 = self.makeRevisionStatusArtifact(report=report)
+        with person_logged_in(report.git_repository.owner):
+            self.assertEqual(artifact3, report.latest_log)
+
 
 class TestRevisionStatusReportWebservice(TestCaseWithFactory):
     layer = LaunchpadFunctionalLayer
diff --git a/lib/lp/code/templates/git-macros.pt b/lib/lp/code/templates/git-macros.pt
index ea6dd1d..262586b 100644
--- a/lib/lp/code/templates/git-macros.pt
+++ b/lib/lp/code/templates/git-macros.pt
@@ -235,7 +235,9 @@
           <tbody>
             <tr tal:repeat="report batch">
               <td class="icon" tal:content="structure report/image:icon" />
-              <td><a tal:attributes="href report/url|nothing" tal:content="report/title" /></td>
+              <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>
               <td tal:content="report/result_summary" />
             </tr>
           </tbody>