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

UI for orderable orchestration templates #7822

Merged

Conversation

mzazrivec
Copy link
Contributor

Implemented:

  • don't display non-orderable orchestration templates in orch. templates catalogs view
  • new read-only screen showing stack's orchestration template + spec
  • new button to make a template orderable + spec
  • new button to copy a template as orderable + spec
  • button to take the user from the stack's orch. template page to the orch. template view in catalog controller

Fixes: #6600

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

@mzazrivec mzazrivec changed the title UI for orderable orchestration templates [WIP] UI for orderable orchestration templates Apr 8, 2016
@mzazrivec mzazrivec force-pushed the orderable_orchestration_templates branch 3 times, most recently from 6eb6c9b to 3e1b38c Compare April 8, 2016 14:49
@mzazrivec mzazrivec force-pushed the orderable_orchestration_templates branch 4 times, most recently from 2b7f83c to a6970c1 Compare April 11, 2016 16:07
@mzazrivec mzazrivec changed the title [WIP] UI for orderable orchestration templates UI for orderable orchestration templates Apr 11, 2016
@mzazrivec
Copy link
Contributor Author

@bzwei @h-kataria @martinpovolny Review?

@miq-bot
Copy link
Member

miq-bot commented Apr 12, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@h-kataria
Copy link
Contributor

@bzwei can you try this in UI, tested in my environment LGTM.

@@ -56,11 +56,12 @@ def textual_ems_cloud
def textual_orchestration_template
template = @record.orchestration_template
return nil if template.nil?
return nil unless template.orderable
Copy link
Contributor

Choose a reason for hiding this comment

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

@mzazrivec i think we don't need this check anymore, we do want to show a link for both types of Orchestartion Templates from Stack summary screen,

@bzwei
Copy link
Contributor

bzwei commented Apr 12, 2016

@mzazrivec from the stack summary page I don't see the template appears in the Relationships table when the template is not orderable. So I cannot test the feature to convert or copy to orderable.

@h-kataria
Copy link
Contributor

@bzwei once @mzazrivec removes the check on #7822 (diff), it should show links to both orderable & non-orderable templates.

@mzazrivec mzazrivec force-pushed the orderable_orchestration_templates branch from a6970c1 to 7d72ed7 Compare April 13, 2016 09:37
@mzazrivec
Copy link
Contributor Author

@bzwei @h-kataria I removed the check preventing from displaying non-orderable
template in UI.

Regarding the new screen -- the requirements, as they were described in the ticket -- were mentioning
a new screen for both orderable & non-orderable templates. There's a button to take you to
the template in catalog explorer. Plus, after you create a copy of a template, you'll be also taken
to catalog explorer to the newly created template.

@h-kataria
Copy link
Contributor

@bzwei please re-test

@bzwei
Copy link
Contributor

bzwei commented Apr 13, 2016

@mzazrivec
Tested the copy as orderable feature. It does not create a new template, instead the old template got renamed and orderable flag set to true. With this feature a new template is expected, leaving the existing one untouched.

Tested the make template orderable feature. I saw message "Orchestration Template is now orderable". But the configuration menu does not change. It still looks as if the template is not orderable. Even worse the template got deleted from the database. I am doing some debugging too to see whether this is a backend problem.

@bzwei
Copy link
Contributor

bzwei commented Apr 13, 2016

@mzazrivec I confirmed there is a backend bug causing the template being deleted. Please do not merge this PR until the backend issue is cleared. At the mean time please take a look at the other problems that I reported.

@h-kataria
Copy link
Contributor

marking it WIP until back end issue is fixed.

@h-kataria h-kataria added the wip label Apr 13, 2016
@h-kataria h-kataria changed the title UI for orderable orchestration templates [WIP] - UI for orderable orchestration templates Apr 13, 2016
@mzazrivec
Copy link
Contributor Author

@bzwei So what probably happened with the OT copy you described above was that a new template (the copy) was created and the old one was deleted.

@bzwei
Copy link
Contributor

bzwei commented Apr 13, 2016

@mzazrivec no, I verified from database that the stack still referenced to the old template but it got renamed and flagged.

@mzazrivec
Copy link
Contributor Author

@bzwei That's strange, because this is the controller code doing the copy:

ot = OrchestrationTemplate.new(
  :name        => params[:templateName],
  :description => params[:templateDescription],
  :type        => original_template.type,
  :content     => params[:templateContent],
  :draft       => params[:templateDraft] == "true",
)
begin
  ot.save_as_orderable!
rescue StandardError => bang
  ...
else
  ...
end

I'm not sure how this can create the results you're describing.

@bzwei
Copy link
Contributor

bzwei commented Apr 13, 2016

@mzazrivec then it must be caused by the same backend bug. I am able to create a PR. Will update here.

@bzwei
Copy link
Contributor

bzwei commented Apr 13, 2016

@mzazrivec I understand the problem now. For a copied template, if the user modifies the content before save, a new template will be generated. If the user only renames, all the changes (name and orderable flag) will be applied to the existing template. This is according to design. So this is not a real problem. The reasoning behind it is we don't allow two templates to have the same content.

@miq-bot
Copy link
Member

miq-bot commented Apr 14, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@mzazrivec mzazrivec force-pushed the orderable_orchestration_templates branch 2 times, most recently from fc290dc to ecd475f Compare April 14, 2016 11:14
@mzazrivec mzazrivec changed the title [WIP] - UI for orderable orchestration templates UI for orderable orchestration templates Apr 14, 2016
@miq-bot miq-bot removed the wip label Apr 14, 2016
@mzazrivec
Copy link
Contributor Author

@bzwei I fixed the problem with re-rendering the screen after making a non-orderable template orderable.

@bzwei
Copy link
Contributor

bzwei commented Apr 14, 2016

otemplate

@mzazrivec The problem still exists. After the template gets converted to orderable, the view in a catalog menu is gray out, and the other two menus are enabled. Also I suggested change the text to View this Orchestration Template in Catalogs because Catalogs is a valid tab under Services.

@mzazrivec mzazrivec force-pushed the orderable_orchestration_templates branch from ecd475f to e1a1886 Compare April 14, 2016 15:07
@mzazrivec mzazrivec force-pushed the orderable_orchestration_templates branch from e1a1886 to 43639e1 Compare April 15, 2016 10:51
@mzazrivec
Copy link
Contributor Author

@bzwei Fixed.

@miq-bot
Copy link
Member

miq-bot commented Apr 15, 2016

Checked commits mzazrivec/manageiq@cf577d3~...43639e1 with ruby 2.2.3, rubocop 0.37.2, and haml-lint 0.16.1
15 files checked, 32 offenses detected

app/controllers/orchestration_stack_controller.rb

app/helpers/application_helper/toolbar/stack_orchestration_template_center.rb

  • 🔶 - Line 3, Col 5 - Style/IndentArray - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
  • 🔶 - Line 29, Col 3 - Style/IndentArray - Indent the right bracket the same as the first position after the preceding left parenthesis.

app/views/orchestration_stack/_copy_orchestration_template.html.haml

  • 🔴 Warn - Line 10, Col - - Operator => should be surrounded by a single space.
  • 🔴 Warn - Line 10, Col - - Operator => should be surrounded by a single space.
  • 🔴 Warn - Line 10, Col - - Operator => should be surrounded by a single space.
  • 🔴 Warn - Line 10, Col - - Operator => should be surrounded by a single space.
  • 🔴 Warn - Line 10, Col - - Operator => should be surrounded by a single space.
  • 🔴 Warn - Line 20, Col - - Operator => should be surrounded by a single space.
  • 🔴 Warn - Line 20, Col - - Operator => should be surrounded by a single space.
  • 🔴 Warn - Line 20, Col - - Operator => should be surrounded by a single space.
  • 🔴 Warn - Line 20, Col - - Operator => should be surrounded by a single space.
  • 🔴 Warn - Line 29, Col - - Operator => should be surrounded by a single space.
  • 🔴 Warn - Line 29, Col - - Operator => should be surrounded by a single space.
  • 🔴 Warn - Line 29, Col - - Operator => should be surrounded by a single space.
  • 🔴 Warn - Line 36, Col - - Operator => should be surrounded by a single space.
  • 🔴 Warn - Line 36, Col - - Operator => should be surrounded by a single space.
  • 🔴 Warn - Line 36, Col - - Operator => should be surrounded by a single space.
  • 🔴 Warn - Line 36, Col - - Operator => should be surrounded by a single space.

app/views/orchestration_stack/_stack_orchestration_template.html.haml

  • 🔴 Warn - Line 9, Col - - Operator => should be surrounded by a single space.
  • 🔴 Warn - Line 9, Col - - Operator => should be surrounded by a single space.
  • 🔴 Warn - Line 9, Col - - Operator => should be surrounded by a single space.
  • 🔴 Warn - Line 9, Col - - Operator => should be surrounded by a single space.
  • 🔴 Warn - Line 9, Col - - Operator => should be surrounded by a single space.
  • 🔴 Warn - Line 9, Col - - Operator => should be surrounded by a single space.
  • 🔴 Warn - Line 32, Col - - Operator => should be surrounded by a single space.
  • 🔴 Warn - Line 32, Col - - Operator => should be surrounded by a single space.

spec/factories/orchestration_stack.rb

@bzwei
Copy link
Contributor

bzwei commented Apr 15, 2016

Tested and looks good now. 👍

@martinpovolny martinpovolny merged commit 3dbae31 into ManageIQ:master Apr 15, 2016
@martinpovolny martinpovolny added this to the Sprint 39 Ending Apr 18, 2016 milestone Apr 15, 2016
@mzazrivec mzazrivec deleted the orderable_orchestration_templates branch April 21, 2016 08:40
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.

None yet

6 participants