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

Alternative to #1040 based on #1094 #1119

Conversation

okurz
Copy link
Member

@okurz okurz commented Jan 2, 2017

  • Add help popover for '/tests/overview'
  • Add defaults for all job attributes used for sorting
  • Allow selecting multiple job groups in overview query
  • Delete unused variable '$failedid' from overview template
  • Use word wrap for title in test suite popover
  • Add test suite description to overview page
  • Get rid of redundant @configs array in favor of keys

@codecov-io
Copy link

codecov-io commented Jan 2, 2017

Current coverage is 68.40% (diff: 100%)

Merging #1119 into master will increase coverage by 0.05%

@@             master      #1119   diff @@
==========================================
  Files           105        105          
  Lines          7013       7019     +6   
  Methods           0          0          
  Messages          0          0          
  Branches       1212       1210     -2   
==========================================
+ Hits           4793       4801     +8   
  Misses         1830       1830          
+ Partials        390        388     -2   

Powered by Codecov. Last update 38575ee...705df4b

@okurz okurz changed the title Alternative to https://github.com/os-autoinst/openQA/pull/1040 based on https://github.com/os-autoinst/openQA/pull/1094 Alternative to #1040 based on #1094 Jan 3, 2017
my $test = $job->TEST;
my $flavor = $job->FLAVOR || 'sweet';
my $arch = $job->ARCH || 'noarch';
my $distri = $job->DISTRI || 'nodistri';
Copy link
Contributor

Choose a reason for hiding this comment

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

this is the wrong way around. we need to make these fields required instead of adding more hacks

Copy link
Contributor

Choose a reason for hiding this comment

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

In schema jobs->DISTRI is nullable, but does it even make sense? Shouldn't schema require it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it should. But you need to add a script to set 'nodistri' to those currently being NULL. There are some :(

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess the discussion should be continued in https://progress.opensuse.org/issues/6762

Copy link
Contributor

Choose a reason for hiding this comment

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

that is about BUILD. DISTRI, VERSION, ARCH and FLAVOR are mandatory. We just don't check very carefully. That's why I am saying this is the wrong way around.

my $flavor = $job->FLAVOR || 'sweet';
my $arch = $job->ARCH || 'noarch';
my $test = $job->TEST;
my $distri = $job->DISTRI || 'nodistri';
Copy link
Contributor

Choose a reason for hiding this comment

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

same here. DISTRI VERSION FLAVOR ARCH are required fields - and we at least discussed also to include BUILD as required.

@@ -1,6 +1,7 @@
% layout 'bootstrap';
% title 'Test summary';
% use OpenQA::Schema::Result::Jobs;
% use OpenQA::Utils;
Copy link
Contributor

Choose a reason for hiding this comment

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

if you need openQA::Utils in a template it's a clear indicator that you put a render helper in the wrong file

@coolo
Copy link
Contributor

coolo commented Jan 3, 2017

Can these popups please have a proper close button? I find it very hard to get rid of them

@okurz
Copy link
Member Author

okurz commented Jan 3, 2017

I assume the popover's are not meant to have a close button, e.g. compare to the example in http://www.w3schools.com/bootstrap/bootstrap_popover.asp. I will take a look if I can tweak the trigger

@coolo
Copy link
Contributor

coolo commented Jan 3, 2017

but other than that I didn't find any problems beside my inline comments

@coolo
Copy link
Contributor

coolo commented Jan 3, 2017

but you can now rebase. The problematic commits are in here

This alters previous behaviour where additional 'groupid' parameters would
overwrite previous ones. Now a logical 'or' operation is done on the job
group ids as well as names to show jobs from all job groups. Jobs are still
filtered to show only the latest ones over all job groups. Still this should
allow to show more jobs of one build when multiple job groups are involved.
Later on this can be extended to not require a version/distri/build.
Icon is in the filter box title bar.
@okurz okurz force-pushed the feature/overview_multiple_groups_based_on_test_suite_description branch from 981d1df to 37bd8f8 Compare January 3, 2017 12:23
@okurz
Copy link
Member Author

okurz commented Jan 3, 2017

updated:

  • rebased
  • change the popover behaviour from "click" to "hover" with firefox workaround to close other popovers
  • delete commit adding fallback defaults for variables considered mandatory

@okurz okurz force-pushed the feature/overview_multiple_groups_based_on_test_suite_description branch from 37bd8f8 to 9645a26 Compare January 3, 2017 13:59
@coolo coolo merged commit 7373049 into os-autoinst:master Jan 3, 2017
@okurz okurz deleted the feature/overview_multiple_groups_based_on_test_suite_description branch January 3, 2017 15:18
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