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.

Sign up for GitHub

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

[WIP] Add type column to service_order table #328

Closed
wants to merge 3 commits into from
Closed

[WIP] Add type column to service_order table #328

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
19bb4ce
Add type column to service_order table.
Jan 23, 2019
9ff7413
Change column from type to request_type, and use joined string as res…
Mar 19, 2019
da0823d
Switch back to STI.
djberg96 Jan 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions db/migrate/20190123145942_add_type_column_to_service_order.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
class AddTypeColumnToServiceOrder < ActiveRecord::Migration[5.0]
def up
add_column :service_order, :type, :string

say_with_time("Set service_order type") do
ServiceOrder.all.each do |service_order|
type = service_order.miq_requests.pluck(:type).any? { |t| t === 'ServiceTemplateTransformationPlanRequest' } ? ServiceOrderV2V : ServiceOrderCart
service_order.update_attribute(:type, type)
Copy link
Member

Choose a reason for hiding this comment

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

A few thoughts on these changes.

  1. add_colum and remove_column reference service_order instead of service_orders (plural)
  2. Use update instead of update_attribute
  3. Since you are manipulating the type column directly I would recommend disabling STI at the top of the migration with self.inheritance_column = :_type_disabled in the class.
  4. I recall we try to avoid active_record relationships in migration and use direct queries. In this case that would mean avoiding the service_order.miq_requests call.
  5. There are references to new class types ServiceOrderV2V : ServiceOrderCart which are not merged yet and the migration will fail. (See [WIP] Use STI with ServiceOrder model manageiq#19766)
  6. PR is missing migration tests.

Then I really started thinking about the changes and wanted to suggest the following migration replacement.

The basic premise is we get all the v2v Requests and do a single call to update the related ServiceOrders. Any ServiceOrders remaining will be cart orders, so we can update those by finding ones that have type => nil.

Here's what I think the migration should look like: (

class AddTypeColumnToServiceOrder < ActiveRecord::Migration[5.0]
  class ServiceOrder < ActiveRecord::Base
    self.inheritance_column = :_type_disabled
  end

  def up
    add_column :service_orders, :type, :string

    say_with_time("Set service_orders type") do
      v2v_order_ids = ServiceTemplateTransformationPlanRequest.pluck(:service_order_id)
      ServiceOrder.where(:id => v2v_order_ids).update_all(:type => 'ServiceOrderV2V')

      ServiceOrder.where(:type => nil).update_all(:type => 'ServiceOrderCart')
    end
  end

  def down
    remove_column :service_orders, :type
  end
end

This still requires that ManageIQ/manageiq#19766 gets merged and I highly recommend tests be added to this PR before merging.

Copy link
Member

Choose a reason for hiding this comment

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

Also need to update ManageIQ/manageiq#19766 to address the comment above (#328 (comment))

Well, we actually need a way to guarantee that this column gets set automatically when a new ServiceOrder is created, which would cover it. I think that means a before_save hook.

Copy link
Contributor

Choose a reason for hiding this comment

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

It will also need some logic to make sure the v2v paths create the right kind

end
end
end

def down
remove_column :service_order, :type
end
end