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

Fix for #2873 - CSRF Exception after adding to Collections/Sharing #2875

Merged
merged 3 commits into from Apr 2, 2018

Conversation

jgondron
Copy link
Contributor

@jgondron jgondron commented Mar 29, 2018

Related to rails/jquery-ujs#456

@samvera/hyrax-code-reviewers

- Found the root issue is related to turbolinks + jquery-ujs + a button that uses ajax to post. This may also fix #1191. Adding a call to refresh the CSRF tokens in the rendered DOM after turbolinks loads fixes it.
@no-reply
Copy link
Member

no-reply commented Mar 30, 2018

Looks great! Thanks for tracking this down and getting this in.

Can we confirm whether this does close #1191.

I'd also like to get a regression test in for both tickets. Both seem like basic functionality that we could and should be testing.

@elrayle elrayle added this to Ready for Review in 2.1.0 Release Mar 30, 2018
@elrayle
Copy link
Contributor

elrayle commented Mar 31, 2018

@jgondron You don't say so in the PR message, but doesn't this fix Issue #2873?

@elrayle
Copy link
Contributor

elrayle commented Mar 31, 2018

I did some testing with this branch and was able to remove the data: { turbolinks: false } for app/views/hyrax/my/_collection_action_menu.html.erb for the hyrax.edit_dashboard_collection_path(id) link. I confirmed it got the invalid token error without the change in this PR and did not when using the change in this PR.

This test follows the steps to reproduce the error in issue #2873.  But I can’t reproduce the error when I remove the original PR fix that adds turbolinks_events.js.  So I cannot confirm that this test can serve as a regression test.
@elrayle
Copy link
Contributor

elrayle commented Apr 1, 2018

@no-reply The scenario described in issue #1191 was fixed using a different method than turning off turbo links, so I am not able to directly create a regression test showing the exception happening before this PR's fix and not happening once the PR's fix is applied.

I added a test for the scenario described in Issue #2873 (see commit). But I wasn't able to recreate the exception without this PR's fix.

I was able to verify this PR's fix with another scenario and was able to remove the code that turned off turbolinks. (see commit)

Are you good with merging this PR? It is an excellent fix that I think will really put a dent in our turbolinks woes.

@no-reply no-reply merged commit e37466a into master Apr 2, 2018
2.1.0 Release automation moved this from Ready for Review to Closed Apr 2, 2018
@no-reply no-reply deleted the 2873-csrf-exception branch April 2, 2018 15:10
@elrayle elrayle mentioned this pull request Apr 11, 2018
@elrayle elrayle moved this from Closed to On nurax in 2.1.0 Release Apr 16, 2018
@kdid kdid mentioned this pull request Jul 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
2.1.0 Release
  
On nurax
Development

Successfully merging this pull request may close these issues.

Adding existing works to existing collection fails
3 participants