Validate towhat policy field

[jvlcek]

https://bugzilla.redhat.com/show_bug.cgi?id=1435780

Invalid values for policy field towhat are not recognized, result in the policy being
created using the API.

The validations for mode were added in [PR 14519](#14519

The list of valid values for towhat were arrived at by examination of what is supported via
the UI tree_builder_policy.rb

Steps for Testing/QA

Attempt to create a policy with the following data:

    {
      "action": "create",  
      "resources": [{
        "name": "test policy bobsyouruncle",
        "description": "Desc bobsyouruncle",
        "mode": "control",
        "towhat": "bobsyouruncle",      
        "conditions_ids": [ 2000, 3000 ],
        "policy_contents": [{             
          "event_id": 2,
          "actions": [ {"action_id": 1, "opts": { "qualifier": "failure" } } ]
        }]
      }]
    }

e.g.:

$MY_MIQ_REPO/manageiq/tools/rest_api.rb  -l https://10.8.99.50 get /api/policies

<when prompted enter>

        {
          "action": "create",  
          "resources": [{
            "name": "test policy bobsyouruncle",
            "description": "Desc bobsyouruncle",
            "mode": "control",
            "towhat": "bobsyouruncle",      
            "conditions_ids": [ 2000, 3000 ],
            "policy_contents": [{             
              "event_id": 2,
              "actions": [ {"action_id": 1, "opts": { "qualifier": "failure" } } ]
            }]
          }]
        }

[jvlcek]

@miq-bot add_label bug

[jvlcek]

@miq-bot assign @bdunne

[jvlcek]

@miq-bot add_label hammer/yes

[bdunne]

Can this be sorted alphabetically?

[jvlcek]

👍 will do

[bdunne]

You don't need to save the record to test this, you can

expect(FactoryGirl.build(:miq_policy, :towhat => "Host", :mode => "compliance").valid?).to be true

[bdunne]

Should mode even be tested here since it didn't change in this PR?

[bdunne]

I'm usually not a huge fan of this syntax, but you could also

expect(FactoryGirl.build(:miq_policy, :towhat => "Host")).to be_valid

[jvlcek]

@bdunne Thank you for the input. I'll have the new test focus on towhat

I like the syntax the way it is and if you don't have strong feelings about it I'd like to leave it as is, mostly because it's more consistent with the tests through the spec.

Let me know if you are OK with leaving the test as is.

Thank you. JoeV

[bdunne]

I guess I don't see the point of writing to the database if we just want to make sure the instance is valid. Also, using FactoryGirl makes it much more obvious that you're only testing :towhat and not any other validation.

[jrafanie]

I guess the question is: are you testing that it's valid or testing if it raises an exception. If you're testing validity: you don't need to create the record, you can do as @bdunne suggests. If you're testing exception raising, you need to create!

It looks like your testing validity so create! isn't needed here. It's minor and I know other code does this but it's fine to mix patterns in the specs until you can convert the rest to the new way.

[jvlcek]

@bdunne and @jrafanie Ah I see the reasoning now. Thank you for explaining. Will do.

JoeV

[bdunne]

yeah, I prefer expecting the specific thing (validity) rather than the side effect of rails raising an error when calling create! on an invalid instance.

[bdunne]

Similar here

expect(FactoryGirl.build(:miq_policy, :towhat => "BobsYourUncle")).not_to be_valid

[jvlcek]

@bdunne Here again, if you feel strongly I'd be glad to change this test but for me it's much easier to quickly grasp what the test is doing, that a particular exception is expected and it's more consistent with existing tests in the spec.

Let me know if you are OK with it as is.

Thank you for the review feedback.
JoeV

[jrafanie]

I'd be ok with testing create! if you're concerned with a custom validation message but since this is out of the box validation messages, I think you can just do thing.new + thing.valid? and maybe even thing.errors[:towhat].blank? or something like that.

[jvlcek]

@jrafanie Thank you for explaining. Will do.

JoeV

[jvlcek]

@bdunne and @jrafanie Now that I see the result I appreciate the simplicity of this version of the code!
Thank you for your input.

JoeV

[miq-bot]

Checked commits jvlcek/manageiq@d344e43~...a9bdbd9 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

[simaishi]

Hammer backport details:

$ git log -1
commit 28ab0177beb315a791af3d60a7ca5ae1cfc5c29a
Author: Brandon Dunne <brandondunne@hotmail.com>
Date:   Fri Sep 28 14:38:24 2018 -0400

    Merge pull request #18032 from jvlcek/bz_1435780_tow_hat_validations
    
    Validate towhat policy field
    
    (cherry picked from commit 52704166f4dc48ccc2ced8c6bac0e8001e8d786f)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1435780