Fixes #38416 - Hosts bulk action : Assign organization/ location by lfu · Pull Request #10538 · theforeman/foreman

lfu · 2025-05-13T10:58:01Z

Add Assign organization/location for new hosts page.

To test
Select hosts - Assign organization/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.
Move one host/multiple hosts from current org and current location to another org and another location.
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 · 2025-06-13T14:21:03Z

Could we have a review here?

jeremylenz

When I reassign hosts out of my organization but have my org selected in org context (e.g. not "Any Organization") the hosts are reassigned but I get this weird success message about 0 hosts:

Then I tried to assign the test2 hosts back to Default Organization and got this:

Even though I had said "Fix on mismatch" so there should be no mismatch in settings.

Reassigning with "Any Organization" selected seems to work OK.

Side note: It might be nice to be able to add "Organization" and "Location" columns to the new All Hosts page.

Another problem: If a host is already registered, I should not be able to change its organization.

I should get an error similar to this one:

~~But I was able to reassign org for my registered hosts without error here.~~ edit: Oops, didn't see the Katello PR. 😳

jeremylenz · 2025-06-16T20:22:31Z

Suggested change

api :PUT, "/hosts/bulk/assign_taxonomy", N_("Assign organization/ location")

api :PUT, "/hosts/bulk/assign_taxonomy", N_("Assign organization/location")

jeremylenz · 2025-06-16T20:22:49Z

Suggested change

param :optimistic_import, :bool, N_("Fix organization/ location on mismatch")

param :optimistic_import, :bool, N_("Fix organization/location on mismatch")

jeremylenz · 2025-06-16T20:25:12Z

You can't use JS template strings (${taxonomy} etc.) with __, unfortunately. Need to use a <FormattedMessage> instead, for the translation to work correctly.

jeremylenz · 2025-06-16T20:25:41Z

Suggested change

'Select organization/ location to add systems to. This change may affect all your selected systems. Some hosts may already be in your chosen group.'

'Select organization/location to add systems to. This change may affect all your selected systems. Some hosts may already be in your chosen group.'

jeremylenz · 2025-06-16T20:32:54Z

I see the tooltips next to "Select organization." But that implies that it's explaining the text "Select organization," which is self-explanatory. I would expect the tooltip to be next to "Fix on mismatch," which is what it's explaining.

jeremylenz · 2025-06-16T20:35:45Z

"Some hosts may already be in your chosen group" was about hostgroups.
We are not adding additional; rather, we are assigning.
We call them "hosts" and not "systems" everywhere in Satellite, so this should be consistent.

Suggested change

'Select organization/ location to add systems to. This change may affect all your selected systems. Some hosts may already be in your chosen group.'

'Select organization/location to assign hosts to. This change may affect all your selected hosts.'

Those messages come from the provided mock page.
I could use your suggestion as well.

I vote for my suggestion 😇 but will also tag @MariSvirik in case she thinks more argument is needed :)

👍 for what @jeremylenz said because Foreman uses hosts, not systems. (Katello used to have systems, but thankfully it's now aligned on hosts).

MariSvirik · 2025-06-18T08:33:46Z

Microcopy: Do it, microcopy is very often a placeholder and open for discussion. Thank you for spotting problems.
Index page columns: If you want to add columns to the index page, feel free to do it, but please, don't make it default
Questionmark icon - keep it next to the field label, add a description about mismatches. If you add it to the radios, it would get crowded by circles. It gives information about the entire section - field and connected selections.
Check the alignment of a radio and a label - it should be center-aligned horizontally. View PF5 here.
@jeremylenz

jeremylenz · 2025-06-24T13:26:04Z

How do I get the selected orgId for the new modal?

You should be able to use useForemanOrganization, similar to this:

https://github.com/theforeman/foreman_rh_cloud/blob/develop/webpack/ForemanInventoryUpload/Components/InventoryFilter/InventoryFilter.js#L22

lfu · 2025-06-25T15:46:39Z

Updated.

ekohl · 2025-06-25T16:35:53Z

This looks like an unrelated change. If it was intentional, it still needs its own PR.

It's intentional to fix the typo.

Should it then be fixed in the description too?

ekohl · 2025-06-25T16:36:56Z

Suggested change

msg = tax_type == 'organization' ? _("organizatoin %s") % tax.name : _("location %s") % tax.name

msg = tax_type == 'organization' ? _("organization %s") % tax.name : _("location %s") % tax.name

Nice catch 👍

ekohl · 2025-06-25T16:39:59Z

Should this be a proper boolean?

It is a string.
This code is just got refactored here. Shared by both API v1 and v2 code to reassign taxonomy.

You can introduce a proper API in the new ome. Then in the old controller compare it to yes to make it a boolean

This sounds right. 👍

ekohl · 2025-06-25T16:43:44Z

Count is fairly expensive and !any? should be cheaper.

Suggested change

elsif taxonomy.need_to_be_selected_ids.count == 0

elsif !taxonomy.need_to_be_selected_ids.any?

This refactor code is really complicated. I'd like to not touch it at all.
But this change seems simple. So here it is.

Feel it is not straightforward to read !my_array.any?. Look what Google says.

I put back the original code.

Looks like this is a HashWithIndifferentAccess so it's not a query as I assumed. That means it probably doesn't matter all that much.

Feel it is not straightforward to read !my_array.any?. Look what Google says.

Source? https://stackoverflow.com/questions/53480273/are-there-performance-reasons-to-prefer-size-over-length-or-count-in-ruby/53480585#53480585 says

For Hash is not redefined and will fallback to Enumerable#count, which means its complexity will be O(n) as all key-values will be traversed.For Hash is not redefined and will fallback to Enumerable#count, which means its complexity will be O(n) as all key-values will be traversed.

Note your screenshot states:

collections with a count property (e.g., Arrays)

But Hash is not such a collection if my quick research is correct.

Note that Hash implements enumerable so you can also write:

Suggested change

elsif taxonomy.need_to_be_selected_ids.count == 0

elsif taxonomy.need_to_be_selected_ids.none?

Overall, I think that should win on both performance and readability.

ekohl · 2025-06-25T16:55:30Z

The space after the / is really jarring to me. I also question assign. We don't have that for Host group so why do we have it here? Would this be better?

Suggested change

{__('Assign organization/ location')}

{__('Change organization and/or location')}

I agree with you. And in my opinion it is not a good idea to combine organization and location into one page. But I might be wrong.

Let me paste the mock page here that I'm trying to follow.

I debated that as well, but there is one potential benefit: if the location belongs to another org you can do it in a single operation. It may otherwise not be possible.

Another option is to go another route and actually combine all bulk changes into 1: also include the host group into the dialog. Then the menu item is simple (a single button). Host groups may also belong to another org so it may actually be common to perform that in a single operation.

It is a multiple operation to change the hosts if both organization and location are selected. Hosts are modified in separate calls.
It's not just two attributes. There is lots of checking there for a list of things that might mismatch.
Without enough knowledge in that area, I think it's better just keep it as it is.

ekohl · 2025-06-25T16:55:58Z

I don't consider host group an affiliation. Can we avoid that word? Overall I'm wondering if a fly out menu is a good idea for 2 items. Perhaps just list them both as their own?

How about this..

Change associations > Host group Host collections Owner Location/organization Disassociate compute resource

I think that could work. Or, as I suggested in #10538 (comment), group more together in the real dialog.

ekohl · 2025-06-25T16:58:09Z

I think taxonomy itself needs to be translated as well. Otherwise you'll end up with an English word in a non-English sentence.

Nice catch 👍
Updated.

ekohl · 2025-06-25T17:01:11Z

Please don't parse error messages. HTTP responses have a code.

Suggested change

if (message.includes('Request failed with status code 422')) {

message = __('Update failed because of mismatch in settings');

} else if (message.includes('Request failed with status code 400')) {

if (response.status == 422) {

message = __('Update failed because of mismatch in settings');

} else if (response.status == 400) {

Having said that, please make sure the HTTP response contains a proper message you can display instead of coming up with your own. Server side you often know much better why something failed.

Somehow the response here is not in that format. There is no response.status.

Could I have more details about HTTP response? How to set it up and where?

handleError calls APIActions.put which is also in Foreman. I think a good start would be to go where that's defined and do some console logs.

Thanks to @ekohl for catching this and @jeremylenz @parthaa for finding out the issue.

ekohl · 2025-06-25T17:01:41Z

Regular users do not have access to the server logs so that's not a valid suggestion. They will get stuck.

Updated the message.

ekohl

Summary of this review: please modify assign_taxonomy to assign both taxonomies at once and make it return the number of updated hosts. Then return that to the user.

ekohl · 2025-06-30T09:11:18Z

Isn't this easier?

Suggested change

if [params[:optimistic_import_organization], params[:optimistic_import_location]].any? { |val| !val }

if !params[:optimistic_import_organization] || !params[:optimistic_import_location]

But effectively that means both options are mandatory. Why present them as options at all then?

Another thing to consider: is it mandatory to assign both a location and organization? From reading the code (Taxonomy.where) I think it deals with nil because then targets will not have the entry. That implies you don't need optimistic_import to be true for that type.

And to be clear, I can see benefits in presenting the option so perhaps it's better to just drop this validation.

Now that also makes me think you can return early if neither target_organization_id nor target_location_id was given.

ekohl · 2025-06-30T09:13:47Z

Nit: you can shorten this

Suggested change

messages = []

targets = Taxonomy.where(id: [params[:target_organization_id], params[:target_location_id]])

targets.each do |tax|

tax_type = tax.type.downcase

BulkHostsManager.new(hosts: @hosts).assign_taxonomy(tax, params["optimistic_import_#{tax_type}"])

msg = tax_type == 'organization' ? _("Organization is set to %s.") % tax.name : _("Location is set to %s.") % tax.name

messages << msg

end

targets = Taxonomy.where(id: [params[:target_organization_id], params[:target_location_id]])

messages = targets.map do |tax|

tax_type = tax.type.downcase

BulkHostsManager.new(hosts: @hosts).assign_taxonomy(tax, params["optimistic_import_#{tax_type}"])

tax_type == 'organization' ? _("Organization is set to %s.") % tax.name : _("Location is set to %s.") % tax.name

end

ekohl · 2025-06-30T09:14:40Z

If messages is empty then it didn't update anything. Perhaps that should also be considered?

And I'd still think that assign_taxonomy should return the number of updated hosts instead of just assuming every host was updated. See inline comments there for hints.

This is the message for a success update. All hosts will be updated in this case.
And no hosts should be updated in case of a failure. That is why i try to bail out earlier by checking the settings.

ekohl · 2025-06-30T09:16:40Z

Since it's failing, could you update it as well so the failure message is useful?

Suggested change

assert flash[:error] == "Cannot update location to Location 1 because of mismatch in settings"

assert_equal "Cannot update location to Location 1 because of mismatch in settings", flash[:error]

I can change the assert style. But I don't see how it changes anything about the failure message.

Didn't you see the failure? It said that false wasn't true. With assert_equal it will tell you the exact mismatch: both the expected and actual texts. That makes looking at CI results better.

ekohl · 2025-06-30T09:26:58Z

You can consistently make it return the number of updated hosts:

Suggested change

@hosts.update_all("#{tax_type}_id".to_sym => taxonomy.id)

# hosts location needs to be updated before import missing ids

taxonomy.import_missing_ids

updated = @hosts.update_all("#{tax_type}_id".to_sym => taxonomy.id)

# hosts location needs to be updated before import missing ids

taxonomy.import_missing_ids

updated

Then you can also update the documentation of this method with:

# @return [Integer] # The number of updated hosts

You can then use that in the bulk controller to accurately state how many hosts were updated.

However, that also presents another problem: you can't simply add the 2 numbers and they may have overlap. That again stresses why it would be better to do both updates in a single transaction and only update the hosts once. I'd expect it to be faster and also gives a more accurate summary to the user.

updated = @hosts.update_all("#{tax_type}_id".to_sym => taxonomy.id)

Isn't this the same as @hosts.count?

Currently there is no problem to find out how many hosts are updated accurately. Not sure what the concern is here.

It is really hard to make the both updates in one transaction considering the two different mismatch settings and the support for both API versions. And I'm not sure if taxonomy.import_missing_ids works with updating both organization and location in one transaction or not.

If it really worth to make it one transaction and it is doable, maybe we can do it in a followup.

Isn't this the same as @hosts.count?

No, it's not. update_all returns the number of affected rows, not how many you sent in the first place.

So it is at least 0 (in case nothing was updated) or at most @hosts.count (in case all rows were updated).

Currently there is no problem to find out how many hosts are updated accurately. Not sure what the concern is here.

That's because you misunderstood the return value of update_all. There is a problem.

It is really hard to make the both updates in one transaction considering the two different mismatch settings and the support for both API versions. And I'm not sure if taxonomy.import_missing_ids works with updating both organization and location in one transaction or not.

The last point is an interesting challenge. But if you can't, then combining both into a single frontend dialog may be wrong to start with.

If it really worth to make it one transaction and it is doable, maybe we can do it in a followup.

I think it is about returning a correct number. If you don't want to show a correct number, then please don't show one at all (like the old UI).

I will change the message to not display number of hosts.

The last point is an interesting challenge. But if you can't, then combining both into a single frontend dialog may be wrong to start with.

I want to make it right. Do you think it is reasonable to have 2 tasks for change organization and change location instead of 1 task here in this PR? @ekohl @jeremylenz

cc @parthaa

I think having separate modals for change org & change location would be fine, especially since it simplifies our implementation here. I can't imagine a lot of users wanting to do both at the same time, but if I am wrong, it's a rare-enough task that it's worth the tradeoff.

Today it's also separate, so it's the most straight forward migration for the user.

lfu · 2025-07-02T18:57:38Z

Close this one in favor of #10591.

pr-processor Bot added the Not yet reviewed label May 13, 2025

github-actions Bot added the UI label May 13, 2025

lfu force-pushed the 31030_change_org_loc branch 16 times, most recently from 874862d to 1d2dcb6 Compare May 16, 2025 18:20

lfu force-pushed the 31030_change_org_loc branch from 1d2dcb6 to 5745831 Compare May 20, 2025 16:40

lfu mentioned this pull request May 20, 2025

Fixes #38436 - Override assign_taxonomy to check registered hosts before assigning to a new organization Katello/katello#11396

Merged

lfu force-pushed the 31030_change_org_loc branch 3 times, most recently from f2d8412 to d224847 Compare May 29, 2025 17:19

jeremylenz requested changes Jun 16, 2025

View reviewed changes

pr-processor Bot added Waiting on contributor and removed Not yet reviewed labels Jun 16, 2025

lfu force-pushed the 31030_change_org_loc branch from d224847 to 9471bac Compare June 20, 2025 16:27

pr-processor Bot added the Needs re-review label Jun 20, 2025

lfu force-pushed the 31030_change_org_loc branch 2 times, most recently from b6bcda0 to 3b4d838 Compare June 23, 2025 17:30

lfu force-pushed the 31030_change_org_loc branch 5 times, most recently from 80cd367 to f4f8308 Compare June 25, 2025 15:06

ekohl requested changes Jun 25, 2025

View reviewed changes

pr-processor Bot added the Waiting on contributor label Jun 25, 2025

lfu force-pushed the 31030_change_org_loc branch from f4f8308 to 5cfa1fe Compare June 25, 2025 21:28

pr-processor Bot added Needs re-review and removed Waiting on contributor Needs re-review labels Jun 25, 2025

lfu force-pushed the 31030_change_org_loc branch 4 times, most recently from f1e543c to e5cd7fc Compare June 27, 2025 20:06

ekohl reviewed Jun 30, 2025

View reviewed changes

lfu force-pushed the 31030_change_org_loc branch 5 times, most recently from 12e2fe8 to 7a0a850 Compare July 1, 2025 15:25

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

26dfacd

lfu force-pushed the 31030_change_org_loc branch from 7a0a850 to 26dfacd Compare July 2, 2025 13:51

lfu mentioned this pull request Jul 2, 2025

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

Merged

lfu closed this Jul 2, 2025

	api :PUT, "/hosts/bulk/assign_taxonomy", N_("Assign organization/ location")
	api :PUT, "/hosts/bulk/assign_taxonomy", N_("Assign organization/location")

	param :optimistic_import, :bool, N_("Fix organization/ location on mismatch")
	param :optimistic_import, :bool, N_("Fix organization/location on mismatch")

	'Select organization/ location to add systems to. This change may affect all your selected systems. Some hosts may already be in your chosen group.'
	'Select organization/location to add systems to. This change may affect all your selected systems. Some hosts may already be in your chosen group.'

	'Select organization/ location to add systems to. This change may affect all your selected systems. Some hosts may already be in your chosen group.'
	'Select organization/location to assign hosts to. This change may affect all your selected hosts.'

	msg = tax_type == 'organization' ? _("organizatoin %s") % tax.name : _("location %s") % tax.name
	msg = tax_type == 'organization' ? _("organization %s") % tax.name : _("location %s") % tax.name

	elsif taxonomy.need_to_be_selected_ids.count == 0
	elsif !taxonomy.need_to_be_selected_ids.any?

	elsif taxonomy.need_to_be_selected_ids.count == 0
	elsif taxonomy.need_to_be_selected_ids.none?

	{__('Assign organization/ location')}
	{__('Change organization and/or location')}

-    if (message.includes('Request failed with status code 422')) {
-      message = __('Update failed because of mismatch in settings');
-    } else if (message.includes('Request failed with status code 400')) {
+    if (response.status == 422) {
+      message = __('Update failed because of mismatch in settings');
+    } else if (response.status == 400) {

	if [params[:optimistic_import_organization], params[:optimistic_import_location]].any? { \|val\| !val }
	if !params[:optimistic_import_organization] \|\| !params[:optimistic_import_location]

-          messages = []
-          targets = Taxonomy.where(id: [params[:target_organization_id], params[:target_location_id]])
-          targets.each do |tax|
-            tax_type = tax.type.downcase
-            BulkHostsManager.new(hosts: @hosts).assign_taxonomy(tax, params["optimistic_import_#{tax_type}"])
-            msg = tax_type == 'organization' ? _("Organization is set to %s.") % tax.name : _("Location is set to %s.") % tax.name
-            messages << msg
-          end
+          targets = Taxonomy.where(id: [params[:target_organization_id], params[:target_location_id]])
+          messages = targets.map do |tax|
+            tax_type = tax.type.downcase
+            BulkHostsManager.new(hosts: @hosts).assign_taxonomy(tax, params["optimistic_import_#{tax_type}"])
+            tax_type == 'organization' ? _("Organization is set to %s.") % tax.name : _("Location is set to %s.") % tax.name
+          end

	assert flash[:error] == "Cannot update location to Location 1 because of mismatch in settings"
	assert_equal "Cannot update location to Location 1 because of mismatch in settings", flash[:error]

-      @hosts.update_all("#{tax_type}_id".to_sym => taxonomy.id)
-      # hosts location needs to be updated before import missing ids
-      taxonomy.import_missing_ids
+      updated = @hosts.update_all("#{tax_type}_id".to_sym => taxonomy.id)
+      # hosts location needs to be updated before import missing ids
+      taxonomy.import_missing_ids
+      updated

Conversation

lfu commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lfu commented Jun 13, 2025

Uh oh!

jeremylenz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MariSvirik commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeremylenz commented Jun 24, 2025

Uh oh!

lfu commented Jun 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lfu Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lfu commented May 13, 2025 •

edited

Loading

jeremylenz left a comment •

edited

Loading

MariSvirik commented Jun 18, 2025 •

edited

Loading

lfu Jun 25, 2025 •

edited

Loading