-
Notifications
You must be signed in to change notification settings - Fork 0
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
Tco 19 snapshot dashboard #109
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
# frozen_string_literal: true | ||
|
||
class ReportController < ApplicationController | ||
def index; end | ||
|
||
def algorithm_metrics | ||
@metrics = if params[:type] == 'aggregate' | ||
Metrics::Algorithms.aggregates | ||
else | ||
Metrics::Algorithms.monthlies | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
# frozen_string_literal: true | ||
|
||
module MetricsHelper | ||
# Calculate percentage of search events in which we had any Detection match | ||
def percent_match(metric) | ||
(sum_matched(metric) / sum_total(metric) * 100).round(2) | ||
end | ||
|
||
# Sums all detection matches for a given Metrics::Algorithms record | ||
def sum_matched(metric) | ||
metric.doi.to_f + metric.issn.to_f + metric.isbn.to_f + metric.pmid.to_f + metric.journal_exact.to_f + metric.suggested_resource_exact.to_f | ||
end | ||
|
||
# Calculates total events for a given Metrics::Algorithms record | ||
def sum_total(metric) | ||
sum_matched(metric) + metric.unmatched.to_f | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,12 +7,18 @@ class Ability | |
# See the wiki for details: | ||
# https://github.com/CanCanCommunity/cancancan/blob/develop/docs/define_check_abilities.md | ||
def initialize(user) | ||
# Actions allowed for non-authenticated Users should add `skip_before_action :require_user` | ||
|
||
# Start of Rules for all authenticated user with no additional roles required | ||
return if user.blank? | ||
# Rules will go here. | ||
|
||
# all authenticated | ||
# can :view, :playground | ||
can :manage, :detector__suggested_resource | ||
can :manage, Detector::SuggestedResource | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Am I seeing this correctly? Both of these lines appear to be required, for different reasons? One grants access to the resource, while the other is needed to get access to the dashboard? If it works, it works, but ... huh. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's complicated. I spent some time trying to understand how to avoid this, but it felt like it was going to require additional work in a possibly less maintainable way than just doubling up things in |
||
|
||
can :view, :report | ||
# End of Rules for all authenticated user with no additional roles required | ||
|
||
# Rules for admins | ||
return unless user.admin? | ||
|
||
can :manage, :all | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
<%# | ||
# Navigation | ||
|
||
This partial is used to display the navigation in Administrate. | ||
By default, the navigation contains navigation links | ||
for all resources in the admin dashboard, | ||
as defined by the routes in the `admin/` namespace | ||
|
||
This local template was forked from the administrate version to allow direct usage of | ||
the `can?` check rather than administrates `accessible_action?` which didn't work in | ||
our environment. Our option was to fork this template or monkey patch that method. This | ||
felt least likely to have undesired side effects as `accessible_action?` is used in more | ||
places than just this and we haven't noted it not working as expected there. If we realize | ||
other templates are also having it not work as needed, we should monkey patch that method | ||
and delete this template. | ||
%> | ||
|
||
<nav class="navigation"> | ||
<%= link_to(t("administrate.navigation.back_to_app"), root_url, class: "button button--alt button--nav") if defined?(root_url) %> | ||
<% Administrate::Namespace.new(namespace).resources_with_index_route.each do |resource| %> | ||
<%= link_to( | ||
display_resource_name(resource), | ||
resource_index_route(resource), | ||
class: "navigation__link navigation__link--#{nav_link_state(resource)}" | ||
) if can?(:index, model_from_resource(resource)) %> | ||
<% end %> | ||
</nav> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
<div class="alert alert-banner warning"> | ||
<p><i class="fa fa-exclamation-circle fa-lg"></i> Percentage match is not accurate for Terms that match multiple algorithms. The actual percentage match will be lower than the reported value. At this time, the error is not believed to be significant based on the currently low volume of Terms that match multiple algorithms. This may change as we develop new algorithms to a point where we need to address this discrepancy.</p> | ||
</div> | ||
|
||
<% if params[:type] == 'aggregate' %> | ||
<h3>Aggregate Algorithm Metrics</h3> | ||
<% else %> | ||
<h3>Monthly Algorithm Metrics</h3> | ||
<% end %> | ||
|
||
<table class="table"> | ||
<tr> | ||
<% if params[:type] == 'aggregate' %> | ||
<th>Aggregation date</th> | ||
<% else %> | ||
<th>Month</th> | ||
<% end %> | ||
<th>DOI</th> | ||
<th>ISSN</th> | ||
<th>ISBN</th> | ||
<th>PMID</th> | ||
<th>Journal</th> | ||
<th>SuggestedResource</th> | ||
<th>Unmatched</th> | ||
<th>% matched</th> | ||
</tr> | ||
<% @metrics.each do |metric| %> | ||
<tr> | ||
<% if params[:type] == 'aggregate' %> | ||
<td><%= metric.updated_at.strftime("%d %B %Y") %></td> | ||
<% else %> | ||
<td><%= metric.month.strftime("%B %Y") %></td> | ||
<% end %> | ||
<td><%= metric.doi %></td> | ||
<td><%= metric.issn %></td> | ||
<td><%= metric.isbn %></td> | ||
<td><%= metric.pmid %></td> | ||
<td><%= metric.journal_exact %></td> | ||
<td><%= metric.suggested_resource_exact %></td> | ||
<td><%= metric.unmatched %></td> | ||
<td> | ||
<%= percent_match(metric) %> | ||
</td> | ||
</tr> | ||
<% end %> | ||
</table> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
<h3>Report index</h3> | ||
|
||
<ul> | ||
<li><%= link_to("Monthly algorithm metrics", report_algorithm_metrics_path) %></li> | ||
<li><%= link_to("Aggregate algorithm metrics", report_algorithm_metrics_path(type: 'aggregate') ) %></li> | ||
</ul> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
require 'test_helper' | ||
|
||
class ReportControllerTest < ActionDispatch::IntegrationTest | ||
test 'report index is not accessible without authentication' do | ||
get '/report' | ||
|
||
assert_redirected_to '/' | ||
follow_redirect! | ||
|
||
assert_select 'div.alert', text: 'Please sign in to continue', count: 1 | ||
end | ||
|
||
test 'report index is accessible to admins when authenticated' do | ||
sign_in users(:admin) | ||
|
||
get '/report' | ||
|
||
assert_response :success | ||
end | ||
|
||
test 'report index url is accessible to basic users when authenticated' do | ||
sign_in users(:basic) | ||
|
||
get '/report' | ||
|
||
assert_response :success | ||
end | ||
|
||
test 'algorithm metrics report is not accessible without authentication' do | ||
get '/report/algorithm_metrics' | ||
|
||
assert_redirected_to '/' | ||
follow_redirect! | ||
|
||
assert_select 'div.alert', text: 'Please sign in to continue', count: 1 | ||
end | ||
|
||
test 'algorithm metrics report is accessible to admins when authenticated' do | ||
sign_in users(:admin) | ||
|
||
get '/report/algorithm_metrics' | ||
|
||
assert_response :success | ||
end | ||
|
||
test 'algorithm metrics report is accessible to basic users when authenticated' do | ||
sign_in users(:basic) | ||
|
||
get '/report/algorithm_metrics' | ||
|
||
assert_response :success | ||
end | ||
|
||
test 'algorithm metrics can show monthly data' do | ||
sign_in users(:basic) | ||
|
||
get '/report/algorithm_metrics' | ||
|
||
assert_select 'h3', text: 'Monthly Algorithm Metrics', count: 1 | ||
end | ||
|
||
test 'algorithm metrics can show aggregate data' do | ||
sign_in users(:basic) | ||
|
||
get '/report/algorithm_metrics?type=aggregate' | ||
|
||
assert_select 'h3', text: 'Aggregate Algorithm Metrics', count: 1 | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
require 'test_helper' | ||
|
||
class MetricsHelperTest < ActionView::TestCase | ||
def metric | ||
Metrics::Algorithms.new( | ||
month: DateTime.now - 1.month, | ||
doi: 1, | ||
issn: 2, | ||
isbn: 3, | ||
pmid: 4, | ||
unmatched: 79, | ||
journal_exact: 5, | ||
suggested_resource_exact: 6, | ||
created_at: DateTime.now, | ||
updated_at: DateTime.now | ||
) | ||
end | ||
test 'can calculate a percentage of matches for a record' do | ||
assert_in_delta(21.0, percent_match(metric), 0.01) | ||
end | ||
|
||
test 'can calculate sum of matches for a record' do | ||
assert_in_delta(21.0, sum_matched(metric), 0.01) | ||
end | ||
|
||
test 'can calculate sum of all events for a record' do | ||
assert_in_delta(100.0, sum_total(metric), 0.01) | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be a minor point, so please ignore if you feel strongly...
I really like calling out the block of permissions like this - but it feels like the
return if user.blank?
line should come before this block starts?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like the common ruby documentation practice is to document before the thing being documented. i.e. you document the Class before the
Class class_name
definition, you document the method before thedef method_name
. This feels in line with that expectation that docs come before the thing they are documenting.