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

Highlight available test result comments on overview #514

Merged
merged 8 commits into from
Feb 11, 2016

Conversation

okurz
Copy link
Member

@okurz okurz commented Jan 26, 2016

The availability of comments on the test result page is shown on the test
result overview page next to the status icon and failed modules if available and if selected with the corresponding query parameter "show_comment=1".

The icon only appears if comments are available. This way it should be much
easier for reviewers of test runs to exchange and see if an issue has already
been noticed by someone else.

Be aware that so far the comments are per test result not per test case or
scenario.

Related issue: https://progress.opensuse.org/issues/10212

see screenshot for example with hover info:
openqa_comment_bubble

@okurz okurz force-pushed the feature/label branch 3 times, most recently from 9740792 to 8c9ac90 Compare January 26, 2016 21:52
@@ -533,6 +533,8 @@ table.overview .failedmodule {
.fa.result_incomplete { color: #AF1E11;}
.fa.module_none { color:#bebebe;}

.comment { color: Grey; }
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you want the selector more specific - or the class name itself. comments can be anywhere anything - but you're only meaning the one icon on the overview page.

@okurz okurz changed the title Highlight available test result comments on overview [ON HOLD]Highlight available test result comments on overview Jan 30, 2016
@okurz okurz changed the title [ON HOLD]Highlight available test result comments on overview Highlight available test result comments on overview using query Feb 3, 2016
@okurz
Copy link
Member Author

okurz commented Feb 4, 2016

updated:

  • slightly more specific CSS selector
  • showing comment icon (and therefore if correctly understood only calling expensive database query) only if selected with query parameter "show_comment=1"
  • left out commit for javascript update to deep-link into tabs for now, will do that later

@coolo I did not do "prefetch" as I don't know what this would actually do in this context or how I should do it and also I did not test with a production database dump as I forgot the link you gave me with a dump.

@@ -139,6 +139,14 @@
</span>
% }
% }
% if ($res->{comments} && $res->{comments} > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

do you expect it to be < 0?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, but if it would be, I better not try to dereference the comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

then the > 0 is nonsense

@coolo
Copy link
Contributor

coolo commented Feb 4, 2016

30ba5b1

@okurz
Copy link
Member Author

okurz commented Feb 5, 2016

updated:

  • removed "> 0 nonsense"
  • incorporated job prefetching based on @coolo's proposal but adapted to show only when "&show_comment=1"

@okurz
Copy link
Member Author

okurz commented Feb 8, 2016

+waitfor should the comments bubble always be shown or only when selected with a query parameter?

@coolo
Copy link
Contributor

coolo commented Feb 8, 2016

as job comments are very rare, I'm fine with them being always there in case

The availability of comments on the test result page is shown on the test
result overview page next to the status icon and failed modules if available.

The icon only appears if comments are available. This way it should be much
easier for reviewers of test runs to exchange and see if an issue has already
been noticed by someone else.

Be aware that so far the comments are per test result not per test case or
scenario.

Related issue: https://progress.opensuse.org/issues/10212
Prefetch the job comments instead of relying on custom query.
@okurz
Copy link
Member Author

okurz commented Feb 8, 2016

aww, I messed up, wait for update. Please don't look

Also remembers tab on reload of URLs with hash.
@okurz
Copy link
Member Author

okurz commented Feb 8, 2016

updated:

  • re-add javascript code to allow deep-links into tabs (e.g. comment tab)
  • count available comments by prefetching
  • always display job comments notification if available (not using query param, as suggested by coolo and sysrich)

@okurz okurz changed the title Highlight available test result comments on overview using query Highlight available test result comments on overview Feb 9, 2016
coolo added a commit that referenced this pull request Feb 11, 2016
Highlight available test result comments on overview
@coolo coolo merged commit 9b8b5fd into os-autoinst:master Feb 11, 2016
@okurz okurz deleted the feature/label branch November 13, 2018 13:06
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

2 participants