Fixes #37390 - drop jquery-ui

[MariaAga]

Last use was in b55d3ed
didnt find any other use for the jquery-ui js or css

[MariaAga]

I'll check if Katello uses jquery-ui or are those just leftovers

[ekohl]

If they still are, I wonder what else is silently relying on it. If other plugins don't then we could consider moving it to katello, until it's dropped there

[MariaAga]

Added a PR to Katello Katello/katello#10982

[ekohl]

In https://github.com/theforeman/actions?tab=readme-ov-file#foreman-plugin-ruby-tests I've added documentation on how to run CI for plugins against this PR. You'd use foreman_version: refs/pull/10142/head in the YAML. Though I wonder how many cases we would actually find vs how many cases actually exist.

[adamruzicka]

Do we expect the checks here to turn green now that the katello pr was merged or is there anything else that needs to happen?

[ekohl]

The test failures look unrelated, but I wonder how those ever passed. I don't think that code path was ever supposed to be valid because Struct doesn't define a logger. Did something change in some library that monkey patches something?

[ekohl]

#10410 should fix that. Could you rebase?

[ekohl]

The Katello error looks related:

ActionView::Template::Error: couldn't find file 'jquery-ui/widgets/dialog' with type 'application/javascript'
Checked in these paths: 

When I check Gemfile.lock that's archived I see it includes jquery-ui-rails so perhaps Katello/katello#10982 should have added @import lines as well to make it available in the bundle?

[MariaAga]

@ekohl What are @import lines in what context?

[ekohl]

Isn't that just regular scss? So https://sass-lang.com/documentation/at-rules/import/

[MariaAga]

'jquery-ui/widgets/dialog' with type 'application/javascript'

@ekohl It seems that jquery-ui/widgets/dialog is js and not scss?

https://github.com/Katello/katello/blob/fb6cca703783c443c4a4f4a368f8d5fe1072c20e/app/assets/javascripts/katello/common/vendor.js#L1

[ekohl]

Looking at https://github.com/jquery-ui-rails/jquery-ui-rails/tree/master?tab=readme-ov-file#require-everything I get the impression this was never valid but somehow worked anyway.

Now I look further, I see that this is probably the line that really triggers it:
https://github.com/Katello/katello/blob/fb6cca703783c443c4a4f4a368f8d5fe1072c20e/app/assets/javascripts/katello/common/vendor.js#L1

That looks correct if I look at https://github.com/jquery-ui-rails/jquery-ui-rails/tree/master?tab=readme-ov-file#widgets.

Perhaps easiest to try to get it to work locally and then open a PR to Katello that adds the right requires and point it to this PR (https://github.com/theforeman/actions?tab=readme-ov-file#foreman-plugin-ruby-tests has a section on testing against a specific PR using foreman_version), but our asset pipeline is some dark magic I always need to look up.

I was digging a bit deeper and I see it's used in 1 place:
https://github.com/Katello/katello/blob/fb6cca703783c443c4a4f4a368f8d5fe1072c20e/app/assets/javascripts/katello/common/katello.common.js#L24-L47

Then the customAlert function is only called here:
https://github.com/Katello/katello/blob/fb6cca703783c443c4a4f4a368f8d5fe1072c20e/app/assets/javascripts/katello/common/katello.js#L246

Do I read it right that it tries to replace window.alert with a custom function? I also get the impression that nothing actually calls that so perhaps it's easier to get rid of it?

Sadly, the progressbar is used on the sync management page. I know there's a planned effort to rewrite it, but perhaps @jeremylenz or @parthaa know more about that progress.

[jeremylenz]

the progressbar is used on the sync management page. I know there's a planned effort to rewrite it

There's currently no plan to rewrite Sync Status. (ERB is quite stable, as you know ;) But if you need to remove this I'm sure we can shove a React progress bar in there or figure something else out.

[MariaAga]

Adding require "jquery-ui-rails" in lib/katello.rb seems to fix it, can I add it there?

[ekohl]

Adding require "jquery-ui-rails" in lib/katello.rb seems to fix it, can I add it there?

Oh yes, that must be it. I think with just Foreman it looks at the bundler config and relies on bundler to require it. For plugins that doesn't work and it needs to be explicit. That sounds like the correct fix to me.

There's currently no plan to rewrite Sync Status. (ERB is quite stable, as you know ;) But if you need to remove this I'm sure we can shove a React progress bar in there or figure something else out.

It's more about getting rid dependencies and right now that page means we need to continue including jquery-ui.

[MariaAga]

Thanks, added: Katello/katello#11274

[ekohl]

Now that's merged I just kicked off the failed tests again.

[ekohl]

Current Katello test failures look unrelated. @adamruzicka agreed we can merge this now?

[adamruzicka]

Aye, let's get this in

[adamruzicka]

Thank you @MariaAga , @jeremylenz & @ekohl !