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.

Sign up for GitHub

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

Jump to bottom

Provide message when tag assignment failed #20216

Merged
merged 2 commits into from
Jun 17, 2020
Merged

Provide message when tag assignment failed #20216

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
76c59d2
return what went wrong if assigning tag failed
yrudman May 27, 2020
d9df6b4
added test coverage for Classification.classify
yrudman May 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions app/models/classification.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,11 +98,16 @@ def ns
# rubocop:disable Style/NumericPredicate

def self.classify(obj, category_name, entry_name, is_request = true)
cat = Classification.lookup_by_name(category_name, obj.region_id)
unless cat.nil?
ent = cat.find_entry_by_name(entry_name, obj.region_id)
ent.assign_entry_to(obj, is_request) unless ent.nil? || obj.is_tagged_with?(ent.to_tag, :ns => "none")
end
cat = Classification.find_by_name(category_name, obj.region_id)
Copy link
Member

Choose a reason for hiding this comment

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

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.

return " - FAILED. Tag category '#{category_name}' not found in region #{obj.region_id}" if cat.nil?
Copy link
Member

@Fryguy Fryguy May 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Member

Choose a reason for hiding this comment

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

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")

Copy link
Contributor Author

@yrudman yrudman May 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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


ent = cat.find_entry_by_name(entry_name, obj.region_id)
return " - FAILED. Tag name '#{entry_name}' not found in region #{obj.region_id}" if ent.nil?

return " - FAILED. Object already tagged with tag namespace set to 'none'" if obj.is_tagged_with?(ent.to_tag, :ns => "none")

ent.assign_entry_to(obj, is_request)
" - SUCCESS."
end

def self.unclassify(obj, category_name, entry_name, is_request = true)
Expand Down
29 changes: 29 additions & 0 deletions spec/models/classification_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,35 @@
FactoryBot.create(:classification_tag, :name => "multi_entry_2", :parent => parent)
end

describe ".classify" do
Copy link
Member

Choose a reason for hiding this comment

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

thanks for the tests.

makes understanding the goals that much easier

let(:entry) { "test_entry" }
let(:category) { "test_category" }
before { @vm = FactoryBot.create(:vm) }

it "returns detailed message if tag category not found" do
category = "Hello, World"
msg = Classification.classify(@vm, category, entry)
expect(msg).to include("Tag category '#{category}' not found in region #{@vm.region_id}")
end

it "returns detailed message message if tag entry not found" do
entry = "Hello, World"
msg = Classification.classify(@vm, category, entry)
expect(msg).to include("Tag name '#{entry}' not found in region #{@vm.region_id}")
end

it "returns detailed message message if object already tagged with tag namespace set to 'none'" do
allow(@vm).to receive(:is_tagged_with?)
allow(@vm).to receive(:is_tagged_with?).with("/managed/test_category/test_entry", :ns => "none").and_return(true)
msg = Classification.classify(@vm, category, entry)
expect(msg).to include("Object already tagged with tag namespace set to 'none'")
end

it "assign tag entry to object if tag category and tag name exist and returns 'SUCCESS'" do
expect(Classification.classify(@vm, category, entry)).to include("SUCCESS")
end
end

context "#destroy" do
it "a category deletes all entries" do
cat = Classification.lookup_by_name("test_category")
Expand Down