Add internal column to service template for transformation plan #17748

hsong-rh · 2018-07-23T21:01:28Z

Add internal flag to limit the access scope for transformation plan.

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

Schema PR to migrate away from reserves table: ManageIQ/manageiq-schema#238.

hsong-rh · 2018-07-23T21:31:10Z

gmcculloug · 2018-07-24T13:26:11Z

@bdunne @Fryguy Can you give this a quick review?

Note in the description that the first commit is for Gaprindashvili so it uses the ReservedMixin. Also, tests are failing because of the schema PR dependency.

bdunne · 2018-07-24T13:56:25Z

app/models/service_template.rb

@@ -74,6 +77,7 @@ class ServiceTemplate < ApplicationRecord
  scope :without_service_template_catalog_id,       ->         { where(:service_template_catalog_id => nil) }
  scope :with_existent_service_template_catalog_id, ->         { where.not(:service_template_catalog_id => nil) }
  scope :displayed,                                 ->         { where(:display => true) }
+  scope :public_service_template,                   ->         { where(:internal => true) }


Does this work? I don't think you can use a where like this to query on a reserved attribute.

I tried it on another model and got:

NoMethodError: undefined method `eq' for nil:NilClass from /home/bdunne/.gem/ruby/2.4.3/gems/activerecord-5.0.7/lib/active_record/relation/predicate_builder/basic_object_handler.rb:9:in `call' from /home/bdunne/.gem/ruby/2.4.3/gems/activerecord-5.0.7/lib/active_record/relation/predicate_builder.rb:76:in `build' from /home/bdunne/.gem/ruby/2.4.3/gems/activerecord-5.0.7/lib/active_record/relation/predicate_builder.rb:46:in `expand' from /home/bdunne/.gem/ruby/2.4.3/gems/activerecord-5.0.7/lib/active_record/relation/predicate_builder.rb:90:in `block in expand_from_hash' from /home/bdunne/.gem/ruby/2.4.3/gems/activerecord-5.0.7/lib/active_record/relation/predicate_builder.rb:86:in `each' from /home/bdunne/.gem/ruby/2.4.3/gems/activerecord-5.0.7/lib/active_record/relation/predicate_builder.rb:86:in `flat_map' from /home/bdunne/.gem/ruby/2.4.3/gems/activerecord-5.0.7/lib/active_record/relation/predicate_builder.rb:86:in `expand_from_hash' from /home/bdunne/.gem/ruby/2.4.3/gems/activerecord-5.0.7/lib/active_record/relation/predicate_builder.rb:31:in `build_from_hash' from /home/bdunne/.gem/ruby/2.4.3/gems/activerecord-5.0.7/lib/active_record/relation/where_clause_factory.rb:22:in `build' from /home/bdunne/.gem/ruby/2.4.3/gems/activerecord-5.0.7/lib/active_record/relation/query_methods.rb:632:in `where!' from /home/bdunne/.gem/ruby/2.4.3/gems/activerecord-5.0.7/lib/active_record/relation/query_methods.rb:625:in `where' from /home/bdunne/.gem/ruby/2.4.3/gems/activerecord-5.0.7/lib/active_record/querying.rb:10:in `where' from (irb):2 from /home/bdunne/.gem/ruby/2.4.3/gems/railties-5.0.7/lib/rails/commands/console.rb:65:in `start' from /home/bdunne/.gem/ruby/2.4.3/gems/railties-5.0.7/lib/rails/commands/console_helper.rb:9:in `start' from /home/bdunne/.gem/ruby/2.4.3/gems/railties-5.0.7/lib/rails/commands/commands_tasks.rb:78:in `console' from /home/bdunne/.gem/ruby/2.4.3/gems/railties-5.0.7/lib/rails/commands/commands_tasks.rb:49:in `run_command!' from /home/bdunne/.gem/ruby/2.4.3/gems/railties-5.0.7/lib/rails/commands.rb:18:in `<top (required)>' from bin/rails:4:in `require' from bin/rails:4:in `<main>'

Fryguy · 2018-07-24T16:11:49Z

app/models/service_template.rb

@@ -75,6 +78,10 @@ class ServiceTemplate < ApplicationRecord
  scope :with_existent_service_template_catalog_id, ->         { where.not(:service_template_catalog_id => nil) }
  scope :displayed,                                 ->         { where(:display => true) }

+  def self.public_service_templates
+    ServiceTemplate.all - ServiceTemplate.joins(:reserved_rec).where(:reserves=> {:reserved => {:internal => true}.to_yaml}).to_a


This should return a scope if possible

That where clause is wrong, because there could be other things in reserves if we need to add them.

Also, please add a spec

@Fryguy what is the way to write the where clause? We tested but cannot make reserved field deserialized in this query.

Can we query like ServiceTemplate.includes(:reserved_rec).select { |s| !s.internal }?

@Fryguy Changed back to scope based on above query.

bdunne · 2018-07-24T16:16:25Z

Injecting a ServiceTemplateInternal subclass is looking better and better... What do you think @gmcculloug?

Fryguy · 2018-07-24T16:18:41Z

👎 for subclass...it conflates a property of potentially any service template with the type of a service template.

gmcculloug · 2018-07-24T19:38:41Z

app/models/service_template.rb

@@ -74,6 +77,7 @@ class ServiceTemplate < ApplicationRecord
  scope :without_service_template_catalog_id,       ->         { where(:service_template_catalog_id => nil) }
  scope :with_existent_service_template_catalog_id, ->         { where.not(:service_template_catalog_id => nil) }
  scope :displayed,                                 ->         { where(:display => true) }
+  scope :public_service_templates,                  ->         { includes(:reserved_rec).select { |s| s.internal.nil? }


@hsong-rh You are missing the closing bracket on the scope.

gmcculloug · 2018-07-24T20:29:33Z

app/models/service_template.rb

@@ -1,4 +1,7 @@
 class ServiceTemplate < ApplicationRecord
+  include ReservedMixin
+  reserve_attribute :internal, :boolean


These lines should be moved down by the other include statements around line 38.

gmcculloug · 2018-07-24T20:30:43Z

app/models/service_template_transformation_plan.rb

@@ -7,6 +7,8 @@ def request_type
    "transformation_plan"
  end

+  default_value_for :internal, true


We would also want a default_value_for :internal, false at the base class.

do we really want such a default_value? Without default we don't need to insert a record in the reserved table.

I would prefer this upstream once we have the real column, so it is fine to ignore while we are working on the reserves table commits.

gmcculloug · 2018-07-24T20:34:29Z

app/models/service_template.rb

@@ -74,6 +77,7 @@ class ServiceTemplate < ApplicationRecord
  scope :without_service_template_catalog_id,       ->         { where(:service_template_catalog_id => nil) }
  scope :with_existent_service_template_catalog_id, ->         { where.not(:service_template_catalog_id => nil) }
  scope :displayed,                                 ->         { where(:display => true) }
+  scope :public_service_templates,                  ->         { includes(:reserved_rec).select { |s| s.internal.nil? } }


Should the condition be !s.internal to handle both nil and false values.

This returns an array, but we want a true scope. The best I can come up is

where.not(:id => ServiceTemplate.includes(:reserved_rec).joins(:reserved_rec).select { |s| s.internal == true }.collect(&:id))

@bzwei 👍

gmcculloug

@bdunne @Fryguy Any further comments?

Note, the existing scope is intended for gaprindashvili and will be changed upstream once we have the new internal column available.

gmcculloug · 2018-07-27T13:16:08Z

@bdunne @Fryguy bump

bdunne · 2018-07-30T13:49:30Z

spec/models/service_template_transformation_plan_spec.rb

+      st1 = FactoryGirl.create(:service_template_transformation_plan)
+      st2 = FactoryGirl.create(:service_template)
+
+      expect(st1.internal).to be_truthy


Prefer internal? since it's a boolean attribute

On second thought... Do you need to test this? It's just testing default_value_for, right?

Yes, they are testing the default value, to make sure if it is set correctly. internal is column name.

bdunne · 2018-07-30T13:50:22Z

spec/models/service_template_transformation_plan_spec.rb

+
+      expect(st1.internal).to be_truthy
+      expect(st2.internal).to be_falsey
+      expect(ServiceTemplate.public_service_templates.count).to eq(1)


These two expectations can be merged into:

expect(ServiceTemplate.public_service_templates).to match_array([st2])

kbrock

filtering on columns that are serialized in reserves#reserved will always bring back all records. (sure wish it were an hstore or jsonb column)

kbrock · 2018-07-30T15:30:15Z

app/models/service_template.rb

@@ -74,6 +76,7 @@ class ServiceTemplate < ApplicationRecord
  scope :without_service_template_catalog_id,       ->         { where(:service_template_catalog_id => nil) }
  scope :with_existent_service_template_catalog_id, ->         { where.not(:service_template_catalog_id => nil) }
  scope :displayed,                                 ->         { where(:display => true) }
+  scope :public_service_templates,                  ->         { where.not(:id => ServiceTemplate.includes(:reserved_rec).joins(:reserved_rec).select { |s| s.internal == true }.collect(&:id)) }


not sure if the joins is necessary

@kbrock What if we query the Reserve first?

where.not(:id => Reserved.where(:resource_type => "ServiceTemplate").all.collect { |r| r.resource_id if r[:internal] == true }.compact)

@kbrock Without join, ServiceTemplate.includes(:reserved_rec).select { |s| s.internal == true }.collect(&:id) give us same result.

@hsong-rh Remember we need to retain this as a scope so users can chain other scopes onto the resulting object.

@bdunne This works: where.not(:id => Reserve.where(:resource_type => "ServiceTemplate").all.collect { |r| r.resource_id if r.reserved[:internal] == true }.compact)

@gmcculloug yes, they return as scope.

bzwei · 2018-07-30T18:25:27Z

app/models/service_template.rb

@@ -74,6 +76,7 @@ class ServiceTemplate < ApplicationRecord
  scope :without_service_template_catalog_id,       ->         { where(:service_template_catalog_id => nil) }
  scope :with_existent_service_template_catalog_id, ->         { where.not(:service_template_catalog_id => nil) }
  scope :displayed,                                 ->         { where(:display => true) }
+  scope :public_service_templates,                  ->         { where.not(:id => Reserve.where(:resource_type => "ServiceTemplate").all.collect { |r| r.resource_id if r.reserved[:internal] == true }.compact) }


you don't need .all, or even simpler

where.not(:id => Reserve.where(:resource_type => "ServiceTemplate").collect { |r| r.reserved[:internal] && r.resource_id }.compact

@bzwei Changed.

gmcculloug · 2018-07-30T19:46:46Z

@Fryguy @bdunne @kbrock Any chance we can get this merged today?

@kbrock I hear your concern, we'll be migrating away from this upstream. The good news is this is against service_templates which is likely to be in the 100s of instances and not services.

bdunne · 2018-07-30T20:01:34Z

Oh, good catch on the r.reserved[:internal], and not needing the == true. Any reason that you used && rather than if?

bzwei · 2018-07-30T20:25:45Z

@bdunne The line is too long. Just to make it shorter.

bdunne · 2018-07-30T20:50:10Z

Too long for what?

bzwei · 2018-07-30T21:35:31Z

Previously the line for the scope is very long. I thought we can make it shorter and proposed to use &&. It is an issue of coding style preference. Feel free to choose the way you want to code.

gmcculloug · 2018-07-31T14:26:59Z

@bdunne Do you have a preference? I feel like this PR is really close now and would like to see if we can get it to the finish line. 🏁

bdunne · 2018-07-31T15:05:59Z

I thought if reads better, but it doesn't really matter

hsong-rh · 2018-07-31T15:33:08Z

@bdunne Changed as you suggested. Thanks,

miq-bot · 2018-07-31T15:41:53Z

Checked commit hsong-rh@f333539 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🍪

Add internal column to service template for transformation plan (cherry picked from commit 2a66cb5) https://bugzilla.redhat.com/show_bug.cgi?id=1610729

simaishi · 2018-08-01T10:20:15Z

Gaprindashvili backport details:

$ git log -1
commit 685c571bef061200b7a26b8aa3b4f3577014733e
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Tue Jul 31 12:07:50 2018 -0400

    Merge pull request #17748 from hsong-rh/add_internal_to_service_template
    
    Add internal column to service template for transformation plan
    (cherry picked from commit 2a66cb59e26816c7296896620b5b7731b350943d)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1610729

gmcculloug requested a review from bzwei July 24, 2018 13:09

gmcculloug self-assigned this Jul 24, 2018

gmcculloug added blocker gaprindashvili/yes v2v labels Jul 24, 2018

bzwei approved these changes Jul 24, 2018

View reviewed changes

bdunne reviewed Jul 24, 2018

View reviewed changes

Fryguy reviewed Jul 24, 2018

View reviewed changes

gmcculloug reviewed Jul 24, 2018

View reviewed changes

hsong-rh force-pushed the add_internal_to_service_template branch 3 times, most recently from 42d0556 to da7a48d Compare July 25, 2018 13:28

gmcculloug approved these changes Jul 25, 2018

View reviewed changes

gmcculloug mentioned this pull request Jul 28, 2018

Refactor service_type to use constants #17681

Merged

bdunne reviewed Jul 30, 2018

View reviewed changes

hsong-rh force-pushed the add_internal_to_service_template branch from da7a48d to d22be20 Compare July 30, 2018 14:02

kbrock reviewed Jul 30, 2018

View reviewed changes

h-kataria mentioned this pull request Jul 30, 2018

Changes to exclude service templates marked as service_type = internal ManageIQ/manageiq-ui-classic#4274

Merged

hsong-rh force-pushed the add_internal_to_service_template branch from d22be20 to 61ec522 Compare July 30, 2018 18:12

bzwei reviewed Jul 30, 2018

View reviewed changes

hsong-rh force-pushed the add_internal_to_service_template branch from 61ec522 to ee21a01 Compare July 30, 2018 18:50

gmcculloug mentioned this pull request Jul 31, 2018

Add internal column to ServiceTemplate table ManageIQ/manageiq-schema#238

Merged

hsong-rh force-pushed the add_internal_to_service_template branch from ee21a01 to 54daf2a Compare July 31, 2018 15:32

bdunne approved these changes Jul 31, 2018

View reviewed changes

Add internal attribute as reserved. (Commits for Gaprindashvili)

f333539

hsong-rh force-pushed the add_internal_to_service_template branch from 54daf2a to f333539 Compare July 31, 2018 15:40

gmcculloug merged commit 2a66cb5 into ManageIQ:master Jul 31, 2018

hsong-rh deleted the add_internal_to_service_template branch July 31, 2018 18:12

gmcculloug added this to the Sprint 92 Ending Aug 13, 2018 milestone Jul 31, 2018

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Aug 1, 2018

Add internal column to service template for transformation plan #17748

Add internal column to service template for transformation plan #17748

Conversation

hsong-rh commented Jul 23, 2018 • edited by gmcculloug Loading

hsong-rh commented Jul 23, 2018

gmcculloug commented Jul 24, 2018

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

bzwei Jul 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdunne commented Jul 24, 2018

Fryguy commented Jul 24, 2018

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

gmcculloug left a comment

Choose a reason for hiding this comment

gmcculloug commented Jul 27, 2018

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

kbrock left a 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

hsong-rh Jul 30, 2018 • 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

gmcculloug commented Jul 30, 2018

bdunne commented Jul 30, 2018

bzwei commented Jul 30, 2018

bdunne commented Jul 30, 2018

bzwei commented Jul 30, 2018

gmcculloug commented Jul 31, 2018

bdunne commented Jul 31, 2018

hsong-rh commented Jul 31, 2018

miq-bot commented Jul 31, 2018

simaishi commented Aug 1, 2018

hsong-rh commented Jul 23, 2018 •

edited by gmcculloug

Loading

bzwei Jul 24, 2018 •

edited

Loading

hsong-rh Jul 30, 2018 •

edited

Loading