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

Fixes #24405: Node tabs have onclick event handlers assigned on unrendered elements #5463

Conversation

clarktsiory
Copy link
Contributor

@clarktsiory clarktsiory commented Mar 11, 2024

https://issues.rudder.io/issues/24405
We display the logs table in two different tabs Compliance and System status, but having the same id for different elements across tables makes it impossible to dispatch event handlers correctly.
Also, the event handlers are not set on the Show logs buttons because the tab is has not yet been rendered.

We just append the tab id to every HTML element, and set the buttons HTML with the correct event handler when the tab is rendered.

@clarktsiory clarktsiory marked this pull request as draft March 11, 2024 15:57
@clarktsiory
Copy link
Contributor Author

PR updated with a new commit

@clarktsiory clarktsiory marked this pull request as ready for review March 11, 2024 17:39
@fanf
Copy link
Member

fanf commented Mar 12, 2024

I don't know if it's that PR or if it was like that before, but if the (server) local API is not available, that I get a JS console error in place of a correct error:

XML Parsing Error: syntax error
Location: http://localhost:8082/rudder-web/secure/api/nodes/0c6ea44e-db4a-474a-bfac-0546bfd20371/applyPolicy
Line Number 1, Column 1:

The API seems to return an error correctly:

Error occurred when contacting internal remote-run API to apply classes on Node '0c6ea44e-db4a-474a-bfac-0546bfd20371': Can not connect to local remote run API (POST:https://localhost/rudder/relay-api/remote-run/nodes/0c6ea44e-db4a-474a-bfac-0546bfd20371) 

(and then a lot of \u0 which is maybe a problem)

@fanf
Copy link
Member

fanf commented Mar 12, 2024

It works as expected for the "show log" in compliance, but it doesn't seems to do anything for the system status page. Can you check it's OK ?

@clarktsiory
Copy link
Contributor Author

It works fine in both tabs on my side, did you have any specific error log in the console for the "System status" tab ?

@clarktsiory
Copy link
Contributor Author

Also, running the previous version also return the XML Parsing Error: syntax error on my side, but it still displays the content correctly. I found out this is a browser error : https://stackoverflow.com/a/51000139 (I guess none of us is using Chome ^^)

@fanf
Copy link
Member

fanf commented Mar 12, 2024

OK, finally understood: I had a previous version of the PR :)
So it's working. But now, each time I click on a tab, I have a very viewable loading effect (and it's normal, there's 4 AJAX queries for each tab click o_O). It looks like that PR just "revealed" the underlying inefficient pattern, but now it is very, very annoying.
I'm opening an other ticket for that.

@Normation-Quality-Assistant
Copy link
Contributor

This PR is not mergeable to upper versions.
Since it is "Ready for merge" you must merge it by yourself using the following command:
rudder-dev merge https://github.com/Normation/rudder/pull/5463
-- Your faithful QA
Kant merge: "Live your life as though your every act were to become a universal law."
(https://ci.normation.com/jenkins/job/merge-accepted-pr/81412/console)

@fanf
Copy link
Member

fanf commented Mar 14, 2024

OK, squash merging this PR

@fanf fanf force-pushed the bug_24405/node_tabs_have_onclick_event_handlers_assigned_on_unrendered_elements branch from 7458ccb to 793a058 Compare March 14, 2024 16:39
@fanf fanf merged commit 793a058 into Normation:branches/rudder/8.1 Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants