Handle non-existant tag category id when importing a service dialog

[branic]

Importing a Service Dialog with a Tag Control element will fail if the Tag Category ID has changed (e.g. when importing into a different region or when the tag category has been deleted and recreated).

There is no indication of failure in the UI, but a backtrace is generated in the logs

[----] F, [2018-03-27T15:01:04.983489 #2798:2af9b7ad40f4] FATAL -- : Error caught: [ActiveRecord::RecordNotFound] Couldn't find Category with 'id'=173 [WHERE "classifications"."parent_id" = $1]
/home/bevans/.gem/ruby/2.3.5/gems/activerecord-5.0.6/lib/active_record/relation/finder_methods.rb:353:in `raise_record_not_found_exception!'
/home/bevans/.gem/ruby/2.3.5/gems/activerecord-5.0.6/lib/active_record/relation/finder_methods.rb:479:in `find_one'
/home/bevans/.gem/ruby/2.3.5/gems/activerecord-5.0.6/lib/active_record/relation/finder_methods.rb:458:in `find_with_ids'
/home/bevans/.gem/ruby/2.3.5/gems/activerecord-5.0.6/lib/active_record/relation/finder_methods.rb:66:in `find'
/home/bevans/.gem/ruby/2.3.5/gems/activerecord-5.0.6/lib/active_record/querying.rb:3:in `find'
/home/bevans/.gem/ruby/2.3.5/gems/activerecord-5.0.6/lib/active_record/core.rb:151:in `find'
/home/bevans/my-repos/manageiq/app/models/dialog_field_importer.rb:40:in `adjust_category'
/home/bevans/my-repos/manageiq/app/models/dialog_field_importer.rb:34:in `set_category_for_tag_control'
/home/bevans/my-repos/manageiq/app/models/dialog_field_importer.rb:17:in `import_field'
/home/bevans/my-repos/manageiq/lib/services/dialog_import_service.rb:84:in `block in build_dialog_fields'

This PR captures the error condition and looks for the tag category using the name instead of the ID.

Steps for Testing/QA

1. Create a Tag Category and add a couple of values
2. Create a Service Dialog with a Tag Control Element using the previously created category
3. Export the Service Dialog
4. Delete the Service Dialog created in step 3
5. Delete the Tag Category created in step 1
6. Recreate the Tag Category, ensuring that the name and description are exactly the same as in step 1
7. Import the Service Dialog
   7a. Without this PR the import fails with no error for the user
   7b. With this PR the import succeeds and the Service Dialog Tag Control Element has the category assigned

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

[Fryguy]

@eclarizio Please review.

[eclarizio]

Looks fine, can you add a spec for the case where we have an ID, the ID doesn't match, it looks up the category by name, but the description doesn't match? Same with having an ID, ID not matching, and also name can't find one as well? (In that case it doesn't matter if the description matches/doesn't match).

This is going to be a bit nitpicky, but, if you wanted, you could also consolidate some of the setup for those tests into a larger context block that is something like the following (you don't have to do this though, it's just a personal preference on how to organize the specs):

context "when the import file contains a category id" do
  before do
    @existing_category = #...
  end

  let(:options) do
    {
      :category_id          => category_id,
      :category_name        => category_name,
      :category_description => category_description
    }
  end

  context "when the id matches an existing category" do
    let(:category_id) { @existing_category.id }    

    context "when the category exists by name" do
      let(:category_name) { "best_category" }

      context "when the description does not match" do
        let(:category_description) { "worst_category" }

        #...
      end

      context "when the description does match" do
        #...
      end
    end
  end

  context "when the id does not match an existing category" do
    let(:category_id) { @existing_category.id + 1 }

    #...
  end
end

You can look at the context block that currently exists named "when the import file contains a category name with no category_id" to see what I mean. Unfortunately there's a lot of paths to test here so we need to make sure we test them all.

The actual model code change looks fine though, only thing I could suggest is to push the whole if statement down into another private method, maybe, but again that's nitpicking.

Also as the rubocop issue pointed out, can you change the two find_by_name methods to find_by? I know you technically didn't touch the other one, but it would be nice to get it consistent since we're right here anyway.

[branic]

@eclarizio re: find_by_name. In this case the rubocop issue is spurious. The find_by_name method is a defined method of Classification (https://github.com/ManageIQ/manageiq/blob/master/app/models/classification.rb#L335-L338) and can't be replaced by the more generic find_by.

I'll work on making the other changes requested.

[branic]

@eclarizio Can you help me with adding the new tests and refactoring the test cases?

Some of the test cases are failing and it is due to variables not being set, but I'm not seeing why they are not being set.

The test snippet:

      context "when the import file contains a category_id" do
        before do
          @existing_category = Category.create!(:name => "best_category", :description => "best_category")
        end

        let(:options) do
          {
            :category_id          => @existing_category.id,
            :category_name        => @existing_category.name,
            :category_description => @existing_category.description
          }
        end

...

        context "when the category_id does not match an existing category" do
          let(:category_id) { 1_234_567 }

...

          context "when the category_name does not match the existing category" do
            let(:category_name) { "worst_category" }

            it "returns nil" do
              pp dialog_field
              dialog_field_importer.import_field(dialog_field)
              expect(DialogFieldTagControl.first.category).to eq(nil)
            end
          end
        end
      end

The when the category_name does not match the existing category test is failing. I added a pp to the test to see what values dialog_field contains and found that the category_id and category_name are not being overridden by the let statements.

{"type"=>"DialogFieldTagControl",
 "name"=>"Something",
 "label"=>"Something else",
 "resource_action"=>
  {"ae_namespace"=>"Customer/Sample",
   "ae_class"=>"Methods",
   "ae_instance"=>"Testing"},
 "options"=>
  {:category_id=>6000000000151,
   :category_name=>"best_category",
   :category_description=>"best_category"},
 "dialog_field_responders"=>["foo_that_gets_ignored"]}

Any help or pointers you can give me would be greatly appreciated.

[eclarizio]

@branic I wrote some of this on gitter but I'll put it here as well. It looks like in the options hash you passed in the @existing_category stuff instead of the lets that you defined.

The options hash I'm guessing is being used by the dialog_field in question to be imported. The @existing_category is only there so that something exists in the database to potentially find.

The options hash should probably look like this:

let(:options) do
  {
    :category_id => category_id,
    :category_name => category_name,
    :category_description => category_description
  }
end

Then you would adjust those three parameters with lets in each context. Sometimes they'll match the @existing_category, and sometimes they won't, depending on what you need to test, for example in the contexts you've provided above, the ID could be set to @existing_category.id + 1 just in case for some random reason the @existing_category that gets created has its ID set to 1,234,567. The category_name is fine.

For the context of "when the category id matches the existing category", that's where your let would look like:
let(:category_id) { @existing_category.id }

Then when dialog_field.options gets evaluated, it would use the proper category_id.

[eclarizio]

👍

[d-m-u]

@branic I think this fixes https://bugzilla.redhat.com/show_bug.cgi?id=1566266

[branic]

@d-m-u You are correct. This PR does fix that bug.

[d-m-u]

@gmcculloug please review 🌵

[gmcculloug]

@branic Thanks for the PR, this is a great start. I would advise against using error conditions as control flow. I think a simple change to use find_by gets us what we want. The other change I am proposing below is that if the category name does not match opts[:category_name] do the lookup by opts[:category_name] instead of returning nil.

Let me know what you think about changing the method to the following:

def find_category(opts)
  if opts[:category_id]
    cat = Category.find_by(:id => opts[:category_id])
    return cat if cat.try(:name) == opts[:category_name]
  end
  Category.find_by_name(opts[:category_name])
end

[branic]

@gmcculloug I've changed the method as you suggested.

[gmcculloug]

Thanks @branic. You can safely ignore the find_by_name warning as the Classification model exposes that method, it is not using the Rails dynamic finder logic.

I would just ask that you squash the commits and then I think we are good.

[branic]

@gmcculloug Thanks. I've squashed the commits.

[miq-bot]

Checked commit https://github.com/branic/manageiq/commit/04d4ba343d76e8b8eb97b062c90c59845b6ad905 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 1 offense detected

app/models/dialog_field_importer.rb

• ❗ - Line 48, Col 5 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_name.

[chrispy1]

@miq-bot add_label blocker

[simaishi]

Gaprindashvili backport details:

$ git log -1
commit a243a24507e5217237b7c85422e5a625dd84567d
Author: Greg McCullough <[email protected]>
Date:   Mon Apr 16 16:32:43 2018 -0400

    Merge pull request #17237 from branic/service_dialog_import_tag_category_id
    
    Handle non-existant tag category id when importing a service dialog
    (cherry picked from commit 7c2ef9c5ed0465560607f6d1b1737c0fca493659)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1568156