From c172e8883d8f32fced5e02f9b6faaacc913df27b Mon Sep 17 00:00:00 2001
From: Marius Kittler <mkittler@suse.de>
Date: Mon, 22 Jul 2019 13:35:41 +0200
Subject: [PATCH] Improve rendering summary in test result overview

---
 lib/OpenQA/WebAPI/Controller/Test.pm | 73 ++++++++++++++++++----------
 t/10-tests_overview.t                | 19 ++++++++
 t/ui/10-tests_overview.t             |  2 +-
 templates/test/overview.html.ep      |  4 +-
 4 files changed, 69 insertions(+), 29 deletions(-)

diff --git a/lib/OpenQA/WebAPI/Controller/Test.pm b/lib/OpenQA/WebAPI/Controller/Test.pm
index 33cb5a4b..9de2ec9b 100644
--- a/lib/OpenQA/WebAPI/Controller/Test.pm
+++ b/lib/OpenQA/WebAPI/Controller/Test.pm
@@ -1,4 +1,4 @@
-# Copyright (C) 2015-2016 SUSE LLC
+# Copyright (C) 2015-2019 SUSE LLC
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -22,9 +22,10 @@ use OpenQA::Jobs::Constants;
 use OpenQA::Schema::Result::Jobs;
 use OpenQA::Schema::Result::JobDependencies;
 use OpenQA::WebAPI::Controller::Developer;
+use OpenQA::Utils qw(determine_web_ui_web_socket_url get_ws_status_only_url);
+use Mojo::ByteStream;
 use File::Basename;
 use POSIX 'strftime';
-use OpenQA::Utils qw(determine_web_ui_web_socket_url get_ws_status_only_url);
 
 sub referer_check {
     my ($self) = @_;
@@ -569,6 +570,31 @@ sub prepare_job_results {
     return (\%archs, \%results, $aggregated);
 }
 
+# appends the specified $distri and $version to $array_to_add_parts_to as string or if $raw as Mojo::ByteStream
+sub _add_distri_and_version_to_summary {
+    my ($array_to_add_parts_to, $distri, $version, $raw) = @_;
+
+    for my $part ($distri, $version) {
+        # handle case when multiple distri/version parameters have been specified
+        $part = $part->{-in} if (ref $part eq 'HASH');
+        next unless $part;
+
+        # separate distri and version with a whitespace
+        push(@$array_to_add_parts_to, ' ') if (@$array_to_add_parts_to);
+
+        if (ref $part eq 'ARRAY') {
+            # separate multiple distris/versions using a slash
+            if (@$part) {
+                push(@$array_to_add_parts_to, map { ($raw ? Mojo::ByteStream->new($_) : $_, '/') } @$part);
+                pop(@$array_to_add_parts_to);
+            }
+        }
+        elsif (ref $part ne 'HASH') {
+            push(@$array_to_add_parts_to, $raw ? Mojo::ByteStream->new($part) : $part);
+        }
+    }
+}
+
 # A generic query page showing test results in a configurable matrix
 sub overview {
     my ($self) = @_;
@@ -585,43 +611,38 @@ sub overview {
     ($stash{archs}, $stash{results}, $stash{aggregated}) = $self->prepare_job_results(\@latest_jobs);
 
     # determine distri/version from job results if not explicitely specified via search args
-    my @distris     = keys %{$stash{results}};
+    my @distris = keys %{$stash{results}};
+    my $formatted_distri;
+    my $formatted_version;
     my $only_distri = scalar @distris == 1;
     if (!defined $stash{distri} && $only_distri) {
-        my $distri = $stash{distri} = $distris[0];
+        $formatted_distri = $distris[0];
         if (!defined $stash{version}) {
-            my @versions = keys %{$stash{results}->{$distri}};
-            $stash{version} = $versions[0] if (scalar @versions == 1);
+            my @versions = keys %{$stash{results}->{$formatted_distri}};
+            $formatted_version = $versions[0] if (scalar @versions == 1);
         }
     }
 
-    # determine summary name for "Overall Summary of ..."
-    my $summary_name;
+    # compose summary for "Overall Summary of ..."
+    my @summary_parts;
     if (@$groups) {
-        $summary_name = join(', ',
-            map { $self->link_to($_->name => $self->url_for('group_overview', groupid => $_->id)) } @$groups);
+        # use groups if present
+        push(@summary_parts,
+            map { ($self->link_to($_->name => $self->url_for('group_overview', groupid => $_->id)), ', ') } @$groups);
+        pop(@summary_parts);
     }
     else {
-        my @variables = ($stash{distri}, $stash{version});
-        my @formatted_parts;
-        for my $part (@variables) {
-            $part = $part->{-in} if (ref $part eq 'HASH');
-            next unless $part;
-
-            if (ref $part eq 'ARRAY') {
-                push(@formatted_parts, join('/', @$part));
-            }
-            elsif (ref $part ne 'HASH') {
-                push(@formatted_parts, $part);
-            }
-        }
-        $summary_name = join(' ', @formatted_parts) if (@formatted_parts);
+        # add pre-formatted distri and version as Mojo::ByteStream
+        _add_distri_and_version_to_summary(\@summary_parts, $formatted_distri, $formatted_version, 1);
+
+        # add distri and version from query parameters as regular strings
+        _add_distri_and_version_to_summary(\@summary_parts, $stash{distri}, $stash{version}, 0);
     }
 
     $self->stash(
         %stash,
-        summary_name => $summary_name,
-        only_distri  => $only_distri,
+        summary_parts => \@summary_parts,
+        only_distri   => $only_distri,
     );
     $self->respond_to(
         json => {json     => \%stash},
diff --git a/t/10-tests_overview.t b/t/10-tests_overview.t
index f6d9d294..e8f60a7c 100644
--- a/t/10-tests_overview.t
+++ b/t/10-tests_overview.t
@@ -65,6 +65,25 @@ like(get_summary, qr/Overall Summary of opensuse 13\.1 build 0091/i, 'specifying
 $form = {distri => 'opensuse', version => '13.1', build => '0091', groupid => 1001};
 $t->get_ok('/tests/overview' => form => $form)->status_is(200);
 like(get_summary, qr/Overall Summary of opensuse build 0091/i, 'specifying groupid parameter');
+subtest 'escaping works' => sub {
+    $form = {
+        distri  => '<img src="distri">',
+        version => ['<img src="version1">', '<img src="version2">'],
+        build   => '<img src="build">'
+    };
+    $t->get_ok('/tests/overview' => form => $form)->status_is(200);
+    my $body = $t->tx->res->body;
+    unlike($body, qr/<img src="distri">/,                         'no unescaped image tag for distri');
+    unlike($body, qr/<img src="version1">.*<img src="version2">/, 'no unescaped image tags for version');
+    unlike($body, qr/<img src="build">/,                          'no unescaped image tag for build');
+    like($body, qr/&lt;img src=&quot;distri&quot;&gt;/, 'image tag for distri escaped');
+    like(
+        $body,
+        qr/&lt;img src=&quot;version1&quot;&gt;.*&lt;img src=&quot;version2&quot;&gt;/,
+        'image tags for version escaped'
+    );
+    like($body, qr/&lt;img src=&quot;build&quot;&gt;/, 'image tag for build escaped');
+};
 
 #
 # Overview of build 0048
diff --git a/t/ui/10-tests_overview.t b/t/ui/10-tests_overview.t
index 1e83f93d..ea78e440 100644
--- a/t/ui/10-tests_overview.t
+++ b/t/ui/10-tests_overview.t
@@ -208,7 +208,7 @@ subtest 'filtering by distri' => sub {
         $driver->get('/tests/overview?distri=foo&distri=opensuse&distri=bar&version=13.1&build=0091');
         check_build_0091_defaults;
         is(
-            OpenQA::Test::Case::trim_whitespace($driver->find_element('#summary .card-header b')->get_text()),
+            OpenQA::Test::Case::trim_whitespace($driver->find_element('#summary .card-header strong')->get_text()),
             'foo/opensuse/bar 13.1',
             'filter also visible in summary'
         );
diff --git a/templates/test/overview.html.ep b/templates/test/overview.html.ep
index f618337e..2d5cb72f 100644
--- a/templates/test/overview.html.ep
+++ b/templates/test/overview.html.ep
@@ -11,8 +11,8 @@
     <div id="summary" class="card <%= ($aggregated->{failed} + $aggregated->{incomplete}) ? 'border-danger' : 'border-success' %>">
         <div class="card-header">
             Overall Summary of
-            % if ($summary_name) {
-                <b><%= b $summary_name %></b>
+            % if (@$summary_parts) {
+                <strong><% for my $part (@$summary_parts) { %><%= $part %><% } %></strong>
             % }
             % else {
                 multiple distri/version
-- 
2.22.0