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

Fix filter for running jobs in test overview #855

Merged
merged 3 commits into from Sep 14, 2016

Conversation

Martchus
Copy link
Contributor

@Martchus Martchus commented Sep 9, 2016

@coveralls
Copy link

Coverage Status

Coverage remained the same at 71.08% when pulling 718babd on Martchus:testres_filter into 3fe8be9 on os-autoinst:master.

@okurz
Copy link
Member

okurz commented Sep 9, 2016

LGTM

A proper change would be to include a check box row for state

@aaannz
Copy link
Contributor

aaannz commented Sep 9, 2016

I'm too for using state and keep filtering for running.

@Martchus
Copy link
Contributor Author

Included an extra check box row for the state, also added missing results.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 70.741% when pulling 0b08882 on Martchus:testres_filter into 7ea7b0d on os-autoinst:master.

@Martchus
Copy link
Contributor Author

Added another commit for showing results/states dynamically like @okurz proposed.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 70.741% when pulling 960c9d2 on Martchus:testres_filter into 7ea7b0d on os-autoinst:master.

@okurz
Copy link
Member

okurz commented Sep 12, 2016

LGTM

@aaannz
Copy link
Contributor

aaannz commented Sep 13, 2016

Hmm.. Tried it for a while and there are few things that bother me:

  • the overall summary is calculated only for filtered jobs - ie you filter out only scheduled and the title bar is green even thou build overall has some failures
  • there is no other indication filtering is active apart selected checkbox at the bottom of the page. What about add something like 'filtered by state: .. result: ..' to the overall summary bar?
  • I would like to avoid the need to click on apply, but in that case the filter box should be somewhere on the top so it doesn't scroll out of the screen when clicking on state/result to filter for.

@Martchus
Copy link
Contributor Author

  • I decided to put the filters at the bottom of the page because I didn't want use too much space before the actual information.
  • I can add a summary for the current filter at the top of the page. I can also add a link for scrolling at the bottom when we keep the filter box at the bottom.
  • I think having to apply explicitly makes sense. Otherwise every time the user clicks a checkbox the page must reload which is more annoying in my opinion.

@okurz
Copy link
Member

okurz commented Sep 13, 2016

I agree with point 1 and 2 of @aaannz but not point 3 about "apply" with same reasoning as given by @Martchus . This is also what redmine does. That would be different if the checkbox selection would only alter the rendering w/o a new query and computation.

I recommend to accept the PR when it's ok to merge to fix something and continue discussion in a progress ticket for feature requests.

@Martchus I guess we don't have a ticket for this specific case but because you want to create new tickets anyway please also add one for that purpose, include proposals and ideas by you, me and @aaannz and link the ticket to other related ones

@Martchus Martchus changed the title Remove filter for running jobs in test overview Fix filter for running jobs in test overview Sep 13, 2016
@aaannz
Copy link
Contributor

aaannz commented Sep 13, 2016

  • I can add a summary for the current filter at the top of the page. I can also add a link for scrolling at the bottom when we keep the filter box at the bottom.

Please do this and I'll be happy to merge.

@Martchus
Copy link
Contributor Author

I moved the filter to the top, made it expandable/collapsible the currently present filter is displayed.

@aaannz
Copy link
Contributor

aaannz commented Sep 14, 2016

Yep, this looks good. Thanks 👍

@aaannz
Copy link
Contributor

aaannz commented Sep 14, 2016

You still need to adapt tests.

@Martchus
Copy link
Contributor Author

Fixed the tests, but wait with merging because @okurz found another bug.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 70.535% when pulling 05e724f on Martchus:testres_filter into b099201 on os-autoinst:master.

@Martchus
Copy link
Contributor Author

Ok, this should be fixed now, too.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 70.535% when pulling 579d4be on Martchus:testres_filter into b099201 on os-autoinst:master.

@okurz
Copy link
Member

okurz commented Sep 14, 2016

LGTM

@aaannz tests look fine, too, merge it?

@aaannz aaannz merged commit 88233c6 into os-autoinst:master Sep 14, 2016
@Martchus Martchus deleted the testres_filter branch September 14, 2016 11:45
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