Fixes #38416 - Hosts bulk action: Assign organization and Assign location

[lfu]

Add Assign organization and Assign location for new hosts page.

Replace #10538.

To test
Select hosts - Change associations - select Organization or Location from Kebab menu.

Some test cases.
Prepare a testing environment with hosts, both registered and unregistered. Multiple organizations and locations.

Move one host/multiple hosts from a current organization to another. Error message for registered hosts.
Move one host/multiple hosts from a current location to another.

Repeat above tests with Any Organization and Any Location context.
Registered hosts are not allowed to change organization. All other cases should work.
Tests for registered hosts need Katello/katello#11396.

[lfu]

[lfu]

cc @ekohl @jeremylenz

[jeremylenz]

Tested and seems to be working well. Some comments below :)

[jeremylenz]

In Foreman we normally call this file index.js.

[lfu]

One file could have only one default export.

[jeremylenz]

The file can be called whatever, no matter what the export is.

[jeremylenz]

This tooltip is still in the wrong place. The question icon and tooltip should be next to "Fix on mismatch" and "Fail on mismatch."

[lfu]

Each one of them has one tooltip? Or just one next to Fail on mismatch?

[jeremylenz]

Ideally there would be one tooltip next to Fix on mismatch, explaining Fix on mismatch, and one tooltip next to Fail on mismatch, explaining Fail on mismatch.

[jeremylenz]

Suggested change

	const BulkAssignLocatioModalScene = () => {
	const BulkAssignLocationModalScene = () => {

[ekohl]

Not insisting on this, but it may be a good chance to clean this up and rewrite clear_current_taxonomy and restore_current_taxonomy into a single method:

def without_taxonomy
  context = nil # to ensure the variable exists
  context = Foreman::ThreadSession::Context.get
  Foreman::ThreadSession::Context.set(user: context[:user])
  yield
ensure
  Foreman::ThreadSession::Context.set(**context) if context
end

Then inside assign_taxonomy itself:

without_taxonomy do
  # ...
end

That way the scoping is clear and you can't forget the restore.

[ekohl]

I have not seen a good argument why you use @hosts.count instead of letting assign_taxonomy return the number of affected rows. I started that discussion in #10538 (comment) and thought you'd use the separation to easily implement that.

Please make the change I suggested. Summarizing what I said earlier:

• @hosts.count will always return the number of rows.
• update_all returns the number of affected rows.
• You can say: 0 <= affected <= number of rows. If you run update_all a second time the number of affected is 0.

[lfu]

[17] pry(main)> Host.update_all(organization_id: 1)
=> 14
[18] pry(main)> Host.update_all(organization_id: 1)
=> 14
[19] pry(main)> Host.update_all(organization_id: 1)
=> 14
[20] pry(main)> Host.update_all(organization_id: 1)
=> 14

[lfu]

Instead of showing how many hosts are updated in a popup, we could display how many hosts are selected in the modal itself. I prefer the modal approach.

I need the ForemanActionsBarContext.Provider change in #10560 to display the selected hosts in the modal. Any chance to merge that PR first?

[ekohl]

I'm still going to insist that you don't use @hosts.count but rather use the result of update_all. Making BulkHostsManager responsible for that is IMHO a cleaner abstraction and it saves a query. I don't understand why you resist that.

[lfu]

@hosts.update_all(organization_id: 1) would always update all of them. That is @hosts.count if the command is successful.
Otherwise it would throw an error if it fails.
We don't need to do it as you suggested that is unnecessary.

[lfu]

In other words, @hosts.count is the result of update_all here if it is successful.

[lfu]

You can say: 0 <= affected <= number of rows. If you run update_all a second time the number of affected is 0.

This is not right. It is either @hosts.count or an error we would catch.

[ekohl]

The taxonomy will be untranslated now.

Also I think this should use render_error, probably with a 404 error. When you implement that, please also document it with error 404, 'the description here' so it ends up in the API documentation.

[ekohl]

I think the whole method will be easier if you rewrite this to separate API paths. So assign_organization and assign_location. Most lines within this are simply there to support 2 different types, but as you can see in the translations that very quickly gets messy.

[ekohl]

This ends up being a user visible message so it should be translated

[lfu]

Could we have some care here? Thanks!

[LadislavVasina1]

@lfu Hi, I have tried getting the packit build of this PR, and I have found out that the ManageColumns button is missing in the "New UI" of the All hosts page

[LadislavVasina1]

@lfu in the packit build I am also missing the Manage content option from the bulk actions, thus ManageContent/(Packages, Errata ,Repository sets, content source, content view environments) are missing

[LadislavVasina1]

^^^Mentioned issues don't affect latest All hosts page redesign features though

[jeremylenz]

Tested again and still works well, just a couple remaining issues

1. Please rebase and fix lint failures
2. Tooltips and radio buttons are not vertically aligned with text:

[lfu]

2. Tooltips and radio buttons are not vertically aligned with text:

@jeremylenz That was the result when alignItems: 'center'.

With alignItems: 'baseline':

Is this one looking better? Somehow the position between the button and its label was never changed.
Could you please help fine tune this?

[lfu]

Tests failure not related.

[jeremylenz]

Thanks @lfu ! LGTM

comments were addressed

[MariaAga]

@jeremylenz @lfu, reminder to always use specific CSS rules, as this PR changes the position of all radio inputs in foreman + plugins

Use specific css selectors to avoid conflicts with other plugins or the core.

https://theforeman.org/handbook.html

Added a fix here: #10967