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

Tco 19 snapshot dashboard #109

Merged
merged 4 commits into from
Oct 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions app/controllers/report_controller.rb
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
18 changes: 18 additions & 0 deletions app/helpers/metrics_helper.rb
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
12 changes: 9 additions & 3 deletions app/models/ability.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

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?

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 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 the def method_name. This feels in line with that expectation that docs come before the thing they are documenting.

return if user.blank?
# Rules will go here.

# all authenticated
# can :view, :playground
can :manage, :detector__suggested_resource
can :manage, Detector::SuggestedResource
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 Ability. I wasn't sure how best to document it either.


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
Expand Down
3 changes: 3 additions & 0 deletions app/models/metrics/algorithms.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ module Metrics
class Algorithms < ApplicationRecord
self.table_name = 'metrics_algorithms'

scope :monthlies, -> { where.not(month: nil).order(month: :asc) }
scope :aggregates, -> { where(month: nil).order(month: :asc) }

# generate metrics data about SearchEvents matches
#
# @note This is expected to only be run once per month per type of aggregation (once with no month supplied, once
Expand Down
27 changes: 27 additions & 0 deletions app/views/admin/application/_navigation.html.erb
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>
12 changes: 9 additions & 3 deletions app/views/layouts/_site_nav.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,12 @@
<% if can? :view, :playground %>
<%= link_to('Playground', '/playground', class: 'nav-item') %>
<% end %>
<% if can? :manage, :detector__suggested_resource %>
<%= link_to('Suggested Resources', admin_detector_suggested_resources_path, class: 'nav-item') %>
<% end %>
<% if can? :view, :report %>
<%= link_to('Reports', report_path, class: 'nav-item') %>
<% end %>
<% end %>
</nav>
<nav class="nav-user" aria-label="User menu">
Expand All @@ -24,11 +30,11 @@
method: :delete) %>
<% else %>
<% if FakeAuthConfig.fake_auth_enabled? %>
<%= button_to("Sign in", user_developer_omniauth_authorize_path, id: "sign_in", class: 'action-auth',
method: :post) %>
<%= button_to("Sign in", user_developer_omniauth_authorize_path, id: "sign_in", class: 'action-auth',
data: { turbo:false }, method: :post) %>
<% else %>
<%= button_to("Sign in", user_openid_connect_omniauth_authorize_path, class: 'action-auth', id: "sign_in",
method: :post) %>
data: { turbo:false }, method: :post) %>
<% end %>
<% end %>
</nav>
Expand Down
46 changes: 46 additions & 0 deletions app/views/report/algorithm_metrics.html.erb
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>
6 changes: 6 additions & 0 deletions app/views/report/index.html.erb
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>
2 changes: 2 additions & 0 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@
get 'up' => 'rails/health#show', as: :rails_health_check

get 'playground', to: 'static#playground'
get '/report', to: 'report#index'
get '/report/algorithm_metrics', to: 'report#algorithm_metrics'

# Defines the root path route ("/")
root to: 'static#index'
Expand Down
69 changes: 69 additions & 0 deletions test/controllers/report_controller_test.rb
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
29 changes: 29 additions & 0 deletions test/helpers/metrics_helper_test.rb
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