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.

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

Added UI support to add/edit OVF Template type Catalog Item(s) #7370

Merged
merged 1 commit into from
Oct 9, 2020

Conversation

h-kataria
Copy link
Contributor

@h-kataria h-kataria commented Oct 1, 2020

This is Ruby/AJAX form similar with some special parameters for OVF Template provisioning.

ManageIQ/manageiq#19718

Depends on ManageIQ/manageiq#20636

@lfu please review/test

//cc @gtanzillo

Screenshot from 2020-10-01 11-17-34

Screenshot from 2020-10-01 13-47-14

Screenshot from 2020-10-01 13-47-45

@@ -242,7 +242,7 @@ def atomic_form_field_changed
if params[:st_prov_type] || (params[:display] && @edit[:new][:st_prov_type].starts_with?("generic"))
page.replace("form_div", :partial => "st_form")
end
if params[:display] || params[:template_id] || params[:manager_id]
if params[:display] || params[:template_id] || params[:manager_id] || params[:ovf_template_id]
Copy link
Member

@chessbyte chessbyte Oct 1, 2020

Choose a reason for hiding this comment

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

would be nice if this complex if statement (used twice in this file) can be extracted out into a tiny method named so that a developer can better understand the intent. So, instead of
if params[:display] || params[:template_id] || params[:manager_id] || params[:ovf_template_id]
it would say something like
if automate_catalog_needed?

and that method would be defined as

def automate_catalog_needed?
  params[:display] || params[:template_id] || params[:manager_id] || params[:ovf_template_id]
end

@@ -203,7 +203,7 @@ def atomic_form_field_changed

get_form_vars
# Build Catalog Items tree unless @edit[:ae_tree_select]
Copy link
Member

Choose a reason for hiding this comment

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

is this comment still relevant?

@h-kataria h-kataria force-pushed the content_library_catalog_item branch 2 times, most recently from a39bce7 to f5c7a27 Compare October 1, 2020 17:41
@h-kataria
Copy link
Contributor Author

@skateman New fields added on this form were: Resource Pool, EmsFolder, Host, OVF Template, VM Name, Accept All EULA.

@lfu
Copy link
Member

lfu commented Oct 1, 2020

The PR testing went well 👍

@skateman
Copy link
Member

skateman commented Oct 2, 2020

Code looks good, is it enough if I believe @lfu that it works? 😉

@lfu
Copy link
Member

lfu commented Oct 2, 2020

Testing went well.

@skateman
Copy link
Member

skateman commented Oct 5, 2020

@h-kataria the rubocop issues seem valid, you probably copied the indentation from a different level.

@lfu
Copy link
Member

lfu commented Oct 5, 2020

Tested again and LGTM 👍

@skateman
Copy link
Member

skateman commented Oct 6, 2020

@h-kataria if this is ready for a merge, can you please squash the commits?

This is Ruby/AJAX form similar with some special parameters for OVF Template provisioning.

ManageIQ/manageiq#19718
@miq-bot
Copy link
Member

miq-bot commented Oct 6, 2020

Checked commit h-kataria@69b153d with ruby 2.6.3, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
4 files checked, 1 offense detected

**

  • 💣 💥 🔥 🚒 - Linter/Haml - Linter::Haml STDERR:
warning: parser/current is loading parser/ruby26, which recognizes
warning: 2.6.6-compliant syntax, but you are running 2.6.3.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.

@gtanzillo
Copy link
Member

@h-kataria @skateman This should be ready to merge now that the backend PRs are merged

Copy link
Member

@skateman skateman left a comment

Choose a reason for hiding this comment

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

The Seal of Approval

@skateman skateman merged commit 198b543 into ManageIQ:master Oct 9, 2020
@h-kataria h-kataria deleted the content_library_catalog_item branch January 18, 2021 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants