launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing



Diff comments:

> === modified file 'lib/lp/code/browser/branch.py'
> --- lib/lp/code/browser/branch.py	2018-07-16 00:49:00 +0000
> +++ lib/lp/code/browser/branch.py	2019-01-23 16:23:25 +0000
> @@ -635,6 +646,23 @@
>          return self.context.getSpecificationLinks(self.user)
>  
>  
> +class BranchRescanView(LaunchpadEditFormView):
> +
> +    schema = Interface
> +
> +    field_names = []
> +
> +    @action('Rescan', name='rescan')
> +    def rescan(self, action, data):
> +        result = self.context.unscan(rescan=True)
> +        if result:
> +            message = "Branch scan scheduled"
> +        else:
> +            message = "Branch scan schedule failed"

This is technically grammatical, but it reads oddly to me.  How about "Failed to schedule a branch scan"?

Alternatively, since it's impossible for that method to return a false value (it always either returns a non-empty tuple or raises an exception), you could just remove that unreachable code path.

> +        self.request.response.addNotification(message)
> +        self.next_url = canonical_url(self.context)
> +
> +
>  class BranchEditFormView(LaunchpadEditFormView):
>      """Base class for forms that edit a branch."""
>  
> 
> === modified file 'lib/lp/code/interfaces/branch.py'
> --- lib/lp/code/interfaces/branch.py	2018-05-17 14:10:29 +0000
> +++ lib/lp/code/interfaces/branch.py	2019-01-23 16:23:25 +0000
> @@ -1034,6 +1034,9 @@
>              detail page.
>          """
>  
> +    def getLatestScanJob():
> +        """Get the lastest IBranchScanJob for this branch"""

s/lastest/latest/

> +
>      def checkUpgrade():
>          """Check whether an upgrade should be performed, and raise if not.
>  
> 
> === modified file 'lib/lp/code/model/branch.py'
> --- lib/lp/code/model/branch.py	2018-12-10 13:54:34 +0000
> +++ lib/lp/code/model/branch.py	2019-01-23 16:23:25 +0000
> @@ -167,7 +167,7 @@
>  from lp.services.database.datetimecol import UtcDateTimeCol
>  from lp.services.database.decoratedresultset import DecoratedResultSet
>  from lp.services.database.enumcol import EnumCol
> -from lp.services.database.interfaces import IMasterStore
> +from lp.services.database.interfaces import IMasterStore, IStore

LP style is:

  from lp.services.database.interfaces import (
      IMasterStore,
      IStore,
      )

>  from lp.services.database.sqlbase import (
>      SQLBase,
>      sqlvalues,
> @@ -1295,6 +1295,15 @@
>              job.celeryRunOnCommit()
>          return (self.last_mirrored_id, old_scanned_id)
>  
> +    def getLatestScanJob(self):
> +        from lp.code.model.branchjob import BranchJob, BranchScanJob
> +        latest_job = IStore(BranchJob).find(
> +            BranchJob,
> +            branch=self,
> +            job_type=BranchScanJob.class_job_type).order_by(
> +                BranchJob.id).first()

I'm mildly uncomfortable with ordering by BranchJob.id, I think because there isn't a strong guarantee that if multiple jobs are somehow created for a single branch then they'll be run in order.  Also this seems backwards in any case, since it sorts by ascending job ID and takes the first one, so it's going to return the oldest job.

Maybe sort by descending Job.date_finished (noting that PostgreSQL sorts nulls first by default for a descending sort)?

> +        return latest_job
> +
>      def requestMirror(self):
>          """See `IBranch`."""
>          if self.branch_type in (BranchType.REMOTE, BranchType.HOSTED):
> 
> === modified file 'lib/lp/code/model/tests/test_branch.py'
> --- lib/lp/code/model/tests/test_branch.py	2018-07-09 09:27:06 +0000
> +++ lib/lp/code/model/tests/test_branch.py	2019-01-23 16:23:25 +0000
> @@ -3502,6 +3502,19 @@
>                  getUtility(ILaunchpadCelebrities).commercial_admin):
>              branch.unscan()
>  
> +    def test_getLatestScanJob(self):
> +        branch = self.factory.makeAnyBranch()
> +        with person_logged_in(branch.owner):
> +            branch.unscan(rescan=True)
> +        found = Store.of(branch).find(BranchJob, branch=branch)[0]
> +        result = branch.getLatestScanJob()
> +        self.assertEqual(found, result)

In light of the above (what I think is a) mistake, this could do with creating more than one scan job as part of the test setup so that the test is less trivial.

> +
> +    def test_getLatestScanJob_no_scans(self):
> +        branch = self.factory.makeAnyBranch()
> +        result = branch.getLatestScanJob()
> +        self.assertIsNone(result)
> +
>  
>  class TestWebservice(TestCaseWithFactory):
>      """Tests for the webservice."""
> 
> === modified file 'lib/lp/code/templates/branch-index.pt'
> --- lib/lp/code/templates/branch-index.pt	2017-11-06 09:32:45 +0000
> +++ lib/lp/code/templates/branch-index.pt	2019-01-23 16:23:25 +0000
> @@ -130,18 +130,38 @@
>  
>    </div>
>  
> -  <div class="yui-g" tal:condition="view/pending_updates">
> +  <tal:rescan condition="not:view/show_rescan_link">
> +    <div class="yui-g" tal:condition="view/pending_updates">

I'd avoid the extra nesting and do <div class="yui-g" tal:condition="python: not view.show_rescan_link and view.pending_updates">.

> +      <div class="portlet">
> +        <div id="branch-pending-updates" class="pending-update">
> +          <h3>Updating branch...</h3>
> +          <p>
> +            Launchpad is processing new changes to this branch which will be
> +            available in a few minutes. Reload to see the changes.
> +          </p>
> +        </div>
> +      </div>
> +    </div>
> +  </tal:rescan>
> +
> +  <div class="yui-g" tal:condition="view/show_rescan_link">
>      <div class="portlet">
> -      <div id="branch-pending-updates" class="pending-update">
> -        <h3>Updating branch...</h3>
> -        <p>
> -          Launchpad is processing new changes to this branch which will be
> -          available in a few minutes. Reload to see the changes.
> +      <div id="branch-scan-failed" class="pending-update">
> +        <h3>Branch scan failed</h3>
> +        <p>
> +          Scanning this branch for changes has failed, you can manually rescan if required.

Let's not have run-on sentences in user-visible prose.  Maybe:

  Scanning this branch for changes failed.  You can manually rescan if required.

> +        </p>
> +        <p>
> +          <form action="./+rescan" name="launchpadform" method="post" enctype="multipart/form-data" accept-charset="UTF-8">

"./" looks a bit odd in a URL.  I think this could just be action="+rescan".

> +            <input id="field.actions.rescan" class="button" type="submit"
> +             name="field.actions.rescan" value="Rescan" />
> +          </form>
>          </p>
>        </div>
>      </div>
>    </div>
>  
> +
>    <div class="yui-g">
>      <div class="portlet" id="recent-revisions">
>        <h2>Recent revisions</h2>


-- 
https://code.launchpad.net/~twom/launchpad/manually-rescan-link/+merge/362139
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.