Refs #37104 - fix sync_management #10873

MariaAga · 2024-01-31T15:15:13Z

in app/assets/javascripts/katello/sync_management/sync_management.js
the real change is to move KT.content = and KT.content_actions = to be at the top, and remove $(document).ready(function() { as it will only run the script when the document is ready because of the content_for
moving katello.common.js from sync_management/index.js to sync_management/index.html.erb to make sure its fully loaded in time
Wrapping all the scripts in content_for so they wont run too early

evgeni · 2024-01-31T15:33:28Z

Hmm, trying the packit build, it now fails to load common/index.js:

GET https://centos8-stream-katello-nightly.tanso.example.com/assets/katello/common/index.js [HTTP/2 404  1ms]

Edit: https://centos8-stream-katello-nightly.tanso.example.com/assets/katello/common/index-41006bb366e1429ef7397de66336e76035be453bc9bac600cd845b9571266232.js would exist. Something is missing to add the hash?

evgeni · 2024-01-31T16:37:12Z

app/views/katello/sync_management/index.html.erb

+  <script src="/assets/katello/common/index.js"></script>
+  <script src="/assets/katello/sync_management.js"></script>


Suggested change

<script src="/assets/katello/common/index.js"></script>

<script src="/assets/katello/sync_management.js"></script>

<%= javascript 'katello/common/index' %>

<%= javascript 'katello/sync_management' %>

this makes things load the right files but in the wrong order:

<script type="text/javascript"> const html = " \n \n <script>\n localize({\n \"cancel\": \"Cancel\",\n \"cancelled\": \"Sync Canceled\",\n \"error\": \"Error\",\n \"complete\": \"Sync complete.\",\n \"no_start_time\": \"No start time currently available.\"\n });\n\n KT.repo_status = \$.parseJSON(\'{}\');\n KT.permissions = { \"syncable\" : false };\n <\/script>\n <script src=\"/assets/katello/common/vendor-9f3c77c20de190d174986979ad7d0d5cee9362a9a280adabf643e57ee4259155.js\"><\/script>\n <script src=\"/assets/katello/common-41006bb366e1429ef7397de66336e76035be453bc9bac600cd845b9571266232.js\"><\/script>\n <script src=\"/assets/katello/common/index-41006bb366e1429ef7397de66336e76035be453bc9bac600cd845b9571266232.js\"><\/script><script src=\"/assets/katello/sync_management-db168612191a6dc850c8d5cd28375ee6af25d8fd011e62638f11f7fd903f5025.js\"><\/script>\n" load_dynamic_javascripts(html); </script>

and well, this gives the localize error again

Suggested change

<script src="/assets/katello/common/index.js"></script>

<script src="/assets/katello/sync_management.js"></script>

<%= javascript_include_tag 'katello/common/index' %>

<%= javascript_include_tag 'katello/sync_management' %>

This works.
Now, wth is the difference between javascript and javascript_include_tag?!

Ah, it's already wrapping it in content_for… Weird

https://github.com/theforeman/foreman/blob/262b5caceab9c3f71d0b80f189cc724389593fb0/app/helpers/layout_helper.rb#L98-L100

Thinking more, this makes sense!

Your original intent was "have the script load tags and the following embedded code in one block, so that it gets executed in the right order", but had the bug that it ignored asset pipeline paths.

Using javascript creates a content_for(:javascripts…) block, which gets yielded into the :javascripts section individually (so the = of <%= didn't really do anything) and thus the order is fucked up

Using javascript_include_tag creates script tags that honor the asset pipeline AND prints them directly where the code is, thus resulting in the correct order as one block, which then gets yielded into :javascripts.

Mhh. Clicking the "active only" checkbox still gives me

Uncaught TypeError: KT.content is undefined <anonymous> sync_management-db168612191a6dc850c8d5cd28375ee6af25d8fd011e62638f11f7fd903f5025.js:1 jQuery 2 dispatch handle

and spinner.gif is 404 (but that's already a problem in 4.9, and probably older: https://projects.theforeman.org/issues/37133)

>>> KT <<< Object { utils: $(n), common: {…}, widget: {}, getData: getData(n), orgswitcher: {…}, menu: {…}, object: {…} } >>> load_dynamic_javascripts("<script src=\"/assets/katello/sync_management-db168612191a6dc850c8d5cd28375ee6af25d8fd011e62638f11f7fd903f5025.js\"><\/script>") <<< undefined >>> KT <<< Object { utils: $(n), common: {…}, widget: {}, getData: getData(n), orgswitcher: {…}, menu: {…}, object: {…}, content: {…}, content_actions: {…} }

So reloading that sync management script works. WTH.

Could it be something re-initializes KT, thus overwriting what this code did?

evgeni · 2024-02-01T09:23:00Z

app/views/overrides/activation_keys/_host_environment_select.html.erb

-<%= javascript "katello/hosts/host_and_hostgroup_edit" %>
+<% content_for(:javascripts) do -%>
+  <%= javascript "katello/hosts/host_and_hostgroup_edit" %>
+<% end %>


this is redundant, <%= javacript … already does content_for(:javascripts): https://github.com/theforeman/foreman/blob/262b5caceab9c3f71d0b80f189cc724389593fb0/app/helpers/layout_helper.rb#L98-L100

evgeni · 2024-02-01T09:24:28Z

app/views/overrides/activation_keys/_host_tab_pane.html.erb

-<%= javascript "katello/hosts/activation_key_edit" %>
+<% content_for(:javascripts) do -%>
+  <%= javascript "katello/hosts/activation_key_edit" %>
+<% end %>



this is redundant, <%= javacript … already does content_for(:javascripts): https://github.com/theforeman/foreman/blob/262b5caceab9c3f71d0b80f189cc724389593fb0/app/helpers/layout_helper.rb#L98-L100

evgeni · 2024-02-01T09:25:27Z

app/views/overrides/organizations/_edit_override.html.erb

-
-  <%= javascript_include_tag 'katello/organizations/download_certificate' %>
-
+  <% content_for(:javascripts) do -%>
+    <%= javascript_include_tag 'katello/organizations/download_certificate' %>
+  <% end %>


should we standardize on either

<%= javascript … %>

OR

<% content_for(:javascripts) do -%> <%= javascript_include_tag … %> <% end %>

using both seems odd?

I rather use

<% content_for(:javascripts) do -%> <%= javascript_include_tag … %> <% end %>

just because when I see <%= javascript … %> I assumed it was something default from ruby and not a custom thing

I do agree that ths makes it more explicit.

evgeni · 2024-02-01T10:17:32Z

app/views/katello/sync_management/index.html.erb

@@ -1,17 +1,19 @@
-<%= javascript 'katello/sync_management' %>
+<% content_for(:javascripts) do -%>


Suggested change

<% content_for(:javascripts) do -%>

<% content_for(:katello_javascripts) do -%>

THIS fixes it.

the reset happens here:

katello/app/views/katello/layouts/katello.html.erb

Lines 9 to 13 in 136a73e

<% content_for(:javascripts) do %>

<%= javascript_include_tag "katello/common/vendor" %>

<%= javascript_include_tag "katello/common" %>

<%= yield(:katello_javascripts) %>

<% end %>

and once done like this, we don't have to reference katello/common/index ourself at all, as that is now loaded in the right order

<% content_for(:katello_javascripts) do -%> <%= javascript_include_tag 'katello/sync_management' %> <script> localize({ "cancel": "<%= escape_javascript(_('Cancel')) %>", "cancelled": "<%= escape_javascript(_('Sync Canceled')) %>", "error": "<%= escape_javascript(_('Error')) %>", "complete": "<%= escape_javascript(_('Sync complete.')) %>", "no_start_time": "<%= escape_javascript(_('No start time currently available.')) %>" }); KT.repo_status = $.parseJSON('<%= escape_javascript(@repo_status.to_json.html_safe) %>'); KT.permissions = { "syncable" : <%= any_syncable? %> }; </script> <% end -%>

this is how it looks in full on my box now

I still get "Uncaught TypeError: Cannot read properties of undefined (reading 'select_repo')" on select none with this?

Works for me (on a prod install, don't have a devel katello right now)

This is the code generated for loading (I added line breaks for readability):

<script type="text/javascript"> const html = " <script src=\"/assets/katello/common/vendor-9f3c77c20de190d174986979ad7d0d5cee9362a9a280adabf643e57ee4259155.js\"><\/script>\n <script src=\"/assets/katello/common-41006bb366e1429ef7397de66336e76035be453bc9bac600cd845b9571266232.js\"><\/script>\n <script src=\"/assets/katello/sync_management-db168612191a6dc850c8d5cd28375ee6af25d8fd011e62638f11f7fd903f5025.js\"><\/script>\n <script>\n localize({\n \"cancel\": \"Cancel\",\n \"cancelled\": \"Sync Canceled\",\n \"error\": \"Error\",\n \"complete\": \"Sync complete.\",\n \"no_start_time\": \"No start time currently available.\"\n });\n\n KT.repo_status = \$.parseJSON(\'{\\\"1\\\":{\\\"id\\\":1,\\\"product_id\\\":1,\\\"progress\\\":{},\\\"state\\\":\\\"Never Synced\\\",\\\"raw_state\\\":\\\"never_synced\\\"}}\');\n KT.permissions = { \"syncable\" : true };\n <\/script>\n\n" load_dynamic_javascripts(html); </script>

Yeah… but that still means that the code gets executed twice, which seems wrong :)

I mean… It's probably still a good idea to have such a safety guard, but it should not be the "real" fix

which code gets executed twice?

katello/common gets executed twice if we use content_for(:javascripts) (and not :katello_javascripts), as that generates:

katello/common (from sync_management/index.html)

katello/sync_management

katello/common (from layouts/katello)

I mean… It's probably still a good idea to have such a safety guard, but it should not be the "real" fix

Yeah, I thought you meant adding it will cause the double execution.
Added it to the pr and used your solution, thanks!

evgeni · 2024-02-01T11:07:48Z

FWIW, I'd omit the "Refs #37104 - wrap scripts in content_for(:javascripts)" commit from this PR and only have the "fix sync_management" changes

(but I am neither author, nor reviewer, so just stating my thoughts ;) )

jeremylenz · 2024-02-01T15:26:06Z

merging based on @evgeni's review

Thanks @MariaAga !

pr-processor bot added the Not yet reviewed label Jan 31, 2024

MariaAga mentioned this pull request Jan 31, 2024

webpack5 #10735

Merged

evgeni reviewed Jan 31, 2024

View reviewed changes

evgeni reviewed Feb 1, 2024

View reviewed changes

Refs #37104 - fix sync_management

8f5136e

MariaAga force-pushed the webpack5-sync-page branch from a1fd6f4 to 8f5136e Compare February 1, 2024 11:42

evgeni approved these changes Feb 1, 2024

View reviewed changes

pr-processor bot removed the Not yet reviewed label Feb 1, 2024

jeremylenz approved these changes Feb 1, 2024

View reviewed changes

jeremylenz merged commit e8229c3 into Katello:master Feb 1, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refs #37104 - fix sync_management #10873

Refs #37104 - fix sync_management #10873

MariaAga commented Jan 31, 2024

evgeni commented Jan 31, 2024 •

edited

Loading

evgeni Jan 31, 2024

evgeni Feb 1, 2024

evgeni Feb 1, 2024

evgeni Feb 1, 2024

evgeni Feb 1, 2024 •

edited

Loading

evgeni Feb 1, 2024

evgeni Feb 1, 2024

evgeni Feb 1, 2024

evgeni Feb 1, 2024

evgeni Feb 1, 2024

MariaAga Feb 1, 2024

evgeni Feb 1, 2024

evgeni Feb 1, 2024 •

edited

Loading

evgeni Feb 1, 2024

MariaAga Feb 1, 2024

evgeni Feb 1, 2024

evgeni Feb 1, 2024

evgeni Feb 1, 2024

evgeni Feb 1, 2024

MariaAga Feb 1, 2024

evgeni Feb 1, 2024

MariaAga Feb 1, 2024

evgeni commented Feb 1, 2024

jeremylenz commented Feb 1, 2024

		<script src="/assets/katello/common/index.js"></script>
		<script src="/assets/katello/sync_management.js"></script>

		@@ -1,17 +1,19 @@
		<%= javascript 'katello/sync_management' %>
		<% content_for(:javascripts) do -%>

	<% content_for(:javascripts) do %>
	<%= javascript_include_tag "katello/common/vendor" %>
	<%= javascript_include_tag "katello/common" %>
	<%= yield(:katello_javascripts) %>
	<% end %>

Refs #37104 - fix sync_management #10873

Refs #37104 - fix sync_management #10873

Conversation

MariaAga commented Jan 31, 2024

evgeni commented Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evgeni Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evgeni Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evgeni commented Feb 1, 2024

jeremylenz commented Feb 1, 2024

evgeni commented Jan 31, 2024 •

edited

Loading

evgeni Feb 1, 2024 •

edited

Loading

evgeni Feb 1, 2024 •

edited

Loading