8000 Detach previous batch_action click handler before attaching new one (es6) by sgara · Pull Request #5563 · activeadmin/activeadmin · GitHub
[go: up one dir, main page]

Skip to content

Detach previous batch_action click handler before attaching new one (es6) #5563

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

Closed

Conversation

sgara
Copy link
Contributor
@sgara sgara commented Nov 13, 2018

Following #5553 and #5562, here is the es6 version of the same patch to ensures any previously attached click handler is being removed (including own batch_actions handler) before attaching own handler.

@deivid-rodriguez
Copy link
Member

Hi @sgara! Thanks for working on this! Is it possible to add a test? Maybe clicking on the batch action and check that a single modal opens?

@deivid-rodriguez
Copy link
Member

Also, can you add a changelog entry for this bug fix following the current changelog format? Remember to add links to your github profile and to this PR at the bottom of the file.

@sgara
Copy link
Contributor Author
sgara commented Nov 16, 2018

Hi @deivid-rodriguez, thanks for the reply! I'd be happy to add the test, just wondering where to add the "turbolinks" gem, is :test section of Gemfile.common a good place to do it?

@deivid-rodriguez
Copy link
Member
deivid-rodriguez commented Nov 16, 2018

Yes, that's the place! Once you add it, run bundle exec rake bundle so that it gets added to the lock file for every Rails version we support 👍.

@activeadmin activeadmin deleted a comment Nov 16, 2018
@deivid-rodriguez
Copy link
Member

I did some work to try reproducing this and write a regression spec on a separate test application that uses turbolinks, but I couldn't get it to reproduce.

I did try the fix manually and it works, so I propose to merge this as is (with a changelog entry).

@activeadmin/maintainers What do you think?

@sgara
Copy link
Contributor Author
sgara commented Jul 18, 2019

Sorry for the lack of news! I did try to add some tests as well but failed to do so as well. Adding turbolinks and all necessary conditions was quite heavy, and then the spec couldn’t reproduce the issue. We had a cucumber scenario that was reproducing it in our own app though so it is doable somehow.

@deivid-rodriguez
Copy link
Member

Yes, I had the exact same problem. After enabling turbolinks in the test application, the cucumber scenario that I wrote would not reproduce the issue for some reason 😬...

So, do you plan to give it another try? I'm very curious about why it wouldn't reproduce in a test environment.

@sgara
Copy link
Contributor Author
sgara commented Jul 19, 2019

I can give it one more attempt over the next few days. I’d say that if I’m still stuck on Tuesday you can just merge it as is. I don’t want to spend months on such a small change 😅

@deivid-rodriguez
Copy link
Member

Yeah, I totally understand. I'm just curious about what's going on here, so thanks for giving it one more try :)

@sgara
Copy link
Contributor Author
sgara commented Jul 25, 2019

Finally got a test reproducing the issue and fix 👍
Closing this one in favour of #5815 (can't push to this branch anymore)

@sgara sgara closed this Jul 25, 2019
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.

2 participants
0