Refactor #16718

Replace modifications to host list relation in host multiple action code

Added by Dominic Cleal about 1 year ago. Updated 5 months ago.

Status:Closed
Priority:Normal
Assigned To:Dominic Cleal
Category:Rails
Target version:-
Difficulty: Bugzilla link:
Found in release: Pull request:https://github.com/theforeman/foreman/pull/3898, https://github.com/theforeman/foreman/pull/4613
Story points-
Velocity based estimate-
Release1.14.0Release relationshipAuto

Description

On Rails 5, some of the host multiple action tests fail as the controller states that all hosts failed, instead of all succeeding:

HostsControllerTest::submit actions with multiple hosts#test_0001_build without reboot [/home/dcleal/code/foreman/foreman/test/controllers/hosts_controller_test.rb:686]:
--- expected
+++ actual
@ -1 +1 @
-"The selected hosts will execute a build operation on next reboot"
+nil

To obtain a list of hosts where updating their build status etc. didn't work, the controller relied on calling .to_a.delete_if on the host list relation and then checking the result later. This doesn't work under Rails 5, it simply returns the cached result of the relation again as it hasn't actually changed.

Rather than trying to delete entries out of the Rails relation, the code should keep a separate list of hosts that failed the action and use this for display purposes. The code should be clearer as a result too.

Associated revisions

Revision ca59bac0
Added by Dominic Cleal about 1 year ago

fixes #16718 - remove relation.to_a.delete_if in multiple actions

To obtain a list of hosts where updating their build status etc. didn't
work, the controller relied on calling .to_a.delete_if on a relation and
then checking the result later. This doesn't work under Rails 5, it
simply returns the cached result of the relation again as it hasn't
actually changed.

The results are now saved in a different array instead of affecting the
relation.

History

#1 Updated by The Foreman Bot about 1 year ago

  • Status changed from Assigned to Ready For Testing
  • Pull request https://github.com/theforeman/foreman/pull/3898 added

#2 Updated by Dominic Cleal about 1 year ago

  • Status changed from Ready For Testing to Closed
  • % Done changed from 0 to 100

#3 Updated by Dominic Cleal about 1 year ago

  • Release set to 1.14.0

#4 Updated by The Foreman Bot 5 months ago

  • Pull request https://github.com/theforeman/foreman/pull/4613 added

Also available in: Atom PDF