-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Detach previous batch_action click handler before attaching new one (es6) #5563
Conversation
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? |
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. |
Hi @deivid-rodriguez, thanks for the reply! I'd be happy to add the test, just wondering where to add the "turbolinks" gem, is |
Yes, that's the place! Once you add it, run |
I did some work to try reproducing this and write a regression spec on a separate test application that uses 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? |
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. |
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. |
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 😅 |
Yeah, I totally understand. I'm just curious about what's going on here, so thanks for giving it one more try :) |
Finally got a test reproducing the issue and fix 👍 |
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.