[WIP] Add type column to service_order table

[djberg96]

This adds a type column to the service_order table. The associated type would be plucked from its associated requests e.g. ServiceTemplateProvisionRequest or ServiceTemplateTransformationPlanRequest.

Attempts to solve ManageIQ/manageiq-ui-service#1463

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

WIP until I can confirm how the migration should work.

[djberg96]

@himdel @AllenBW Please take a look.

[agrare]

So 2 things

1. Type is reserved for STI to just taking the type from MiqRequest is going to be an issue. If you're going to set a :type column it should match a sub-class of ServiceOrder so something like if MiqRequest is ServiceTemplateProvisionRequest then ServiceOrder would become ServiceProvisionOrder and ServiceTemplateTransformationPlanRequest => ServiceTransformationOrder

2. Updating for every service_order isn't very efficient, it'd be much better to group_by miq_request type and do an update_all

[himdel]

service_order.miq_requests.pluck(:type)

That yields an array of all the types, right? (When there are multiple requests.)

Possibly something like

types = service_order.miq_requests.pluck(:type).sort.uniq
service_order.update_attribute(:type, types.first) if types.count == 1

?

[himdel]

I think..

SELECT string_agg(DISTINCT type, ', ') AS type, service_order_id
FROM miq_requests
WHERE service_order_id IS NOT NULL
GROUP BY service_order_id;

gives you the right list, so you can transform it into a single update, and it should mostly work.
(No idea how to say that in rails-speak though :).)

We may want to clear any types with a comma after though. (And I guess not use type but something like request_type?)

[djberg96]

@agrare Ok, we don't want STI here, so I've changed the column to request_type as per @himdel's suggestion. It's now a sorted and comma-joined string in the event there is more than one request type.

If there's a more efficient way of doing this update I think I will need a little help putting it together.

[djberg96]

@himdel Do we still need this since ManageIQ/manageiq#18368 was merged?

[himdel]

@djberg96 Either you linked the wrong PR, or the two are completely unrelated.

But yes, I think we do need this,
the only alternative I can come up with is adding API support for filtering parents by properties of their children ("give me all service orders with at least one child of type X"), which doesn't seem ..realistic.

[djberg96]

@agrare Ok, clarification, do you think we should be using STI here? Or use a different column altogether (as I've now done)?

[agrare]

@djberg96 I'm not familiar enough with service orders and requests to say, but is it actually possible for a service order to have multiple types of requests? If not then using STI for the service order makes sense.

[agrare]

@bzwei @tinaafitz can you help here?

[tinaafitz]

@djberg96 @agrare, The service_template_provision_request is the only request type for a service order.

[agrare]

@djberg96 I assume v2v is creating a service order for their transformation requests and that is why you need this?

[himdel]

Almost sure the v2v requests are ServiceTemplateTransformationPlanRequest, and definitely sure those get put in service orders as well..

For details, see https://bugzilla.redhat.com/show_bug.cgi?id=1565049 please :)

[tinaafitz]

@himdel Thanks for providing the additional information. :-)

[djberg96]

@himdel @agrare Ok, then what I've got is what we want, right?

There may be a more efficient way of doing it, but I don't know what it is, so help wanted on that front.

[agrare]

Well we wouldn't want a comma joined list of types for a single order right?

If an order can only be of a single type then STI would seem to be the way to go.

I'll post some examples of doing this more efficiently in a bit.

[djberg96]

@himdel @agrare Oops, lost track of this one. So we do want a type column after all?

[agrare]

@djberg96 I forget what you were trying to do here.

From what I gather:

• A single ServiceOrder can have different types of child Requests
• You want to aggregate all of these into a single column on the ServiceOrder
• Since there are multiple child types possible on the same ServiceOrder this isn't STI

Is this correct?

We would also have to handle updating this list when we create new service order requests right?
Is this something that a simple query can't handle? service_order.requests.pluck(:type).uniq

[himdel]

Oops, lost track of this one. So we do want a type column after all?

Frankly, at this point anything that works is fine :).

For the purposes of UI, if this were exposed as a virtual attribute on ServiceOrder, it would be enough, anything we can filter on in the API is fine really.

---

That said, virtual attribute would mean the API has to run that for every single service order, just to determine whether to show it or not, which sounds like an N+1 to me, hence the preference to cache it in the service order table.

The "let's just join the types" approach is the simplest that works without assuming anything about what ServiceOrder can or cannot contain. (And yes, it requires adding/removing requests to update that field.)

An STI-based approach of having 2 kinds of ServiceOrder and let each only accept specific request types does sound better, but possibly not at the cost of making this take another release to spec and finish.

[martinpovolny]

@tinaafitz, @agrare, @djberg96 : we would actually need you to move forward with this.

Is that anything we can do? /cc: @himdel

[himdel]

(Alternatively, if the decision was to abandon the effort completely, close this and ManageIQ/manageiq-ui-service#1463.)

[djberg96]

Alright, trying to dust off the cobwebs on this one. I don't think we want a type column since ServiceTemplateProvisionRequest and ServiceTemplateTransformationPlanRequest are types of MiqRequest, not ServiceOrder. I think this is what @agrare was trying to tell me originally.

So, let's leave it at request_type for now. Then I guess the question is the migration strategy.

[djberg96]

We would also have to handle updating this list when we create new service order requests right?

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.

[himdel]

Created a simple "just add a .v2v? virtual attribute on ServiceOrder" in ManageIQ/manageiq#19332 for your consideration :)

[agrare]

I don't love the comma-separated list of child types on a service_order.

I don't think I ever got an answer to "Can a ServiceOrder have both v2v child request types and non-v2v child request types?"

According to @himdel's PR v2v if .any? { kind_of?(ServiceTemplateTransformationPlanRequest) } it seems like if there is a transformation plan request, then the service_order is a v2v type.

If that is the case, lets make ServiceOrder STI and create a ServiceTransformationOrder subclass.

[himdel]

I don't think I ever got an answer to "Can a ServiceOrder have both v2v child request types and non-v2v child request types?"

I have no idea there either. I don't think anybody knows, IMO it was a mistake to reuse ServiceOrder for v2v without an explicit plan, but we're stuck with that.

(That's pretty much the only reason I proposed the comma separated list - nobody knows, but this shouldn't break anything.)

The .any? bit is based on the same logic. We don't know anything, but if there's at least 1 v2v request, it's probably a v2v thing.

[himdel]

(Definitely +1 on STI, if it can be done :)).

[agrare]

Okay let me try to find out, git log app/models/service_template_transformation_plan_request.rb | tail has given me a name :)

[bzwei]

ServiceOrder is transparent in the current use of v2v, one service order per v2v request. Therefore STI should work.

[himdel]

Any progress here? The migration seems ready to me..

EDIT: except it says request_type, not type.

[miq-bot]

Checked commits https://github.com/djberg96/manageiq-schema/compare/19bb4ceae42c4a332570647f9394e3eac3a84149~...da0823d6e75f6b9d02a3c757da7af29003407d3c with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 2 offenses detected

db/migrate/20190123145942_add_type_column_to_service_order.rb

• ❗ - Line 7, Col 69 - Style/CaseEquality - Avoid the use of the case equality operator ===.
• ❗ - Line 8, Col 23 - Rails/SkipsModelValidations - Avoid using update_attribute because it skips validations.

[gmcculloug]

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.

[gmcculloug]

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.

[himdel]

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

[gmcculloug]

Closing in favor of #452