Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify the 'towhat' inclusion validation message on policy creation. #18338

Conversation

coderbydesign
Copy link
Member

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

In this resolved bug / #18032, it was found that providing an invalid towhat value in the payload while creating a Policy record via the API would result in an unclear error message. The received message is the default error message for a failed Rails inclusion validation: "Towhat is not included in the list".

This message can be (and was) mistakenly interpreted to mean towhat was not included in the payload structure, when it's actually referring to the inclusion list it's being validated against.

This helps clarify the messaging to specify the list of valid "towhat" values, following established patterns throughout other model validations, such as:

validates_inclusion_of :content_type, :in => VALID_CONTENT_TYPES, :message => "should be one of #{VALID_CONTENT_TYPES.join(", ")}"
by explicitly setting the inclusion whitelist as part of the error message. This will be particularly helpful when making requests against the API.

This also updates a supportive spec which tests the validity of the policy record.

Links

Steps for Testing/QA

Send a POST API request to create a Policy by running:

$ tools/rest_api.rb post /api/policies

and entering the following payload (invalid towhat):

{
  "action": "create",  
  "resources": [{
    "name": "Invalid Towhat Policy",
    "description": "Invalid Towhat Policy",
    "mode": "control",
    "towhat": "invalidToWhat",      
    "conditions_ids": [ 2000, 3000 ],
    "policy_contents": [{             
      "event_id": 2,
      "actions": [ {"action_id": 1, "opts": { "qualifier": "failure" } } ]
    }]
  }]
}

Response Before:

{
  "error": {
    "kind": "bad_request",
    "message": "Could not create the new policy - Validation failed: MiqPolicy: Towhat is not included in the list",
    "klass": "Api::BadRequestError"
  }
}

Note the error message: "Towhat is not included in the list"

Response After:

{
  "error": {
    "kind": "bad_request",
    "message": "Could not create the new policy - Validation failed: MiqPolicy: Towhat should be one of ContainerGroup, ContainerImage, ContainerNode, ContainerProject, ContainerReplicator, ExtManagementSystem, Host, PhysicalServer, Vm",
    "klass": "Api::BadRequestError"
  }
}

Note the error message: "Towhat should be one of ContainerGroup, ContainerImage, ContainerNode, ContainerProject, ContainerReplicator, ExtManagementSystem, Host, PhysicalServer, Vm"

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

- Explicitly set the inclusion whitelist as part of the error message
when creating a policy with an invalid 'towhat' value to clarify the
expectation, particularly when creating via the API.

- Modify supportive spec which tests the validity of the policy record.
@miq-bot
Copy link
Member

miq-bot commented Jan 8, 2019

Checked commit coderbydesign@5f0ae21 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. ⭐

coderbydesign added a commit to coderbydesign/manageiq_docs that referenced this pull request Jan 8, 2019
Original bug:
https://bugzilla.redhat.com/show_bug.cgi?id=1659899

Messaging bug:
https://bugzilla.redhat.com/show_bug.cgi?id=1663562

With the above bug, it was determined that the documentation - which
was used to test the Policy create action in the API - for the `towhat`
value was not reflective of a valid value for that field.

This change updates the documentation to reflect recent changes which
added an inclusion whitelist for `towhat` values on creation of Policies.

Relevant PRs:
Inclusion list: https://github.com/ManageIQ/manageiq/pull/18032/files
Error messaging: ManageIQ/manageiq#18338
@gtanzillo gtanzillo added the bug label Jan 8, 2019
Copy link
Member

@bdunne bdunne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM

@bdunne bdunne merged commit 7c9925f into ManageIQ:master Jan 14, 2019
@bdunne bdunne added the core label Jan 14, 2019
@bdunne bdunne added this to the Sprint 103 Ending Jan 21, 2019 milestone Jan 14, 2019
@bdunne bdunne self-assigned this Jan 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants