Add textual summary for orchestration_stack on the service summary page #12954

lgalis · 2016-12-01T21:27:20Z

Depends on #13089

Handle empty orchestration stack link in the service summary page.
A service for RHOS can contain an empty orchestration stack. This PR changes the summary page for the service to handle this.

Links

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

Steps for Testing

1.Create a Service for RHOS using a non-existing stack name
2.The link for the stack in the service's summary page is invalid - this PR removes the link

Before:

After:

lgalis · 2016-12-01T21:27:36Z

@miq-bot add_label wip, ui, bug

lgalis · 2016-12-07T21:16:51Z

@miq-bot remove_label wip

martinpovolny · 2016-12-08T13:06:19Z

@lgalis : I have to confess that I don't like the place where this is being fixed.

Could we solve this using a decorator?

@himdel, what do you think?

himdel · 2016-12-08T13:14:46Z

spec/helpers/service_helper/textual_summary_spec.rb

+      expect(textual_orchestration_stack).to eq(@os_cloud)
+    end
+
+    it 'contains the link to the associated stack' do


the description is the same for both cases, can you add cloud & infra to the descriptions so that they differ please?

martinpovolny · 2016-12-08T13:16:24Z

Ok, looking around, it's probably ok to do it this way.

But the specs look strange. @skateman : can you, pls, tell how to fix the specs here?

himdel · 2016-12-08T13:20:03Z

Could we solve this using a decorator?

It feels weird that a non-nil orchestration stack is returned in the first place.

But assuming that's the correct behaviour, yup, decorators would be perfect for the job :). But.. it would be the first textual-summary-related use of decorators in miq, so not sure if now..

skateman · 2016-12-08T13:27:06Z

spec/helpers/service_helper/textual_summary_spec.rb

+
+    subject { textual_orchestration_stack }
+    it 'contains the link to the associated stack' do
+      @os_cloud = FactoryGirl.create(:orchestration_stack_cloud, :name => "cloudstack1")


Using instance variables here doesn't make any sens, could you export this to let blocks combined with contexts. Something like this:

subject { FactoryGirl.create(cloud, :name => name) } context "openstack-cloud" do let(:cloud) { :orchestration_stack_cloud } let(:name) { 'cloudstack1' } it 'contains the link to the associated stack' do # expect(subject...) end end context "openstack-infra" do let(:cloud) { :orchestration_stack_openstack_infra } let(:name) { 'infrastack1' } it 'contains the link to the associated stack' do # expect(subject...) end end # ...

…ry page

…vice summary page

himdel · 2016-12-13T12:17:41Z

app/helpers/service_helper/textual_summary.rb

+    if ost && !ost.id.present?
+      {
+        :label => _("Orchestration Stack"),
+        :image => "orchestration_stack",


@lgalis the format of the images in textual summaries will change with #13089 .. so depending on which gets merged first, you may need to do :image => "100/orchestration_stack.png"

lgalis · 2016-12-13T15:14:19Z

@himdel - thanks - I modified the image and edit a note in the description of this PR that it depends on #13089

skateman · 2016-12-13T15:51:23Z

spec/helpers/service_helper/textual_summary_spec.rb

+    end
+
+    subject { textual_orchestration_stack }
+    it 'contains the link to the associated stack' do


Why are you using the same it twice in the same context? Please create a nested-context for both cases.

skateman · 2016-12-13T15:52:04Z

spec/helpers/service_helper/textual_summary_spec.rb

+    let(:os_infra) { FactoryGirl.create(:orchestration_stack_openstack_infra, :name => "infrastack1") }
+
+    before do
+      login_as @user = FactoryGirl.create(:user)


Unused instance variable @user

skateman · 2016-12-13T15:53:52Z

spec/helpers/service_helper/textual_summary_spec.rb

@@ -0,0 +1,30 @@
+describe ServiceHelper::TextualSummary do
+  context ".textual_orchestration_stack" do


here you want describe instead of context

@skateman - I have changed it - but I do not really agree - context is an alias of describe - and usually - describe wraps the user story, and context the individual scenario. We, as a team -can decide on a set of guidelines for how we mark our specs.

skateman · 2016-12-13T15:54:18Z

spec/helpers/service_helper/textual_summary_spec.rb

+    subject { textual_orchestration_stack }
+    it 'contains the link to the associated stack' do
+      @record = FactoryGirl.create(:service)
+      allow(@record).to receive(:orchestration_stack).and_return(os_cloud)


you don't need instance variables here using record is fine

@skateman - the instance variable is actually needed - textual summary uses @record

miq-bot · 2016-12-13T16:27:34Z

Checked commits lgalis/manageiq@367b2a6~...063b72f with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
2 files checked, 0 offenses detected
Everything looks good. 🍰

skateman · 2016-12-13T17:17:39Z

@lgalis true, they're aliased but there is a semantic difference. Describe refers to describing a class' or method's behavior while context refers to a context with different external attributes than outside the context.

…

On Tue, Dec 13, 2016 at 6:11 PM, lgalis ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In spec/helpers/service_helper/textual_summary_spec.rb <#12954>: > @@ -0,0 +1,30 @@ +describe ServiceHelper::TextualSummary do + context ".textual_orchestration_stack" do @skateman <https://github.com/skateman> - I have changed it - but I do not really agree - context is an alias of describe - and usually - describe wraps the user story, and context the individual scenario. We, as a team -can decide on a set of guidelines for how we mark our specs. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12954>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAnnqg6VFdltw1leu6ALE0ay-JCBOhstks5rHtGygaJpZM4LB6PD> .

-- Dávid Halász dhalasz@redhat.com Brno TPB 2A217 Extension: 82 62532 Phone: +420 532 294 532 IRC: skateman

simaishi · 2017-01-10T00:21:49Z

@lgalis #13089 that this PR depends on is marked as euwe/no. Do you need to create Euwe specific PR?

himdel · 2017-01-10T12:14:59Z

if it helps, the difference will be

-        :image => "100/orchestration_stack.png",
+        :image => "orchestration_stack",

…ervice_summary_page Add textual summary for orchestration_stack on the service summary page (cherry picked from commit 9f4bfb1) https://bugzilla.redhat.com/show_bug.cgi?id=1411373

simaishi · 2017-01-11T16:23:50Z

Euwe backport details (fixed image path):

$ git log -1
commit 548b54fe8ed453008e0b1163e60e723e6a263681
Author: Dan Clarizio <dclarizi@redhat.com>
Date:   Tue Dec 20 07:48:40 2016 -0800

    Merge pull request #12954 from lgalis/orchestration_stack_detail_in_service_summary_page
    
    Add textual summary for orchestration_stack on the service summary page
    (cherry picked from commit 9f4bfb1dd2eecd5e8cc49eb87e07ce5525ccccb6)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1411373

miq-bot added bug ui wip labels Dec 1, 2016

chessbyte assigned dclarizio Dec 2, 2016

lgalis force-pushed the orchestration_stack_detail_in_service_summary_page branch 6 times, most recently from 49c0ad9 to 5394138 Compare December 7, 2016 21:16

lgalis changed the title ~~[WIP] Add textual summary for orchestration_stack on the service summary page~~ Add textual summary for orchestration_stack on the service summary page Dec 7, 2016

miq-bot removed the wip label Dec 7, 2016

mzazrivec assigned martinpovolny Dec 8, 2016

himdel reviewed Dec 8, 2016

View reviewed changes

skateman suggested changes Dec 8, 2016

View reviewed changes

lgalis added 2 commits December 12, 2016 17:45

Add textual summary for orchestration_stack link on the service summa…

367b2a6

…ry page

Add specs for textual summary for orchestration_stack link on the ser…

f89589a

…vice summary page

lgalis force-pushed the orchestration_stack_detail_in_service_summary_page branch from 5394138 to e5d00bb Compare December 12, 2016 23:10

himdel reviewed Dec 13, 2016

View reviewed changes

lgalis force-pushed the orchestration_stack_detail_in_service_summary_page branch from e5d00bb to 82337cd Compare December 13, 2016 15:12

skateman suggested changes Dec 13, 2016

View reviewed changes

lgalis force-pushed the orchestration_stack_detail_in_service_summary_page branch from 82337cd to e435886 Compare December 13, 2016 16:19

Do not display a link for an invalid stack

063b72f

lgalis force-pushed the orchestration_stack_detail_in_service_summary_page branch from e435886 to 063b72f Compare December 13, 2016 16:24

dclarizio merged commit 9f4bfb1 into ManageIQ:master Dec 20, 2016

dclarizio added the euwe/yes label Dec 20, 2016

dclarizio added this to the Sprint 51 Ending Jan 2, 2017 milestone Dec 20, 2016

lgalis deleted the orchestration_stack_detail_in_service_summary_page branch January 6, 2017 18:46

simaishi added euwe/backported and removed euwe/yes labels Jan 11, 2017

lgalis mentioned this pull request Jan 11, 2017

[EUWE] Fix image for orchestration_stack textual summary #13453

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add textual summary for orchestration_stack on the service summary page #12954

Add textual summary for orchestration_stack on the service summary page #12954

lgalis commented Dec 1, 2016 •

edited

Loading

lgalis commented Dec 1, 2016

lgalis commented Dec 7, 2016

martinpovolny commented Dec 8, 2016

himdel Dec 8, 2016

martinpovolny commented Dec 8, 2016

himdel commented Dec 8, 2016 •

edited

Loading

skateman Dec 8, 2016

himdel Dec 13, 2016

lgalis commented Dec 13, 2016

skateman Dec 13, 2016

skateman Dec 13, 2016

skateman Dec 13, 2016

lgalis Dec 13, 2016

skateman Dec 13, 2016

lgalis Dec 13, 2016 •

edited

Loading

miq-bot commented Dec 13, 2016

skateman commented Dec 13, 2016 via email

simaishi commented Jan 10, 2017

himdel commented Jan 10, 2017

simaishi commented Jan 11, 2017

		@@ -0,0 +1,30 @@
		describe ServiceHelper::TextualSummary do
		context ".textual_orchestration_stack" do

Add textual summary for orchestration_stack on the service summary page #12954

Add textual summary for orchestration_stack on the service summary page #12954

Conversation

lgalis commented Dec 1, 2016 • edited Loading

Links

Steps for Testing

lgalis commented Dec 1, 2016

lgalis commented Dec 7, 2016

martinpovolny commented Dec 8, 2016

Choose a reason for hiding this comment

martinpovolny commented Dec 8, 2016

himdel commented Dec 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgalis commented Dec 13, 2016

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

lgalis Dec 13, 2016 • edited Loading

Choose a reason for hiding this comment

miq-bot commented Dec 13, 2016

skateman commented Dec 13, 2016 via email

simaishi commented Jan 10, 2017

himdel commented Jan 10, 2017

simaishi commented Jan 11, 2017

lgalis commented Dec 1, 2016 •

edited

Loading

himdel commented Dec 8, 2016 •

edited

Loading

lgalis Dec 13, 2016 •

edited

Loading