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

Rails5 ajax fixes #2570

Merged
merged 14 commits into from
Jul 1, 2020
Merged

Rails5 ajax fixes #2570

merged 14 commits into from
Jul 1, 2020

Conversation

briri
Copy link
Contributor

@briri briri commented Jun 22, 2020

  • Updated application.js to share Rails UJS with our custom JS and JS.ERB files
  • Fixed up the pagination concern, js, controllers and views to use strong params and to render JSON when the user has interacted with the pagination, sorting or searching functionality

@raycarrick-ed note that in order for a scenario where we were programmatically calling .submit for a form, we now should use Rails.fire(form[0], 'submit'); (where form is the JQuery element). I found this fix in this article: https://stackoverflow.com/questions/12683524/with-rails-ujs-how-to-submit-a-remote-form-from-a-function

Copy link
Contributor

@raycarrick-ed raycarrick-ed left a comment

Choose a reason for hiding this comment

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

Looks fine to me. It's quite a lot of change in how we handle the AJAX specially.

@briri
Copy link
Contributor Author

briri commented Jun 24, 2020

@raycarrick-ed @xsrust lets hold off on this one. I'm going to play around some more with it to see if starting rails ujs in the packs/application.js is behind our puma issues. These scenarios where we do a form submit through JS may be good candidates for switching over to the js.erb approach (for these specific situations)

@briri
Copy link
Contributor Author

briri commented Jun 26, 2020

ok @raycarrick-ed and @xsrust I think I may have stumbled upon the puma hanging issue. I noticed that it was trying to load the JSON file for the ResearchProjectsController which uses the Rails cache and the Thread library. When I commented out the 'Thread' bit things started puma stopped hanging.

That JSON load may be a good candidate for ActionCable's web-socket functionality. Something to investigate down the road I guess.

This PR should be good to go. Note that I enabled Turbolinks. I think now that we have a handle on the Ajax UJS issue that we can enable it again.

I went through more of the pages and fixed up the ajax. Note the checkboxes on the Plans page and Users page which were previously checkboxes wrapped inside a form. This are now just 'remote' checkboxes and I had to add authenticity_token: true to get it to pass that token along to the controller.

@briri
Copy link
Contributor Author

briri commented Jun 30, 2020

ok some final commits on this one @raycarrick-ed and @xsrust. I fixed up all of the tests broken by adding the optional: true to model associations and also addressed #1974 (make timeago localized). Also made some minor tweaks to get the feature specs working (except for the templates/annotations ones which should start working once @raycarrick-ed updates for the JS on those pages is merged in)

I had to re-disable turbolinks in the application.js

@@ -45,6 +45,7 @@ def set_gettext_locale

def current_locale
session[:locale] || FastGettext.default_locale
Copy link
Contributor Author

Choose a reason for hiding this comment

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

switch to ||=

@@ -4,6 +4,8 @@ class OrgsController < ApplicationController

include OrgSelectable

skip_before_action :verify_authenticity_token, only: %w[search]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

switch to pass auth token

@@ -31,7 +31,8 @@ def funder_type

def fetch_projects
Rails.cache.fetch(["research_projects", funder_type], expires_in: expiry) do
Thread.new { ExternalApis::OpenAireService.search(funder: funder_type) }.value
#Thread.new { ExternalApis::OpenAireService.search(funder: funder_type) }.value
Copy link
Contributor Author

Choose a reason for hiding this comment

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

create ticket to investigate this

@briri briri merged commit 63f920e into rails5 Jul 1, 2020
@briri briri deleted the rails5-ajax-fixes branch July 1, 2020 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants