-
Notifications
You must be signed in to change notification settings - Fork 125
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
Add price to service templates. #367
Conversation
Checked commit lfu@1b13ad2 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the same concerns here as I did with #350. And in fact you're referencing the same BZ ... @tinaafitz it looks like we resolved a direction to go in the previous PR I'm still pushing for that direction.
Also, #350 was adding this to services while this PR is adding it to service_templates. Which is the correct place? |
Oh I see it's an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, excellent. Just took a look at the core PR. Sorry for the noise.
Not sure why but usually when anybody mention in BZ "Service" he most likely means Catalog Items or Catalog Bundle. (Service Templates) there is mentioned path Service -> Catalog in UI, - so I think that Catalog Items or Catalog Bundle. (Service Templates) are correct here. Can you confirm it @lfu ? |
@@ -0,0 +1,6 @@ | |||
class AddPriceToServiceTemplates < ActiveRecord::Migration[5.0] | |||
def change | |||
add_reference :service_templates, :currency, :type => :bigint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the idea is to make the method #currency
and the column currency_id
so the relationship in the model specifies the class https://github.com/ManageIQ/manageiq/pull/18754/files#diff-9257fff45a7d1258305c9a6b7d15e322R66
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unfortunate that chargeback_rate_detail_currency
isn't named just currencies
because it doesn't look like the table has anything chargeback specific in it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carbonin Exactly what I am going to say.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the effort to rename the table :)
A service author would create a service template (that can be a catalog item or catalog bundle) that users can order to get services. The relationship of service template and service is kind of like template and VM. |
Add columns for price and currency to service templates. So a service author can specify the price when creating a service template.
Part of ManageIQ/manageiq#18754
Related to #373
https://bugzilla.redhat.com/show_bug.cgi?id=1602072
@miq-bot add_label enhancement, hammer/no, changelog/yes