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.

Sign up for GitHub

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

Jump to bottom

Add administrate gem ... #78

Merged
merged 1 commit into from
Aug 9, 2024
Merged

Add administrate gem ... #78

merged 1 commit into from
Aug 9, 2024

Conversation

matt-bernhardt
Copy link
Member

@matt-bernhardt matt-bernhardt commented Aug 7, 2024 •
edited
Loading

This adds the Administrate gem, and uses the built-in generator for dashboards for four existing resources:

  • Detector::SuggestedResources
  • SearchEvents
  • Terms
  • Users

Notable changes to the generator are:

  1. The admin application generator gets a require_user method in addition to authorize_user, to ensure relevant error messages in all cases (confirmed with tests)
  2. The Suggested Resources edit form has the fingerprint field removed, because that is a calculated field - so the user shouldn't have the opportunity to change it.

There is also a conditional link in the navigation, shown only to users who have the ability to access the default dashboard (currently set to the Terms area, but will change in a future ticket). This is a divergence from the pattern we're using in ETD, where the nav link is conditional on the admin flag on the User model (which I think is a pattern to avoid).

In hindsight, I think this PR would have been cleaner to have one commit of the generator output, and then a second commit to make the customization changes. If the reviewer would rather see this structure, let me know and I'll restructure accordingly.

Developer

Ticket(s)

https://mitlibraries.atlassian.net/browse/TCO-51

Accessibility

  • ANDI or Wave has been run in accordance to our guide and
    all issues introduced by these changes have been resolved or opened
    as new issues (link to those issues in the Pull Request details above)
  • There are no accessibility implications to this change

Documentation

  • Project documentation has been updated
  • No documentation changes are needed

ENV

  • All new ENV is documented in README.
  • All new ENV has been added to Heroku Pipeline, Staging and Prod.
  • ENV has not changed.

Dependencies and migrations

YES dependencies are updated
NO migrations are included

Reviewer

Code

  • I have confirmed that the code works as intended.
  • Any CodeClimate issues have been fixed or confirmed as
    added technical debt.

Documentation

  • The commit message is clear and follows our guidelines
    (not just this pull request message).
  • The documentation has been updated or is unnecessary.
  • New dependencies are appropriate or there were no changes.

Testing

  • There are appropriate tests covering any new functionality.
  • No additional test coverage is required.

@mitlib mitlib temporarily deployed to tacos-api-pipeline-pr-78 August 7, 2024 18:57 Inactive
@mitlib mitlib temporarily deployed to tacos-api-pipeline-pr-78 August 7, 2024 19:54 Inactive
@mitlib mitlib temporarily deployed to tacos-api-pipeline-pr-78 August 8, 2024 15:28 Inactive
@mitlib mitlib temporarily deployed to tacos-api-pipeline-pr-78 August 8, 2024 16:02 Inactive
@mitlib mitlib temporarily deployed to tacos-api-pipeline-pr-78 August 8, 2024 19:47 Inactive
@mitlib mitlib temporarily deployed to tacos-api-pipeline-pr-78 August 8, 2024 19:49 Inactive
@mitlib mitlib temporarily deployed to tacos-api-pipeline-pr-78 August 8, 2024 20:51 Inactive
@mitlib mitlib temporarily deployed to tacos-api-pipeline-pr-78 August 8, 2024 21:02 Inactive
@matt-bernhardt
Adds basic administration area with tests
1bab3fa 
** Why are these changes being introduced:

* We want to add the Administrate gem to the application, and use its
  dashboards to manage information within TACOS.

** Relevant ticket(s):

* https://mitlibraries.atlassian.net/browse/TCO-51

** How does this address that need:

This does several things:

* Adds the Administrate gem

* Builds the default dashboards for several resources already in the
  application: Suggested Resources, Search Events, Terms, and Users

* Adds a nav link to the admin area, visible only to users with the
  rights to see the Terms dashboard (the default display)

* Starts to define the authorization checks in the admin application
  controller, which should in turn look to CanCan's ability model. The
  ability model itself is so far unchanged.

** Document any side effects to this change:

* The only change to the generated dashboards was to remove the
  fingerprint field from the Suggested Resource form, because that
  value is calculated during save.

* The fact that the Terms dashboard is the default display is a bit
  arbitrary, and a future ticket will expand our approach here to
  potentially include a "lobby" display for users with various rights.
@mitlib mitlib temporarily deployed to tacos-api-pipeline-pr-78 August 8, 2024 21:25 Inactive
@matt-bernhardt matt-bernhardt marked this pull request as ready for review August 9, 2024 13:32
jazairi
jazairi approved these changes Aug 9, 2024
View reviewed changes
Copy link
Contributor

@jazairi jazairi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for setting this all up! Everything looks right to me.

This also feels like a good place to share something I learned while researching the issue we were having yesterday with the 'lobby' view. It seems likely that the issue is not with administrate, but with using CanCanCan without a model. If I'm correct about that, then we need to do two things to get authorization to work properly with the Stat/StatDashboard resource, as suggested here:

  • In the controller, specify that there is no associated model: authorize_resource class: false
  • In the Ability model, name the resource as a symbol: can :read, :stat

I tried the above yesterday and it didn't work for me. It was at the end of the day, though, so it might be worth digging into with fresh eyes. (I suspect I was doing something wrong with pluralization.)

app/controllers/admin/application_controller.rb

# Override this value to specify the number of elements to display at a time
# on index pages. Defaults to 20.
# def records_per_page
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this method was generated by the administrate install? I'm fine keeping it in; just want to make sure it wasn't a leftover comment from a WIP.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this was what the generator put in place - I remember noting it while working, and thinking "huh, I wonder if we'll ever actually use that".

@matt-bernhardt
Copy link
Member Author

Ooh, interesting! Thanks for finding this. I've added it as a reference in the follow-up ticket: https://mitlibraries.atlassian.net/browse/TCO-63

@matt-bernhardt matt-bernhardt merged commit fe7ac05 into main Aug 9, 2024
2 checks passed
@matt-bernhardt matt-bernhardt deleted the tco51-administrate branch August 9, 2024 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jazairi jazairi jazairi approved these changes

@JPrevost JPrevost Awaiting requested review from JPrevost

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@matt-bernhardt @jazairi @mitlib