Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make "last good" a link to a job instead of plain job ID #3261

Closed

Conversation

ilausuch
Copy link
Contributor

@ilausuch ilausuch commented Jul 15, 2020

https://progress.opensuse.org/issues/19720

If a valid last_good job ID is present in the investigation result
set, then it is replaced by html code to link directly to this job.

In order to tell to the JS that this is a piece of HTML, backend adds
the type of this variable. Using ':' to split [:].
In this case the type is html. The key will be now 'last_good:html'

In JS detects if a type has being defined or not. If is html then
use this html. In other cases, now only traditional use, then creates
a < pre > element.

This allows to add typed information like links to files,
tables, but also colorize the values is needed.

Alternative to #3259

@ilausuch ilausuch requested review from okurz and Martchus July 15, 2020 10:56
@ilausuch
Copy link
Contributor Author

In this case, more Perl code is present controlling what to paint and how to paint

@ilausuch
Copy link
Contributor Author

I am not sure that add the html conde in the Perl side is a good idea.
Maybe could be better to replace the variable by an other hash like that

last_good:link = { link: '...', text:'...'}

What do you think?

@Martchus
Copy link
Contributor

I like the last_good:link = { link: '...', text:'...'} idea more. The DOM tree isn't hard to build on the JavaScript-side and this way the rendering isn't a weird mix between server-side and client-side rendering.

@ilausuch
Copy link
Contributor Author

I add a new commit with the new approach

lib/OpenQA/WebAPI/Controller/Test.pm Outdated Show resolved Hide resolved
@@ -2142,4 +2142,3 @@ sub video_file_paths {
}

1;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated whitespace change, please revert

if (textLines.length > lineLimit) {
textLinesRest = textLines.slice(lineLimit, textLines.length);
textLines = textLines.slice(0, lineLimit);
if (type === "link"){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again some style issues. Please add the whitespace between ) and {

https://progress.opensuse.org/issues/19720

If a valid last_good job ID is present in the investigation result
set, then it is replaced by html code <a> to link directly to this job.

In order to tell to the JS that this is a link, backend adds
the type of this variable. Using ':' to split <name>[:<type>].
In this case the type is link. The key will be now 'last_good:link'

In JS detects if a type has being defined or not. If is link then
creates a link with the information. In other cases, now only
traditional use, then creates a <pre> element.

This allows to add typed information like links to files,
tables, but also colorize the values is needed.
@ilausuch ilausuch marked this pull request as ready for review July 20, 2020 09:14
var spl = key.split(':');
var value = response[key];
var type = "pre";

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about documenting the format explicitly?

Suggested change
// Parse optional type specifier, e.g. "last_good:link"

Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add/change a test accordingly.

@okurz
Copy link
Member

okurz commented Jul 20, 2020

we can go with #3272 as Martchus also proposed

@okurz okurz closed this Jul 20, 2020
@ilausuch ilausuch deleted the add_link_to_last_good_perl branch November 5, 2020 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants