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

SingleUseLinksViewerController current_ability behavior fixes #6739

Merged
merged 2 commits into from
Mar 28, 2024

Conversation

bbpennel
Copy link
Contributor

@bbpennel bbpennel commented Mar 12, 2024

Summary

This PR adds initially failing tests for current_ability in SingleUseLinksViewerController to demonstrate the issue, and then makes it so the current_ability can be retrieved when a SingleUseLink has already been consumed and adds current_user to it.

There is also a full backtrace logged at ERROR level every time a stale SingleUseLink is accessed that I wouldn't mind reducing to a short warning, which I can add to the PR if the reviewer thinks that's a good idea, but it seems intentional currently.

Guidance for testing, such as acceptance criteria or new user interface behaviors:

To replicate the issue in dassie, on main temporarily edit _footer.html.erb to add <%= current_ability.current_user %> anywhere in it. Then create a single use link and access it twice. On the second access it will produce the following error page:

Couldn't find SingleUseLink

activerecord (6.1.7.6) lib/active_record/core.rb:399:in `find_by!'
activerecord (6.1.7.6) lib/active_record/dynamic_matchers.rb:66:in `find_by_download_key!'
/app/samvera/hyrax-engine/app/controllers/hyrax/single_use_links_viewer_controller.rb:75:in `single_use_link'
/app/samvera/hyrax-engine/app/controllers/hyrax/single_use_links_viewer_controller.rb:93:in `current_ability'
cancancan (3.5.0) lib/cancan/controller_additions.rb:298:in `current_ability'
/app/samvera/hyrax-engine/app/views/shared/_footer.html.erb:14 

Then check out this branch and access the link again, and it should give the Single Use Link Expired or Not Found page instead. (unrelated to this issue, but the page is displaying html with a link to a help page doesn't exist)

Type of change (for release notes)

notes-bugfix

Detailed Description

This issue isn't readily apparent in hyrax itself, but in a local application that modifies parts of the template (in our case the footer) to make use of current_ability, it causes the page to error out and log a FATAL exception in addition to a more expected ERROR from the controller itself.

In both cases, the root of the error is:
ActionView::Template::Error (Couldn't find SingleUseLink)

Which originates from:
https://github.com/samvera/hyrax/blob/main/app/controllers/hyrax/single_use_links_viewer_controller.rb#L87
Which throws an error due to single_use_link not existing anymore. This does not appear to be the expected behavior, since the Ability class below seems to expect to receive unset single_use_links:
https://github.com/samvera/hyrax/blob/main/app/controllers/hyrax/single_use_links_viewer_controller.rb#L108

Changes proposed in this pull request:

  • Makes current_ability on SingleUseLinksViewerController behave more like it does on other pages so as not to cause errors in local applications

@samvera/hyrax-code-reviewers

@dlpierce dlpierce merged commit e32c1cf into main Mar 28, 2024
6 checks passed
@dlpierce dlpierce deleted the single-use-current-user branch March 28, 2024 19:00
@dlpierce dlpierce added the notes-bugfix Release Notes: Fixed a bug label Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notes-bugfix Release Notes: Fixed a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants