Merge ~andersson123/autopkgtest-cloud:fix-package-page-nonexistent-package into autopkgtest-cloud:master

[Tim Andersson]

fix: web: Don't show package pages for packages that don't exist

This commit changes the behaviour when a user tries to reach a package
or results page for a package that doesn't exist.

The results page used to throw an error stating that the package doesn't
exist, however, I think this is slightly innaccurate - the package could
exist, but we could just have no test results for it. This is infact
not really, an error, and not something we should surface *as* an error.

The behaviour, with this commit, is as follows:
On the results and package pages, if the user goes to one of these pages
for a package that doesn't exist, they get the following message:
```
Oops! Looks like this package has no previous results. The package
itself may not exist - you can check by clicking the Launchpad icon.
```

I think this is better because, a, we are no longer throwing an error,
and b, because the user can now validate the package exists by clicking
on the Launchpad icon.

Overall I think this is just an accurate representation of all the
potential possibilities when going to either of these pages with an
"invalid" package name.

There is the possibility of checking if the package exists via
Launchpad, but that'd be an http request to Launchpad every time a user
views a package or results page, which is a waste of resources, and
seems unnecessary.

Fixes bug LP: #2058059

This MP changes the message we surface to users when they request a package or results page for an "invalid" package, and no longer surfaces it as an error, but instead with a verbose message about the two possibilities in this scenario.

Overall, it's a small change for improved UX. It's low priority but worth taking a look at.

[Skia (hyask)]

Looking good!

review: Approve

[Brian Murray (brian-murray)]

Thinking about this some more I'm not sure what is the worst end user experience.

A) Mistype a package name and get a web page with no test results.

OR

B) Properly type a package name and get a 404.

I'm leaning towards B being the worst end user experience since at least with A I can click on the LP url and get a nicely formatted 404. Especially, since case A also will not show any releases which is a pretty good indicator you misspelled the package name.

review: Disapprove

[Tim Andersson (andersson123)]

Okay, understood, so, should I close this MP? And the related bug?

[Brian Murray (brian-murray)]

Well ideally the autopkgtest-web workers would know what is and is not a package and then could either return a 404 or a page saying the package has no tests. However, I think that makes the bug a lower priority given the difficulty of it.

[Tim Andersson (andersson123)]

How about 2 outcomes?

outcome 1:
- user types in a package which exists in launchpad but has no previous results - is greeted with a page stating as such
- user types in a package which doesn't exist in launchpad but does have previous results - is greeted with the normal package browsing page
- user types in a package which doesn't exist in launchpad and has no previous results - we then assume the package doesn't exist and we tell the user as such

The above solution is quite easy I believe. Let me know what you think. If you like that solution I can go ahead and implement it.

[Tim Andersson (andersson123)]

waiting on feedback for this MP.

[Tim Andersson (andersson123)]

This one needs more info.

[Brian Murray (brian-murray)]

> How about 2 outcomes?
>
> outcome 1:
> - user types in a package which exists in launchpad but has no previous
> results - is greeted with a page stating as such

This doesn't seem much different from what is currently implied from the existing page so that makes sense.

> - user types in a package which doesn't exist in launchpad but does have
> previous results - is greeted with the normal package browsing page

This is impossible. Seriously, any package for which we have results should also have a +source page in Launchpad.

> - user types in a package which doesn't exist in launchpad and has no previous
> results - we then assume the package doesn't exist and we tell the user as
> such

That sounds good and is what the original bug report was about.

> The above solution is quite easy I believe. Let me know what you think. If you
> like that solution I can go ahead and implement it.

[Brian Murray (brian-murray)]

review: Needs Fixing

[Tim Andersson (andersson123)]

I don't think it's practical to check the launchpad link when the user is browsing. Then we'd be making a request to launchpad every single time someone views a results package, which seems very wasteful. I think the better thing is to just have a message for when a package doesn't have any previous results, keep the launchpad link there and then the user can click on it if need be. I'll implement this.

[Tim Andersson (andersson123)]

amended and ready for review.

[Skia (hyask)]

review: Abstain

[Tim Andersson (andersson123)]

I've set this back to work in progress - I imagine the user page MP will get merged before this one, and this MP will need to be rebased off of that, as both MP's touch browse-results.html

[Tim Andersson (andersson123)]

The work mentioned in the previous comment has now been completed. This is ready for re-review

[Skia (hyask)]

LGTM :-)

review: Approve

[Skia (hyask)]

Be careful, CI is red

[Tim Andersson (andersson123)]

Thanks for the approval! I'll fix the CI and merge :)

diff --git a/charms/focal/autopkgtest-web/webcontrol/browse.cgi b/charms/focal/autopkgtest-web/webcontrol/browse.cgi
index d4891ee..a4af4d8 100755
--- a/charms/focal/autopkgtest-web/webcontrol/browse.cgi
+++ b/charms/focal/autopkgtest-web/webcontrol/browse.cgi
@@ -599,7 +599,12 @@ def package_release_arch(package, release, arch, _=None):
     test_id = get_test_id(release, arch, package)
     if test_id is None:
         return render(
-            "browse-error.html", error="Package does not exist", code=404
+            "browse-results.html",
+            package=package,
+            release=release,
+            arch=arch,
+            package_results=[],
+            title_suffix="- %s/%s/%s" % (package, release, arch),
         )
 
     seen = set()
diff --git a/charms/focal/autopkgtest-web/webcontrol/templates/browse-package.html b/charms/focal/autopkgtest-web/webcontrol/templates/browse-package.html
index b6a7106..2ca3add 100644
--- a/charms/focal/autopkgtest-web/webcontrol/templates/browse-package.html
+++ b/charms/focal/autopkgtest-web/webcontrol/templates/browse-package.html
@@ -8,29 +8,48 @@
     <li>{{ macros.excuses_link(package) }}</li>
   </ul>
 
-  <table class="table" style="width: auto">
-    <tr>
-      <th />
-      {% for r in releases %}<th>{{r}}</th>{% endfor %}
-    </tr>
-
-    {% for a in arches %}
+  {% if package_results %}
+    <table class="table" style="width: auto">
       <tr>
-        <th>{{a}}</th>
-        {% for r in releases %}
-          <td class="{{ results[r][a] }}">
-            <a href="{{package}}/{{r}}/{{a}}">{{ results[r][a] }}</a>
-          </td>
-        {% endfor %}
+        <th />
+        {% for r in releases %}<th>{{r}}</th>{% endfor %}
       </tr>
-    {% endfor %}
-  </table>
 
-  <hr>
-  <h3>Running tests</h3>
+      {% for a in arches %}
+        <tr>
+          <th>{{a}}</th>
+          {% for r in releases %}
+            <td class="{{ results[r][a] }}">
+              <a href="{{package}}/{{r}}/{{a}}">{{ results[r][a] }}</a>
+            </td>
+          {% endfor %}
+        </tr>
+
+        {% for a in arches %}
+          <tr>
+            <th>{{a}}</th>
+            {% for r in releases %}
+              <td class="{{ results[r][a] }}">
+                <a href="{{package}}/{{r}}/{{a}}">{{ results[r][a] }}</a>
+              </td>
+            {% endfor %}
+          </tr>
+        {% endfor %}
+      </table>
+
+      <hr>
+      <h3>Running tests</h3>
+
+      {% for p, info in running.items()|sort %}{{ macros.display_running_job(p, info) }}{% endfor %}
+
+      <h3>Queued tests</h3>
+      {{ macros.display_queues_info(queues_info) }}
+    {% else %}
+      {{ macros.nonexistent_package() }}
+    {% endif %}
 
-  {% for p, info in running.items()|sort %}{{ macros.display_running_job(p, info) }}{% endfor %}
+    {% for p, info in running.items()|sort %}{{ macros.display_running_job(p, info) }}{% endfor %}
 
-  <h3>Queued tests</h3>
-  {{ macros.display_queues_info(queues_info) }}
-{% endblock content %}
+    <h3>Queued tests</h3>
+    {{ macros.display_queues_info(queues_info) }}
+  {% endblock content %}
diff --git a/charms/focal/autopkgtest-web/webcontrol/templates/browse-results.html b/charms/focal/autopkgtest-web/webcontrol/templates/browse-results.html
index 2978c8c..9d15751 100644
--- a/charms/focal/autopkgtest-web/webcontrol/templates/browse-results.html
+++ b/charms/focal/autopkgtest-web/webcontrol/templates/browse-results.html
@@ -10,15 +10,19 @@
     <li>{{ macros.excuses_link(package, release) }}</li>
   </ul>
 
-  <table class="table">
-    <tr>{{ macros.set_up_results_table() }}</tr>
+  {% if package_results %}
+    <table class="table">
+      <tr>{{ macros.set_up_results_table() }}</tr>
 
-    {% for row in package_results %}
-      <tr {% if row[6] in ["running", "queued"] %}class="unfinished"{% endif %}>
-        {{ macros.results_table_core(row[0], row[1], row[2], row[3], row[4], row[5], row[6], row[7], row[8], row[9], row[10], package, release, arch) }}
-      </tr>
-    {% endfor %}
-  </table>
+      {% for row in package_results %}
+        <tr {% if row[6] in ["running", "queued"] %}class="unfinished"{% endif %}>
+          {{ macros.results_table_core(row[0], row[1], row[2], row[3], row[4], row[5], row[6], row[7], row[8], row[9], row[10], package, release, arch) }}
+        </tr>
+      {% endfor %}
+    </table>
+  {% else %}
+    {{ macros.nonexistent_package() }}
+  {% endif %}
 
   <p>
     To ease the browsing of logs, you can use <a href="{{ url_for('static', filename='logs-viewer.user.js') }}">this userscript</a> with any extension supporting that, like <a href="https://www.tampermonkey.net/">TamperMonkey</a>.
diff --git a/charms/focal/autopkgtest-web/webcontrol/templates/macros.html b/charms/focal/autopkgtest-web/webcontrol/templates/macros.html
index d92d357..0a585c5 100644
--- a/charms/focal/autopkgtest-web/webcontrol/templates/macros.html
+++ b/charms/focal/autopkgtest-web/webcontrol/templates/macros.html
@@ -153,3 +153,9 @@
     <img src="{{ url_for('static', filename='launchpad.ico') }}" class="icon">
   Launchpad</a>
 {%- 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.
+  </p>
+{%- endmacro %}