fixes #15919 statistics page is now loaded via AJAX #3691
Conversation
theforeman-bot
added
Needs testing
Waiting on contributor
and removed
Waiting on contributor
labels
theforeman-bot
added
Needs testing
Waiting on contributor
and removed
Waiting on contributor
labels
|
test failures seems unrelated. |
ohadlevy
force-pushed
the
statistics
branch
3 times, most recently
from
037eb3e
to
e282076
Compare
|
@tbrisker fixed rubocop. |
|
rebased |
|
[test] |
|
I'm not sure how this relates to #3438 or if it's worth thinking about? |
|
@sean797 - nice - I missed that PR. I could see how the two can integrate, but in that case I would also like to discuss dashboard widgets (e.g. what is the different between a widget and statistics model?) I believe my patch will simplify your patch (e.g. in terms of statistics chart type - host counter, fact counter etc). |
|
tests are passing, its just katello job which is unstable atm. |
|
Seems like somewhere some permissions are missing, when trying to view as non-admin this is what it looks like: |
| <% @stats.each do |stat| %> | ||
| <div data-ajax-url=<%= stat.url %> class='stats-well' data-on-complete='refreshCharts'> | ||
| <div class='statistics-pie-placeholder'> | ||
| <h4 class='ca pie-title'> <span class='spinner spinner-xl spinner-inline'> </span> <%= stat.title %> </h4> |
There was a problem hiding this comment.
There is no spinner-xl, the largest is spinner-lg. Also, looks strange to have the spinner inline before the title, maybe move it down to where the chart should be?
There was a problem hiding this comment.
| CountHosts.new(:count_by => :operatingsystem, :title => ("OS Distribution"), :search =>"os_title=~VAL~"), | ||
| CountHosts.new(:count_by => :architecture, :title => _("Architecture Distribution"), :search => "facts.architecture=~VAL~"), | ||
| CountHosts.new(:count_by => :environment, :title => _("Environment Distribution"), :search => "environment=~VAL~"), | ||
| CountFacts.new(:count_by => 'processorcount', :unit => Nn_('%s core', '%s cores'), :title => "Number of CPUs", :search => "facts.processorcount=~VAL1~"), |
There was a problem hiding this comment.
why are some count_by strings and some symbols?
|
@ohadlevy Thanks, looks good in general, just a few minor comments and one permissions issue. |
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
|
@tbrisker fixed all of your comments beside the spinner location, looking for suggestions on this one. |
ohadlevy
force-pushed
the
statistics
branch
2 times, most recently
from
ca70ed0
to
333a688
Compare
|
@tbrisker I can't reproduce your issue with permissions, can you provide a trace? |
ohadlevy
force-pushed
the
statistics
branch
3 times, most recently
from
8bbf176
to
007f47f
Compare
Also: - refactor how statistics data is being called, now it should be trivial to add additional charts or consume other data from it. It should make it much easier to reuse statistics charts in other places (e.g. dashboard) or unify how fact charts are done. - remove unused JSON response from the statistics controller.
| end | ||
|
|
||
| def id | ||
| @id || count_by.to_s |
There was a problem hiding this comment.
why not move this into the initializer? also, why not leave it as a symbol?
There was a problem hiding this comment.
some of the queries required string (e.g. facts) instead of changing that and everywhere that uses it, I chose to change it here.
No description provided.