launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> 
> === modified file 'lib/lp/code/browser/codeimport.py'
> --- lib/lp/code/browser/codeimport.py	2016-10-06 15:59:34 +0000
> +++ lib/lp/code/browser/codeimport.py	2016-10-13 12:55:20 +0000
> @@ -543,6 +549,10 @@
>          else:
>              raise AssertionError('Unknown rcs_type for code import.')
>  
> +        if (self.code_import.target_rcs_type ==
> +                TargetRevisionControlSystems.GIT):

I find the contrast between the == BZR above and the == GIT here disturbing. Perhaps != BZR?

> +            self.form_fields = self.form_fields.omit('whiteboard')
> +
>      def _showButtonForStatus(self, status):
>          """If the status is different, and the user is super, show button."""
>          return self._super_user and self.code_import.review_status != status
> 
> === added file 'lib/lp/code/templates/gitrepository-import-details.pt'
> --- lib/lp/code/templates/gitrepository-import-details.pt	1970-01-01 00:00:00 +0000
> +++ lib/lp/code/templates/gitrepository-import-details.pt	2016-10-13 12:55:20 +0000
> @@ -0,0 +1,95 @@
> +<div
> +  xmlns:tal="http://xml.zope.org/namespaces/tal";
> +  xmlns:metal="http://xml.zope.org/namespaces/metal";
> +  xmlns:i18n="http://xml.zope.org/namespaces/i18n";
> +  tal:define="context_menu view/context/menu:context">
> +
> +  <tal:imported-repository
> +      condition="context/repository_type/enumvalue:IMPORTED">
> +    <div id="import-details" tal:define="repository context;
> +                                         code_import repository/code_import">
> +      <tal:has-codeimport condition="repository/code_import"
> +                          define="code_import repository/code_import">
> +
> +        <div><strong>Import Status:</strong>
> +          <span tal:attributes="class string:codeimport${code_import/review_status/name}"
> +                tal:content="code_import/review_status/title"/>
> +        </div>
> +
> +        <tal:git-import condition="code_import/rcs_type/enumvalue:GIT">
> +          <p id="git-import-details">
> +            This repository is an import of the Git repository at
> +            <tal:is-web-url condition="view/url_is_web">
> +              <a tal:attributes="href code_import/url"
> +                 tal:content="code_import/url" />.
> +            </tal:is-web-url>
> +            <tal:not-web-url condition="not: view/url_is_web">
> +              <span tal:replace="code_import/url" />.
> +            </tal:not-web-url>
> +          </p>
> +        </tal:git-import>
> +
> +        <tal:has-job define="job code_import/import_job"
> +                     condition="job">
> +          <div>
> +          <tal:is-running condition="job/state/enumvalue:RUNNING">
> +            An import is currently running on
> +            <tal:machine content="structure job/machine/fmt:link" />,
> +            and was started
> +            <tal:date-started replace="job/date_started/fmt:displaydate">
> +              2 hours ago
> +            </tal:date-started>.
> +            <tal:is-logtail condition="job/logtail">
> +              The last few lines of the job's output were:
> +              <div class="logtail">
> +                <tal:logtail content="structure job/logtail/fmt:nice_pre" />
> +              </div>
> +            </tal:is-logtail>
> +          </tal:is-running>
> +          <tal:not-running condition="not: job/state/enumvalue:RUNNING">
> +            The next import is scheduled to run
> +            <tal:overdue condition="job/isOverdue">
> +              as soon as possible<tal:requested-by
> +                condition="job/requesting_user">
> +                (requested by
> +                <tal:requested-by-user
> +                   replace="structure job/requesting_user/fmt:link">
> +                  Some user.
> +                </tal:requested-by-user>)</tal:requested-by>.
> +            </tal:overdue>
> +            <tal:not-overdue condition="not: job/isOverdue">
> +              <tal:date-started replace="job/date_due/fmt:displaydate">
> +                in 2 hours
> +              </tal:date-started>.
> +            </tal:not-overdue>
> +          </tal:not-running>
> +          </div>
> +        </tal:has-job>
> +
> +        <tal:failing condition="code_import/review_status/enumvalue:FAILING">
> +          <div id="failing-try-again" class="message warning">
> +            The import has been suspended because it failed
> +            <tal:failure-limit content="modules/lp.services.config/config/codeimport/consecutive_failure_limit"/>
> +            or more times in succession.
> +          </div>
> +        </tal:failing>
> +
> +        <tal:last-successful condition="code_import/date_last_successful">
> +          <p>
> +            Last successful import was
> +            <tal:last-successful replace="code_import/date_last_successful/fmt:displaydate">
> +              2 hours ago
> +            </tal:last-successful>.
> +          </p>
> +        </tal:last-successful>
> +
> +        <div id="import-results" tal:condition="view/latest_code_import_results">
> +          <tal:result repeat="result view/latest_code_import_results">
> +            <metal:result use-macro="code_import/@@+macros/show_result"/>
> +          </tal:result>
> +        </div>
> +      </tal:has-codeimport>
> +    </div>
> +  </tal:imported-repository>
> +
> +</div>

Is this non-trivially different from the branch equivalent? It's a reasonably complex bit of UI that would be nice to not duplicate.



-- 
https://code.launchpad.net/~cjwatson/launchpad/codeimport-git-read-only-views/+merge/308374
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.