Tco 19 snapshot dashboard #109
Conversation
JPrevost
requested review from
jazairi and
matt-bernhardt
and removed request for
jazairi
There was a problem hiding this comment.
I have one non-blocking question about the structure of the ability model, and one head-shaking comment about what is necessary to actually grant permission to a resource. I'm fine with this merging as-is, though - thanks for putting this together to let folks see the sorts of metrics we're compiling.
| @@ -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 | |||
There was a problem hiding this comment.
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.
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.
| # all authenticated | ||
| # can :view, :playground | ||
| can :manage, :detector__suggested_resource | ||
| can :manage, Detector::SuggestedResource |
There was a problem hiding this comment.
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.
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.
Why are these changes being introduced: * Allowing a way to access our historical matches over time will be useful Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/TCO-63 * https://mitlibraries.atlassian.net/browse/TCO-19 How does this address that need: * Adds Report index * Adds Metrics Algorithm report with monthly and aggregate support Document any side effects to this change: * Updates ability.rb for SuggestedResources to handle the difference in syntax when working in Administrate context versus the rest of our app
JPrevost
force-pushed
the
tco-19-snapshot-dashboard
branch
from
6e6c915
to
f131051
Compare
Summary of changes (please refer to commit messages for full details)
Developer
Ticket(s)
https://mitlibraries.atlassian.net/browse/TCO-###
Accessibility
all issues introduced by these changes have been resolved or opened
as new issues (link to those issues in the Pull Request details above)
Documentation
ENV
Stakeholders
Dependencies and migrations
NO dependencies are updated
NO migrations are included
Reviewer
Code
added technical debt.
Documentation
(not just this pull request message).
Testing