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

Proposed by Tim Andersson
Status: Work in progress
Proposed branch: ~andersson123/autopkgtest-cloud:fix-package-page-nonexistent-package
Merge into: autopkgtest-cloud:master
Diff against target: 199 lines (+85/-67)
4 files modified
charms/focal/autopkgtest-web/webcontrol/browse.cgi (+6/-1)
charms/focal/autopkgtest-web/webcontrol/templates/browse-package.html (+25/-20)
charms/focal/autopkgtest-web/webcontrol/templates/browse-results.html (+50/-46)
charms/focal/autopkgtest-web/webcontrol/templates/macros.html (+4/-0)
Reviewer Review Type Date Requested Status
Skia Abstain
Brian Murray Needs Fixing
Review via email: mp+462703@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Skia (hyask) wrote :

Looking good!

review: Approve
Revision history for this message
Brian Murray (brian-murray) wrote :

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
Revision history for this message
Tim Andersson (andersson123) wrote :

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

Revision history for this message
Brian Murray (brian-murray) wrote :

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.

Revision history for this message
Tim Andersson (andersson123) wrote :

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.

Revision history for this message
Tim Andersson (andersson123) wrote :

waiting on feedback for this MP.

Revision history for this message
Tim Andersson (andersson123) wrote :

This one needs more info.

Revision history for this message
Brian Murray (brian-murray) wrote :

> 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.

Revision history for this message
Brian Murray (brian-murray) :
review: Needs Fixing
Revision history for this message
Tim Andersson (andersson123) wrote :

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.

Revision history for this message
Tim Andersson (andersson123) wrote :

amended and ready for review.

Revision history for this message
Skia (hyask) :
review: Abstain
Revision history for this message
Tim Andersson (andersson123) wrote :

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

Unmerged commits

25fdf19... by 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

Succeeded
[SUCCEEDED] pre_commit:0 (build)
[SUCCEEDED] unit_tests:0 (build)
[SUCCEEDED] build_charms:0 (build)
13 of 3 results

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/charms/focal/autopkgtest-web/webcontrol/browse.cgi b/charms/focal/autopkgtest-web/webcontrol/browse.cgi
2index c1b1248..bd7442d 100755
3--- a/charms/focal/autopkgtest-web/webcontrol/browse.cgi
4+++ b/charms/focal/autopkgtest-web/webcontrol/browse.cgi
5@@ -356,7 +356,12 @@ def package_release_arch(package, release, arch, _=None):
6 test_id = get_test_id(release, arch, package)
7 if test_id is None:
8 return render(
9- "browse-error.html", error="Package does not exist", code=404
10+ "browse-results.html",
11+ package=package,
12+ release=release,
13+ arch=arch,
14+ package_results=[],
15+ title_suffix="- %s/%s/%s" % (package, release, arch),
16 )
17
18 seen = set()
19diff --git a/charms/focal/autopkgtest-web/webcontrol/templates/browse-package.html b/charms/focal/autopkgtest-web/webcontrol/templates/browse-package.html
20index 165cfd8..d636f73 100644
21--- a/charms/focal/autopkgtest-web/webcontrol/templates/browse-package.html
22+++ b/charms/focal/autopkgtest-web/webcontrol/templates/browse-package.html
23@@ -12,29 +12,34 @@
24 </li>
25 </ul>
26
27- <table class="table" style='width: auto'>
28- <tr>
29- <th />
30- {% for r in releases %}<th>{{r}}</th> {% endfor %}
31- </tr>
32-
33- {% for a in arches %}
34+ {% if results %}
35+ <table class="table" style='width: auto'>
36 <tr>
37- <th>{{a}}</th>
38- {% for r in releases %}
39- <td class="{{ results[r][a] }}"><a href="{{package}}/{{r}}/{{a}}">{{ results[r][a] }}</a></td>
40- {% endfor %}
41+ <th />
42+ {% for r in releases %}<th>{{r}}</th> {% endfor %}
43 </tr>
44- {% endfor %}
45- </table>
46
47- <hr>
48- <h3>Running tests</h3>
49+ {% for a in arches %}
50+ <tr>
51+ <th>{{a}}</th>
52+ {% for r in releases %}
53+ <td class="{{ results[r][a] }}"><a href="{{package}}/{{r}}/{{a}}">{{ results[r][a] }}</a></td>
54+ {% endfor %}
55+ </tr>
56+ {% endfor %}
57+ </table>
58+
59+ <hr>
60+ <h3>Running tests</h3>
61+
62+ {% for p, info in running.items()|sort %}
63+ {{ macros.display_running_job(p, info) }}
64+ {% endfor %}
65
66- {% for p, info in running.items()|sort %}
67- {{ macros.display_running_job(p, info) }}
68- {% endfor %}
69+ <h3>Queued tests</h3>
70+ {{ macros.display_queues_info(queues_info) }}
71+ {% else %}
72+ {{ macros.nonexistent_package() }}
73+ {% endif %}
74
75- <h3>Queued tests</h3>
76- {{ macros.display_queues_info(queues_info) }}
77 {% endblock %}
78diff --git a/charms/focal/autopkgtest-web/webcontrol/templates/browse-results.html b/charms/focal/autopkgtest-web/webcontrol/templates/browse-results.html
79index fadff6d..7675621 100644
80--- a/charms/focal/autopkgtest-web/webcontrol/templates/browse-results.html
81+++ b/charms/focal/autopkgtest-web/webcontrol/templates/browse-results.html
82@@ -12,55 +12,59 @@
83 </li>
84 </ul>
85
86- <table class="table">
87- <tr>
88- <td><b>Version</b></td>
89- <td><b>Triggers</b></td>
90- <td><b>Env</b></td>
91- <td><b>Date</b></td>
92- <td><b>Duration</b></td>
93- <td><b>Requester</b></td>
94- <td><b>Result</b></td>
95- <td><b>UUID</b></td>
96- <td></td>
97- </tr>
98+ {% if package_results %}
99+ <table class="table">
100+ <tr>
101+ <td><b>Version</b></td>
102+ <td><b>Triggers</b></td>
103+ <td><b>Env</b></td>
104+ <td><b>Date</b></td>
105+ <td><b>Duration</b></td>
106+ <td><b>Requester</b></td>
107+ <td><b>Result</b></td>
108+ <td><b>UUID</b></td>
109+ <td></td>
110+ </tr>
111
112- {% for row in package_results %}
113- <tr {% if row[6] in ["running", "queued"] %}class="unfinished"{% endif %}>
114- <td>{{row[0]}}</td>
115- <td>{{row[1]}}</td>
116- <td>{{row[2]}}</td>
117- <td>{{row[3]}}</td>
118- <td>{{row[4]}}</td>
119- <td>
120- {% if row[5] != "-" %}
121- <a href="https://launchpad.net/~{{row[5]}}">{{row[5]}}</a>
122- {% else %}
123- {{row[5]}}
124- {% endif %}
125- </td>
126- <td class="nowrap {{row[6]}}" title={{ row[6] }}>{{row[6]}}</td>
127- <td>{{row[10]}}</td>
128- <td class="nowrap">
129- {% if row[6] not in ["running", "queued"] %}
130- <a href="{{row[7]}}/log.gz">log</a> &emsp;
131- <a href="{{row[7]}}/artifacts.tar.gz">artifacts</a> &emsp;
132- {% endif %}
133- </td>
134- <td class="nowrap">
135- {% if row[8] %}
136- {% set ts = row[1].split()|map('urlencode')|join("&trigger=")|safe %}
137- {% set all_proposed = row[9] %}
138- {% if all_proposed %}
139- <a href="{{base_url}}request.cgi?release={{release}}&arch={{arch}}&package={{package|urlencode}}&trigger={{ts}}&all-proposed=1">&#9851;</a>
140+ {% for row in package_results %}
141+ <tr {% if row[6] in ["running", "queued"] %}class="unfinished"{% endif %}>
142+ <td>{{row[0]}}</td>
143+ <td>{{row[1]}}</td>
144+ <td>{{row[2]}}</td>
145+ <td>{{row[3]}}</td>
146+ <td>{{row[4]}}</td>
147+ <td>
148+ {% if row[5] != "-" %}
149+ <a href="https://launchpad.net/~{{row[5]}}">{{row[5]}}</a>
150 {% else %}
151- <a href="{{base_url}}request.cgi?release={{release}}&arch={{arch}}&package={{package|urlencode}}&trigger={{ts}}">&#9851;</a>
152+ {{row[5]}}
153 {% endif %}
154- {% endif %}
155- </td>
156- </tr>
157- {% endfor %}
158- </table>
159+ </td>
160+ <td class="nowrap {{row[6]}}" title={{ row[6] }}>{{row[6]}}</td>
161+ <td>{{row[10]}}</td>
162+ <td class="nowrap">
163+ {% if row[6] not in ["running", "queued"] %}
164+ <a href="{{row[7]}}/log.gz">log</a> &emsp;
165+ <a href="{{row[7]}}/artifacts.tar.gz">artifacts</a> &emsp;
166+ {% endif %}
167+ </td>
168+ <td class="nowrap">
169+ {% if row[8] %}
170+ {% set ts = row[1].split()|map('urlencode')|join("&trigger=")|safe %}
171+ {% set all_proposed = row[9] %}
172+ {% if all_proposed %}
173+ <a href="{{base_url}}request.cgi?release={{release}}&arch={{arch}}&package={{package|urlencode}}&trigger={{ts}}&all-proposed=1">&#9851;</a>
174+ {% else %}
175+ <a href="{{base_url}}request.cgi?release={{release}}&arch={{arch}}&package={{package|urlencode}}&trigger={{ts}}">&#9851;</a>
176+ {% endif %}
177+ {% endif %}
178+ </td>
179+ </tr>
180+ {% endfor %}
181+ </table>
182+ {% else %}
183+ {{ macros.nonexistent_package() }}
184+ {% endif %}
185
186 <p>
187 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>.
188diff --git a/charms/focal/autopkgtest-web/webcontrol/templates/macros.html b/charms/focal/autopkgtest-web/webcontrol/templates/macros.html
189index 941dc77..8240082 100644
190--- a/charms/focal/autopkgtest-web/webcontrol/templates/macros.html
191+++ b/charms/focal/autopkgtest-web/webcontrol/templates/macros.html
192@@ -62,3 +62,7 @@
193 {% endif %}
194 <a href="https://launchpad.net/ubuntu/{{ release }}+source/{{ package_name }}"><img src="{{ url_for('static', filename='launchpad.ico') }}" class="icon">Launchpad</a>
195 {%- endmacro %}
196+
197+{% macro nonexistent_package() -%}
198+<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>
199+{%- endmacro %}

Subscribers

People subscribed via source and target branches