Provide message when tag assignment failed

[yrudman]

ISSUE:
if tag category or tag name misspelled when executing API call to assign tag than return message did not indicate what went wrong.

with ManageIQ/manageiq-api#843 fixes BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1792106

Example:
curl --user admin:smartvm -i -X POST -d '{"action":"assign","resources":[{"category":"team","name":"it_pnp"}]}' http://localhost:3000/api/vms/54/tags
BEFORE:
{"results":[{"success":false,"message":"Assigning Tag: category:'team' name:'it_pnp'", ...}
AFTER:
{"results":[{"success":false,"message":"Assigning Tag: category:'team' name:'it_pnp' - FAILED. Tag category 'team' not found in region 0", ...}

cross-repo: ManageIQ/manageiq-cross_repo-tests#133

[miq-bot]

Checked commits yrudman/manageiq@76c59d2~...d9df6b4 with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. 👍

[kbrock]

this fixes the bz and it doesn't adversely effect the other code

[kbrock]

this does feel like ui code.

also programmatically, it is difficult to know if this succeeded or failed without string compare.

nice thing about this code is that it doesn't blow up our other classes that assume this method will not throw an exception.

[kbrock]

thanks for the tests.

makes understanding the goals that much easier

[Fryguy]

Are these going to make their way to the UI? If so they need to be I18N'd properly.

---

Separately, I think it's weird that this method returns failure strings.

I think better interfaces would be

• an Exception with the error and the caller catches that and presents it
• on success return true; on failure return [false, message] where message is the actual failure message without the leading - FAILURE. The caller does success, message = Classification.classify(...) and is then responsible for checking the success value, and then presenting the message with whatever prefix it needs. This is actually a pattern we follow elsewhere in the code.

[kbrock]

@Fryguy a number of places including automate rely upon the fact that exceptions are not thrown.

Do you propose we update automate to handle a thrown exception here?

We could add these rescues in the various places and then merge the change here.

going the way you proposed is how Yuri originally did it. But we reverted because so much stuff seemed to break.

[yrudman]

an Exception with the error and the caller catches that and presents it

this PR #20197 was reverted since it affect automation

Are these going to make their way to the UI? If so they need to be I18N'd properly.

return of method was not checked previously and made use of in ManageIQ/manageiq-api#843. In that place returned message was not go over I18N' before.
If it should be I18N'd properly may be it should go to another PR ?

[gtanzillo]

What do you think about creating an Errors object and returning that? It a hack because you don't have an actual object to add it to but, at least it would be more model like

Something like -

e=ActiveModel::Errors.new(Classification.new)
e.add(:category, "'#{category_name}' not found in region #{obj.region_id")

[yrudman]

it looks slightly over-designed - create new object only to use as wrapper for error string and on consumer side check if that object returned (and not nil) and extract that string from object

I agree that returned string "- FAILED..." does looks strange,
But, it accomplished request; I think that in order to make required improvement better - too many changes / refactoring needed, which may introduce more harm than added value from improvement

[yrudman]

@miq-bot add-label jansa/yes?

[simaishi]

Jansa backport details:

$ git log -1
commit cf1cad141f82bee36e344ed2baf889f7f5963086
Author: Gregg Tanzillo <[email protected]>
Date:   Wed Jun 17 13:41:55 2020 -0400

    Merge pull request #20216 from yrudman/provide-message-when-tag-assignment-failed

    Provide message when tag assignment failed

    (cherry picked from commit 7cebd4e2a5ec5dbfc96d8e02fd2c754cb40ca3ca)