add dialog info when import/export custom buttons #20137

borisko123 · 2020-05-07T16:28:29Z

Fixes #20124

Current flow for import/export custom buttons ignores possible reference of the button to service dialog.
It is very inconvenient, since after import one need to add this reference manually in UI.
What's done:

files custom_buttons.rb modified both for import and export to include stuff for handle dialog data
(actually added dialog label)
modified spec tests for new functionality

How to run unit tests:

rspec spec/lib/task_helpers/imports/custom_buttons_spec.rb
rspec spec/lib/task_helpers/exports/custom_buttons_spec.rb

How to test manually on real data:

Add manually the domain, service dialog, button group, the button
Connect button to service dialog
Create directories for import/export, say contrib_export/buttons/
Export button:
bundle exec rake evm:export:custom_buttons -- --directory contrib_export/buttons/
Remove button
Import button:
bundle exec rake evm:import:custom_buttons -- --source contrib_export/buttons/
Make sure the button is linked to dialog

(see screenshot)

The screen

lfu · 2020-05-08T13:29:46Z

@borisko123 Please address the rubocop warnings.

borisko123 · 2020-05-09T10:06:33Z

@borisko123 Please address the rubocop warnings.

borisko123 · 2020-05-10T13:02:39Z

@lfu seems all clean now

lfu · 2020-05-11T13:41:35Z

@gmcculloug Any idea why dialog was excluded from import/export in #17699? What was the concern?

gmcculloug · 2020-05-11T13:49:08Z

As I recall, we were following scripts that already existed (side the MIQ repo) which processed custom buttons and dialogs separately.

If you add this you'll just want to consider how you handle if the dialog already exists. Dialogs can be re-used by different buttons and/or services.

lfu · 2020-05-11T17:05:31Z

@borisko123 Thanks for the update.
What kind of dialog info is supposed to be added in this PR? Is it the dialog object with all its tabs and fields? Seems only dialog label is added in the export yaml file.

Please take a look at this file for importing a dialog and this for exporting. Hope they can give you some ideas.

borisko123 · 2020-05-11T17:07:39Z

Dialog was concerned in rhconsulting scripts. The code for Miq was ported from there. It worked well. The dialog support was simply missed,IMHO

…

On Mon, May 11, 2020, 16:49 Greg McCullough ***@***.***> wrote: As I recall, we were following scripts that already existed (side the MIQ repo) which processed custom buttons and dialogs separately. If you add this you'll just want to consider how you handle if the dialog already exists. Dialogs can be re-used by different buttons and/or services. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#20137 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AOOOCDSTP2U35LLOUSGIJJTRQ766JANCNFSM4M3PEIEQ> .

borisko123 · 2020-05-11T17:16:05Z

Only dialog name is exported. In import, we lookup the dialog by this name. In case it exists, its ID is added to resource_action. It is exactly the same as add dialog manually by UI.

…

On Mon, May 11, 2020, 20:05 Lucy Fu ***@***.***> wrote: @borisko123 <https://github.com/borisko123> Thanks for the update. What kind of dialog info is supposed to be added in this PR? Is it the dialog object with all its tabs and fields? Seems only dialog label is added in the export yaml file. Please take a look at this file <https://github.com/ManageIQ/manageiq/blob/cddcb42c99cc981f9ab74ba527eee58acc38a9a1/lib/services/dialog_import_service.rb> for importing a dialog and this <https://github.com/ManageIQ/manageiq/blob/master/lib/task_helpers/dialog_exporter.rb> for exporting. Hope they can give you some ideas. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#20137 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AOOOCDVE7VLEOE74FKXGOH3RRAV6VANCNFSM4M3PEIEQ> .

borisko123 · 2020-05-11T17:18:59Z

Forgot to mention that dialog should already exist!. So we deal with existed ID only. Exactly as in rhconsulting code

…

On Mon, May 11, 2020, 20:15 Boris Kozel ***@***.***> wrote: Only dialog name is exported. In import, we lookup the dialog by this name. In case it exists, its ID is added to resource_action. It is exactly the same as add dialog manually by UI. On Mon, May 11, 2020, 20:05 Lucy Fu ***@***.***> wrote: > @borisko123 <https://github.com/borisko123> Thanks for the update. > What kind of dialog info is supposed to be added in this PR? Is it the > dialog object with all its tabs and fields? Seems only dialog label is > added in the export yaml file. > > Please take a look at this file > <https://github.com/ManageIQ/manageiq/blob/cddcb42c99cc981f9ab74ba527eee58acc38a9a1/lib/services/dialog_import_service.rb> > for importing a dialog and this > <https://github.com/ManageIQ/manageiq/blob/master/lib/task_helpers/dialog_exporter.rb> > for exporting. Hope they can give you some ideas. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#20137 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AOOOCDVE7VLEOE74FKXGOH3RRAV6VANCNFSM4M3PEIEQ> > . >

lfu · 2020-05-11T17:42:31Z

What if a dialog exists with the same name but different contents? Dialogs can be re-used by different buttons and/or services. It seems not right to use the existing dialog just because it has the same name.
@Fryguy @gmcculloug @gtanzillo Suggestion?

borisko123 · 2020-05-12T07:33:42Z

From Dialog model: the name/label is unique within region:
validates :name, :unique_within_region => true
In case we will filter our objects (button, dialog,reasource_action),by region, all should work as expected.
Actually, the original rhconsulting (https://github.com/rhtconsulting/cfme-rhconsulting-scripts/blob/master/rhconsulting_buttons.rake) script had this functionality.
Somehow I missed this.

So I suggest to add this functionality (filtering ) and then all should work.

lfu · 2020-05-12T19:34:10Z

If the user really wants to use an existing dialog just because it has the same name, we can add one more command line option like --connect_dialog_by_name true to import.
Existing dialog with the same name would be connected to the custom button if the option is specified. Otherwise, the dialog would not be used.
Hopefully the user would be aware of what is going to happen with the specified option.

@gmcculloug @gtanzillo What do you think of the new option? Any suggestion for the option name?

borisko123 · 2020-05-13T06:11:57Z

If the user really wants to use an existing dialog just because it has the same name, we can add one more command line option like --connect_dialog_by_name true to import.
Existing dialog with the same name would be connected to the custom button if the option is specified. Otherwise, the dialog would not be used.
Hopefully the user would be aware of what is going to happen with the specified option.

It seems this option should be set to true by default.
More often we want to import 'as-is'

gtanzillo · 2020-05-13T15:28:32Z

It seems this option should be set to true by default.
More often we want to import 'as-is'

I would also agree that it would be more often that the user would want to use an existing dialog with the same name. However, i think would still be safer if that intention was explicitly specified on the command line.

borisko123 · 2020-05-13T17:14:45Z

Ok, this option will be set to false. If there are no other objections. I will implement region specific filtering, add new option for import task and add new commit to PR, ok?

…

On Wed, May 13, 2020, 18:28 Gregg Tanzillo ***@***.***> wrote: It seems this option should be set to true by default. More often we want to import 'as-is' I would also agree that it would be more often that the user would want to use an existing dialog with the same name. However, i think would still be safer if that intention was explicitly specified on the command line. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#20137 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AOOOCDQ3UI624WN3T7VCUKTRRK4DBANCNFSM4M3PEIEQ> .

lfu · 2020-05-13T17:55:38Z

Please modify the code where the new feature is added and keep other places untouched.
Then use git rebase to squash your commits into a clean commit without the intermediate steps.

lib/task_helpers/imports/custom_buttons.rb

borisko123 · 2020-05-13T19:25:20Z

@lfu do you mean that some test files have .yml extensions and not .yaml? This is done intentionally, to not include them in glob. They are used individually in tests , by using full name with certain extensions explicitly. This exists in original code as well.

…

On Wed, May 13, 2020, 20:58 Lucy Fu ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/task_helpers/imports/custom_buttons.rb <#20137 (comment)>: > module TaskHelpers class Imports class CustomButtons def import(options) return unless options[:source] - glob = File.file?(options[:source]) ? options[:source] : "#{options[:source]}/*.yaml" + + glob = File.file?(options[:source]) ? options[:source] : "#{options[:source]}/*.yaml*" We should keep the import file as .yaml. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#20137 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AOOOCDTRGQJXDUOIRXRTB73RRLNWFANCNFSM4M3PEIEQ> .

lfu · 2020-05-13T19:41:47Z

> glob = File.file?(options[:source]) ? options[:source] : "#{options[:source]}/*.yaml”

What I said is this line should not be changed.

borisko123 · 2020-05-17T18:23:43Z

Please modify the code where the new feature is added and keep other places untouched.
Then use git rebase to squash your commits into a clean commit without the intermediate steps.
@lfu
please check now

spec/lib/task_helpers/imports/custom_buttons_spec.rb

lib/task_helpers/exports/custom_buttons.rb

lib/task_helpers/imports/custom_buttons.rb

lfu · 2020-05-26T17:34:56Z

lib/task_helpers/imports/custom_buttons.rb

          ActiveRecord::Base.transaction { obj_hash.each { |obj_def| create_object(*obj_def) } }
        end

+        def find_or_create_object(class_name, obj)


In my opinion, this is to import custom buttons where they don't exist.
In case custom buttons exist, they may use Edit task in UI to modify them.
This new method is not necessary.

@lfu, this method is my main enhancement.
Original code raises exception when button exists.
This is unacceptable.
The import may occurs many times on the already existing buttons(in development and even in production), with or without some button changing.
In both cases the code should replace the button, optionally updating the button attributes.
The 'rhconsulting' script, which was used for porting, had this behaviour.
Your idea of using UI, seems strange to me, as all the goal of import/export scripts is to make things without UI.
Please re-think your opinion

The import may occurs many times on the already existing buttons(in development and even in production), with or without some button changing.

There are couple of things here.

Why do you need import again if the buttons exist?

name alone is not enough to identify a button.

Not sure if it is a good practice to update an existing button just because it happens to have the same name. What if the buttons are owned by other users?

It is needed, for example, during development, when import may be done multiple times from the same or different developers. Personally, had problems with this. Currently this will cause error,
and one need to delete manually ALL the button(s) in import directory, prior to import, or update yaml file. This is far from being convenient.

You are right. Button should be looked up by class and apply_id.
This may be added to code

For user's support some logic exists (see check_user method). This method is untouched in PR.
It set user from export data, or 'admin'. Maybe it may be improved.
Summary:
A. Importing existed buttons needs additional efforts.
In my opinion, it is nice to have.
In case ManageIQ team has another opinion. it is not worth to implement this at all
B. This PR includes 2 independent features: dialog support and import existing buttons.
In any way, since dialog support code is already mostly approved by you, I suggest retaining only corresponding code (for dialog support) in this PR and continue to review.
C. Optionally, open issue about importing existing buttons.

lfu · 2020-05-26T17:37:08Z

lib/task_helpers/imports/custom_buttons.rb

+          end
+        end
+
+        def populate_action(item, assoc)


These two methods added here are not necessary. Please move the code into add_associations.

lfu · 2020-06-01T13:32:15Z

@borisko123 I like your idea to keep this PR just for dialog support. Let's open another issue for importing existing buttons.

borisko123 · 2020-06-04T09:35:47Z

@borisko123 I like your idea to keep this PR just for dialog support. Let's open another issue for importing existing buttons.

@lfu done PR just for dialog, please check
New issue will open soon.

lfu · 2020-06-04T15:06:40Z

spec/lib/task_helpers/imports/custom_buttons_spec.rb

          assert_imports_only_custom_button_set_one
        end
      end
    end
  end

+  def assert_dialog_is_set(connect)
+    btn1 = CustomButton.find_by(:name => custom_button_1_name)
+    expect(btn1).to be


This line seems not right.

button_1 = CustomButton.find_by(:name => "button 1") expect(button_1).not_to be_nil if connect expect(button_1.resource_action.dialog.id).to eq(test_dialog_2.id) else expect(button_1.resource_action.dialog).to be_nil end end

Prefer variable name button_1 over btn1.

@borisko123 Can you fix this line expect(btn1).to be?

Hi @lfu , what is wrong with this line?
It asserts that button exists...

from : https://relishapp.com/rspec/rspec-expectations/v/3-9/docs/built-in-matchers/be-matchers
expect(obj).to be # passes if obj is truthy (not nil or false)

lfu · 2020-06-04T15:06:56Z

spec/lib/task_helpers/imports/custom_buttons_spec.rb

        end
      end

      context "with existing identical buttons" do
        it 'should not import anything' do
          TaskHelpers::Imports::CustomButtons.new.import(options)
-          assert_raises_import_error


What caused this change?

Never mind.

lfu · 2020-06-04T15:08:15Z

spec/lib/task_helpers/imports/custom_buttons_spec.rb

          assert_imports_only_custom_button_set_one
        end
      end
    end
  end

+  def assert_dialog_is_set(connect)
+    btn1 = CustomButton.find_by(:name => custom_button_1_name)


Minor. But can we have a better variable name here? Like button or button_1?

lfu · 2020-06-08T11:44:22Z

spec/lib/task_helpers/imports/custom_buttons_spec.rb

-  let(:options)                       { {:source => source} }
+  let(:connect_dialog_by_name) { true }
+  let(:options) { {:source => source, :connect_dialog_by_name => connect_dialog_by_name} }
+  let(:button_1) { 'button 1' }


This variable never changes. Can be removed.

right. :button_1 is constant here, defined via let, and used at line 85:
btn1 = CustomButton.find_by(:name => button_1)
Do you want me to use literal instead? like
btn1 = CustomButton.find_by(:name => 'button 1')
Using 'let' to define constant is found as preffered way: see
https://stackoverflow.com/questions/5230070/scope-constants-to-an-rspec-context

In my opinion, there is no need to define a variable here whose value doesn't change between the test cases.

Agree that btn1 = CustomButton.find_by(:name => 'button 1') is clearer since button_1 does not seem to be used anywhere else. Which code sets the name to button 1? Perhaps both of those can use the same variable, such as button_1_name.

@lfu @chessbyte done

@lfu

-fix error in spec example -the test result depends on file order -code style review -modified according to miq-bot request -fix rubocop offence -add new flag (for dialog), modify specs - after review, simplify export dialog (@lfu, Tempfile creation (@gmcculloug) - refactor to include only dialog support stuff - rubocop issues - review literal button name without let - fix to be (be_an(CustomButton)

miq-bot · 2020-06-17T10:25:25Z

Checked commit borisko123@a1fa28e with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
4 files checked, 0 offenses detected
Everything looks fine. 🍰

lfu · 2020-06-17T16:06:00Z

@gtanzillo LGTM

borisko123 requested review from Fryguy and gtanzillo as code owners May 7, 2020 16:28

gtanzillo assigned lfu May 7, 2020

gtanzillo added the enhancement label May 7, 2020

borisko123 closed this May 9, 2020

borisko123 reopened this May 9, 2020

lfu reviewed May 13, 2020

View reviewed changes

lib/task_helpers/imports/custom_buttons.rb Outdated Show resolved Hide resolved

borisko123 force-pushed the add_dialog_info_for_import_export_custom_buttons branch 2 times, most recently from 44aa954 to 50fbbde Compare May 17, 2020 15:52

borisko123 mentioned this pull request May 18, 2020

Add rake task to export custom buttons #17699

Merged

gmcculloug reviewed May 18, 2020

View reviewed changes

spec/lib/task_helpers/imports/custom_buttons_spec.rb Outdated Show resolved Hide resolved

lfu reviewed May 22, 2020

View reviewed changes

lib/task_helpers/exports/custom_buttons.rb Show resolved Hide resolved

lfu reviewed May 22, 2020

View reviewed changes

lib/task_helpers/imports/custom_buttons.rb Show resolved Hide resolved

borisko123 force-pushed the add_dialog_info_for_import_export_custom_buttons branch 3 times, most recently from d034b34 to 7069262 Compare May 25, 2020 17:13

lfu reviewed May 26, 2020

View reviewed changes

borisko123 force-pushed the add_dialog_info_for_import_export_custom_buttons branch 2 times, most recently from af0240a to 2d8da36 Compare June 4, 2020 08:57

lfu reviewed Jun 4, 2020

View reviewed changes

borisko123 force-pushed the add_dialog_info_for_import_export_custom_buttons branch from 2d8da36 to 257466c Compare June 6, 2020 14:54

lfu reviewed Jun 8, 2020

View reviewed changes

borisko123 force-pushed the add_dialog_info_for_import_export_custom_buttons branch from 257466c to da43b07 Compare June 11, 2020 18:30

borisko123 force-pushed the add_dialog_info_for_import_export_custom_buttons branch from da43b07 to a1fa28e Compare June 17, 2020 10:20

lfu approved these changes Jun 17, 2020

View reviewed changes

gtanzillo merged commit e69f19f into ManageIQ:master Jun 17, 2020

Fryguy mentioned this pull request May 3, 2021

[RFE] evm:import:custom_buttons with associated dialog #19731

Closed

NickLaMuro mentioned this pull request Jun 1, 2021

[Exports::CustomButtons] Refactor/Simplify #21255

Merged

add dialog info when import/export custom buttons #20137

add dialog info when import/export custom buttons #20137

Conversation

borisko123 commented May 7, 2020 • edited Loading

lfu commented May 8, 2020

borisko123 commented May 9, 2020

borisko123 commented May 10, 2020

lfu commented May 11, 2020

gmcculloug commented May 11, 2020

lfu commented May 11, 2020

borisko123 commented May 11, 2020 via email

borisko123 commented May 11, 2020 via email

borisko123 commented May 11, 2020 via email

lfu commented May 11, 2020

borisko123 commented May 12, 2020

lfu commented May 12, 2020

borisko123 commented May 13, 2020

gtanzillo commented May 13, 2020

borisko123 commented May 13, 2020 via email

lfu commented May 13, 2020

borisko123 commented May 13, 2020 via email

lfu commented May 13, 2020 via email • edited Loading

borisko123 commented May 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lfu commented Jun 1, 2020

borisko123 commented Jun 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

borisko123 Jun 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Jun 17, 2020

lfu commented Jun 17, 2020

borisko123 commented May 7, 2020 •

edited

Loading

lfu commented May 13, 2020 via email •

edited

Loading

borisko123 commented May 17, 2020 •

edited

Loading

borisko123 commented Jun 4, 2020 •

edited

Loading

borisko123 Jun 16, 2020 •

edited

Loading