canonical-ubuntu-qa team mailing list archive

[Skia <mp+469607@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing

New round of comment.
In addition, you can probably improve the commit history:
- add the weird error in the first commit
- do the refactors and new macros in individual commits
- finally do a single final commit with your new page

Diff comments:

> diff --git a/charms/focal/autopkgtest-web/webcontrol/browse.cgi b/charms/focal/autopkgtest-web/webcontrol/browse.cgi
> index 3029f4f..6ea1301 100755
> --- a/charms/focal/autopkgtest-web/webcontrol/browse.cgi
> +++ b/charms/focal/autopkgtest-web/webcontrol/browse.cgi
> @@ -756,6 +758,69 @@ def package_release_arch(package, release, arch, _=None):
>      )
>  
>  
> +@app.route("/run/<uuid>")
> +def get_by_uuid(uuid):
> +    package = ""
> +    release = ""
> +    arch = ""
> +    cursor = db_con.cursor()
> +    cursor.row_factory = sqlite3.Row
> +    result = cursor.execute(
> +        "SELECT run_id, version, triggers, duration, exitcode, requester, "
> +        "env, uuid, package, release, arch FROM result "
> +        "LEFT JOIN test ON result.test_id = test.id "
> +        "WHERE uuid = ?",
> +        (uuid,),
> +    ).fetchone()
> +
> +    if result is None:
> +        raise NotFound("uuid", uuid)
> +
> +    requester = result["requester"] if result["requester"] else "-"
> +    code = human_exitcode(result["exitcode"])
> +    version = result["version"]
> +    triggers = result["triggers"]
> +    additional_params = result["env"]
> +    package = result["package"]
> +    release = result["release"]
> +    arch = result["arch"]
> +
> +    show_retry = code != "pass"

Did you discuss this with Brian, about always showing that button? That would be particularly true on that page displaying a single result.

> +    url = os.path.join(
> +        swift_container_url % release,
> +        release,
> +        arch,
> +        srchash(package),
> +        package,
> +        result["run_id"],
> +    )
> +    all_proposed = (
> +        additional_params is not None and "all-proposed=1" in additional_params
> +    )
> +
> +    test_results = (

I know this is not the style used by other views, but if we could change that to be a dict instead of a tuple, that would improve readability of the template by a lot, and also reduce the risk of showing the wrong data by mistake.

> +        version,
> +        triggers,
> +        additional_params,
> +        human_date(result["run_id"]),
> +        human_sec(int(result["duration"])),
> +        requester,
> +        code,
> +        url,
> +        show_retry,
> +        all_proposed,
> +        result["uuid"],
> +    )
> +    return render(
> +        "browse-run.html",
> +        package=package,
> +        release=release,
> +        arch=arch,
> +        test_results=test_results,
> +        title_suffix="- %s/%s/%s" % (package, release, arch),
> +    )
> +
> +
>  @app.route("/releases/<release>")
>  @app.route("/releases/<release>/<arch>")
>  def release(release, arch=None):
> diff --git a/charms/focal/autopkgtest-web/webcontrol/templates/macros.html b/charms/focal/autopkgtest-web/webcontrol/templates/macros.html
> index 0a585c5..e3cd98f 100644
> --- a/charms/focal/autopkgtest-web/webcontrol/templates/macros.html
> +++ b/charms/focal/autopkgtest-web/webcontrol/templates/macros.html
> @@ -154,6 +149,15 @@
>    Launchpad</a>
>  {%- endmacro %}
>  
> +{% macro retry_button(triggers, all_proposed) -%}

YES! That's what we want! :-)

> +  {% set ts = triggers.split()|map('urlencode')|join("&trigger=")|safe %}
> +  {% if all_proposed %}
> +    <a href="{{ url_for("index_root") }}request.cgi?release={{ release }}&arch={{ arch }}&package={{ package|urlencode }}&trigger={{ ts }}&all-proposed=1">&#9851;</a>
> +  {% else %}
> +    <a href="{{ url_for("index_root") }}request.cgi?release={{ release }}&arch={{ arch }}&package={{ package|urlencode }}&trigger={{ ts }}">&#9851;</a>
> +  {% endif %}
> +{%- endmacro %}
> +
>  {% macro nonexistent_package() -%}
>    <p>
>      Oops! Looks like this package has no previous results. The package itself may not exist - you can check by clicking the Launchpad icon.


-- 
https://code.launchpad.net/~uralt/autopkgtest-cloud/+git/autopkgtest-cloud/+merge/469607
Your team Canonical's Ubuntu QA is requested to review the proposed merge of ~uralt/autopkgtest-cloud:get-by-uuid into autopkgtest-cloud:master.