Fix error importing Widget on Custom Report page

[bmclaughlin]

Fixes an exception to a much more user friendly flash message.

@miq-bot add_labels bug, core, ui

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

[gmcculloug]

@bmclaughlin I'm ok with this check, but it looks like doing this for MiqReport (ManageIQ/manageiq-ui-classic#2227) would negate the need for the rescue below on line 59: # for the legacy MiqReport as we would never get to that code.

@gtanzillo Can you comment on this or recommend someone?

[gmcculloug]

Thinking more about this maybe a better approach is to create a validate_import_data method that gets called as part of the import method instead of the caller needing to pass the class.

From what I can tell three classes use this mixin: MiqReport, MiqWidget and MiqPolicy. Both MiqReport and MiqWidget expect the root of the import to be the same class name. However, MiqPolicy supports MiqPolicy, MiqPolicySet and MiqAlert.

The mixin could contain the validation method that checks the the name matches the class name and the classes would expose a constant that defines the IMPORT_CLASS_NAMES.

Something like this:

    def import(fd, options = {})
      fd.rewind # ensure to be at the beginning as the file is read multiple times
      begin
        reps = YAML.load(fd.read)
        validate_import_data(reps)
      rescue Psych::SyntaxError => err
        _log.error("Failed to load from #{fd}: #{err}")
        raise "Invalid YAML file"
      end

      return reps, import_from_array(reps, options)
    end

    def validate_import_data(data)
      data_class_name = data.first.keys.first

      if !self::IMPORT_CLASS_NAMES.include?(data_class_name)
        raise _("Incorrect format: Expected #{self::IMPORT_CLASS_NAMES.join(", ")} and received #{data_class_name}")
      end
    end

Then in app/models/miq_policy/import_export.rb (also MiqReport and MiqWidget)

module MiqPolicy::ImportExport
  extend ActiveSupport::Concern

  IMPORT_CLASS_NAMES = %w(MiqPolicy MiqPolicySet MiqAlert)
...

@gtanzillo @eclarizio Thoughts?

[eclarizio]

@gmcculloug Yeah, I like this idea, and I would even further look into a way to incorporate the already existing WidgetImportValidator class (or create a new one if WidgetImport is too specific?).

[bmclaughlin]

@gmcculloug, implemented your idea, much better solution than what I had originally. Thanks!

@eclarizio, or perhaps validate_import_data should be renamed to validate_import_class to better identify what the method is accomplishing? I didn't see a good way to incorporate your feedback, I think you're looking at this from a different angle than I am.

[eclarizio]

@bmclaughlin My original point was mostly directed at attempting to encapsulate the validation in a different class, but I think since we're not really validating the data, just ensuring that the class is correct, I agree with your other point of changing the name of the method to validate_import_class instead of validate_import_data. Or maybe validate_import_data_class, but either way, I think validate_import_data implies it does more than it really does.

[gmcculloug]

@bmclaughlin Specs are failing because they still references the old method name.

[gmcculloug]

Missed one. 😱

[bmclaughlin]

So close...

[gmcculloug]

For our final act, can you drop the first commit since it's not being used and squash the others?

[gmcculloug]

Somehow you pulled in one of Jillian's PRs!?!

[miq-bot]

Checked commit bmclaughlin@b26b013 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
5 files checked, 0 offenses detected
Everything looks fine. 🏆

[eclarizio]

👍

[gtanzillo]

LGTM 👍

[simaishi]

Fine backport details:

$ git log -1
commit afd835e9fc432d98c789740216455f808d052b18
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Mon Oct 9 09:24:02 2017 -0400

    Merge pull request #16034 from bmclaughlin/import-custom-report-error
    
    Fix error importing Widget on Custom Report page
    (cherry picked from commit 16602fac1c78ba1ff09da8035e489a4c4453895c)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1500029