launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> 
> === modified file 'lib/lp/snappy/interfaces/snapjob.py'
> --- lib/lp/snappy/interfaces/snapjob.py	2018-06-15 13:21:14 +0000
> +++ lib/lp/snappy/interfaces/snapjob.py	2018-08-03 20:41:32 +0000
> @@ -103,3 +113,10 @@
>          :raises: `NotFoundError` if there is no job with the specified snap
>              and ID, or its `job_type` is not `SnapJobType.REQUEST_BUILDS`.
>          """
> +
> +    def findBuildsForJobs(jobs):
> +        """Find builds resulting from an iterable of `SnapRequestBuildJob`s.
> +
> +        :return: A dictionary mapping `SnapRequestBuildJob` IDs to lists of
> +            their resulting builds.
> +        """

This should document that the user argument only performs access checks on the archives, not the snaps or builds.

> 
> === modified file 'lib/lp/snappy/javascript/snap.update_build_statuses.js'
> --- lib/lp/snappy/javascript/snap.update_build_statuses.js	2017-08-31 13:35:55 +0000
> +++ lib/lp/snappy/javascript/snap.update_build_statuses.js	2018-08-03 20:41:32 +0000
> @@ -15,11 +15,113 @@
>      module.pending_states = [
>          "NEEDSBUILD", "BUILDING", "UPLOADING", "CANCELLING"];
>  
> +    module.update_date_built = function(node, build_summary) {
> +        node.set("innerHTML", build_summary.when_complete);

I know this is existing code, but when_complete is a displaydate so should always be text, not HTML.

> +        if (build_summary.when_complete_estimate) {
> +            node.appendChild(document.createTextNode(' (estimated)'));
> +        }
> +        if (build_summary.build_log_url !== null) {
> +            var new_link = Y.Node.create(
> +                '<a class="sprite download">buildlog</a>');
> +            new_link.setAttribute('href', build_summary.build_log_url);
> +            node.appendChild(document.createTextNode(' '));
> +            node.appendChild(new_link);
> +            if (build_summary.build_log_size !== null) {
> +                node.appendChild(document.createTextNode(' '));
> +                node.append("(" + build_summary.build_log_size + " bytes)");
> +            }
> +        }
> +    };
> +
>      module.domUpdate = function(table, data_object) {
> -        Y.each(data_object, function(build_summary, build_id) {
> +        var tbody = table.one('tbody');
> +        if (tbody === null) {
> +            return;
> +        }
> +        var tbody_changed = false;
> +
> +        Y.each(data_object['requests'], function(request_summary, request_id) {
> +            var tr_elem = tbody.one('tr#request-' + request_id);
> +            if (tr_elem === null) {
> +                return;
> +            }
> +
> +            if (request_summary['status'] === 'FAILED') {
> +                // XXX cjwatson 2018-06-18: Maybe we should show the error
> +                // message in this case, but we don't show non-pending
> +                // requests in the non-JS case, so it's not clear where
> +                // would be appropriate.
> +                tr_elem.remove();
> +                tbody_changed = true;
> +                return;
> +            } else if (request_summary['status'] === 'COMPLETED') {
> +                // Insert rows for the new builds.
> +                Y.Array.each(request_summary['builds'],
> +                             function(build_summary) {
> +                    // Construct the new row.
> +                    var new_row = Y.Node.create('<tr/>')
> +                        .set('id', 'build-' + build_summary.id);
> +                    var new_build_status = Y.Node.create('<td/>');
> +                    new_build_status.append('<img/>');
> +                    new_build_status.appendChild('<a/>')
> +                        .set('href', build_summary.self_link);
> +                    Y.lp.buildmaster.buildstatus.update_build_status(
> +                        new_build_status, build_summary);
> +                    new_row.append(new_build_status);
> +                    var new_datebuilt = Y.Node.create(
> +                        '<td class="datebuilt"/>');
> +                    if (build_summary.when_complete !== null) {
> +                        module.update_date_built(new_datebuilt, build_summary);
> +                    }
> +                    new_row.append(new_datebuilt);
> +                    var new_arch = Y.Node.create('<td/>');
> +                    var new_arch_link = Y.Node.create(
> +                        '<a class="sprite distribution"/>');
> +                    new_arch_link.set(
> +                        'href', build_summary.distro_arch_series_link);
> +                    new_arch_link.appendChild(document.createTextNode(
> +                        build_summary.architecture_tag));
> +                    new_arch.append(new_arch_link);
> +                    new_row.append(new_arch);
> +                    new_row.append(
> +                        '<td>' + build_summary.archive_link + '</td>');

Would this be prettier as a big single Y.Node.create with a bunch of selector sets afterwards?

> +
> +                    // Insert the new row, maintaining descending-ID sorted
> +                    // order.
> +                    var tr_next = null;
> +                    tbody.get('children').some(function(tr) {
> +                        var tr_id = tr.get('id');
> +                        if (tr_id !== null &&
> +                                tr_id.substr(0, 6) === 'build-') {
> +                            var build_id = parseInt(
> +                                tr_id.replace('build-', ''), 10);
> +                            if (!isNaN(build_id) &&
> +                                    build_id < build_summary.id) {
> +                                tr_next = tr;
> +                                return true;
> +                            }
> +                        }
> +                        return false;
> +                    });
> +                    tbody.insert(new_row, tr_next);
> +                });
> +
> +                // Remove the completed build request row.
> +                tr_elem.remove();
> +                tbody_changed = true;
> +                return;
> +            }
> +        });
> +
> +        if (tbody_changed) {
> +            var anim = Y.lp.anim.green_flash({node: tbody});
> +            anim.run();
> +        }
> +
> +        Y.each(data_object['builds'], function(build_summary, build_id) {
>              var ui_changed = false;
>  
> -            var tr_elem = Y.one("tr#build-" + build_id);
> +            var tr_elem = tbody.one("tr#build-" + build_id);
>              if (tr_elem === null) {
>                  return;
>              }
> 
> === modified file 'lib/lp/snappy/model/snap.py'
> --- lib/lp/snappy/model/snap.py	2018-07-30 09:07:30 +0000
> +++ lib/lp/snappy/model/snap.py	2018-08-03 20:41:32 +0000
> @@ -175,9 +180,19 @@
>      webservice-friendly view of an asynchronous build request.
>      """
>  
> -    def __init__(self, snap, id):
> -        self._job = getUtility(ISnapRequestBuildsJobSource).getBySnapAndID(
> -            snap, id)
> +    def __init__(self, snap, id, job=None):
> +        if job is None:
> +            job_source = getUtility(ISnapRequestBuildsJobSource)
> +            job = job_source.getBySnapAndID(snap, id)
> +        if snap != job.snap:
> +            raise AssertionError(
> +                "Mismatched SnapRequestBuildsJob: expected %r, got %r" %
> +                (snap, job.snap))
> +        if id != job.job_id:
> +            raise AssertionError(
> +                "Mismatched SnapRequestBuildsJob: expected %d, got %d" %
> +                (id, job.job_id))
> +        self._job = job
>          self.snap = snap
>          self.id = id

Is this perhaps SnapBuildRequest.from_job(job)?

>  
> @@ -623,6 +643,15 @@
>          """See `ISnap`."""
>          return SnapBuildRequest(self, job_id)
>  
> +    @property
> +    def pending_build_requests(self):
> +        """See `ISnap`."""
> +        job_source = getUtility(ISnapRequestBuildsJobSource)
> +        return [
> +            SnapBuildRequest(self, job.job_id, job=job)
> +            for job in job_source.findBySnap(
> +                self, statuses=(JobStatus.WAITING, JobStatus.RUNNING))]

Should this be sorted?

> +
>      def _getBuilds(self, filter_term, order_by):
>          """The actual query to get the builds."""
>          query_args = [
> @@ -678,6 +711,67 @@
>                  }
>          return result
>  
> +    def getBuildSummaries(self, request_ids=None, build_ids=None):
> +        """See `ISnap`."""
> +        all_build_ids = []
> +        result = {"requests": {}, "builds": {}}
> +
> +        if request_ids:
> +            job_source = getUtility(ISnapRequestBuildsJobSource)
> +            jobs = job_source.findBySnap(self, job_ids=request_ids)
> +            requests = [
> +                SnapBuildRequest(self, job.job_id, job=job) for job in jobs]
> +            builds_by_request = job_source.findBuildsForJobs(jobs)

Does this need a user argument?

> +            for builds in builds_by_request.values():
> +                # It's safe to remove the proxy here, because the IDs will
> +                # go through Snap._getBuilds which checks visibility.  This
> +                # saves an Archive query per build in the security adapter.
> +                all_build_ids.extend(
> +                    [removeSecurityProxy(build).id for build in builds])
> +        else:
> +            requests = []
> +
> +        if build_ids:
> +            all_build_ids.extend(build_ids)
> +
> +        all_build_summaries = self.getBuildSummariesForSnapBuildIds(
> +            all_build_ids)
> +
> +        for request in requests:
> +            build_summaries = []
> +            for build in sorted(
> +                    builds_by_request[request.id], key=attrgetter("id"),
> +                    reverse=True):
> +                if build.id in all_build_summaries:
> +                    # Include enough information for
> +                    # snap.update_build_statuses.js to populate new build
> +                    # rows.
> +                    build_summary = {
> +                        "self_link": canonical_url(
> +                            build, path_only_if_possible=True),
> +                        "id": build.id,
> +                        "distro_arch_series_link": canonical_url(
> +                            build.distro_arch_series,
> +                            path_only_if_possible=True),
> +                        "architecture_tag": (
> +                            build.distro_arch_series.architecturetag),
> +                        "archive_link": ArchiveFormatterAPI(
> +                            build.archive).link(None),
> +                        }
> +                    build_summary.update(all_build_summaries[build.id])
> +                    build_summaries.append(build_summary)
> +            result["requests"][request.id] = {
> +                "status": request.status.name,
> +                "error_message": request.error_message,
> +                "builds": build_summaries,
> +                }
> +
> +        for build_id in (build_ids or []):
> +            if build_id in all_build_summaries:
> +                result["builds"][build_id] = all_build_summaries[build_id]
> +
> +        return result
> +
>      @property
>      def builds(self):
>          """See `ISnap`."""


-- 
https://code.launchpad.net/~cjwatson/launchpad/snap-request-builds-ui/+merge/348299
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.