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.

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

Added Price to Catalog Items. #5719

Merged
merged 9 commits into from Jul 4, 2019

Conversation

h-kataria
Copy link
Contributor

@h-kataria h-kataria commented Jun 17, 2019

Added Currency drop down and Price text field to Angular and non-angular Catalog Item editors. And update Catalog Item details screen t show the price information.

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

Angular Version Catalog Item editor
angular_catalog_item_edit

Non- Angular version Catalog Item editor
non-angular_catalog_item_edit

Catalog Item summary screen
catalog_item_with_pricing

Price info on Catalog Service detail screen
service_with_pricing

Service that has no pricing info saved
service_with_no_pricing

Catalog Item edit, show price as required field when Currency has been selected.
price_required

Added Currency drop down and Price text field to Angular and non-angular Catalog Item editors. And update Catalog Item details screen t show the price information.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1602072
@h-kataria h-kataria force-pushed the add_pricing_to_service_catalog branch from 9e3c073 to 9ad9add Compare June 19, 2019 14:05
@h-kataria h-kataria removed the wip label Jun 19, 2019
@h-kataria h-kataria changed the title [WIP] - Added Price to Catalog Items. Added Price to Catalog Items. Jun 19, 2019
@h-kataria h-kataria requested a review from himdel June 19, 2019 15:18
@edit[:new][:currency] = params[:currency].to_i
@edit[:new][:code_currency] = code_currency_label(params[:currency])
end
@edit[:new][:price] = params[:price]
Copy link
Contributor

Choose a reason for hiding this comment

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

missing if params[:price]?

This is called both when saving from angular (st_edit with ?button=save or add), and from non-angular st_form_field_changed.

@himdel
Copy link
Contributor

himdel commented Jun 24, 2019

On the non-angular version, UAE Dirham is getting preselected (first option).
Maybe we should not preselect a currency? (Unless we have a user setting for a default currency somewhere?)

(Angular starts with "Nothing selected")

(=> #5719 (comment))

%p.form-control-static
- currency = ChargebackRateDetailCurrency.currencies_for_select
= select_tag("currency",
options_for_select(currency, @edit[:new][:currency]),
Copy link
Contributor

Choose a reason for hiding this comment

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

aah, this is missing the usual [["<Nothing>", nil]] +

@h-kataria
Copy link
Contributor Author

@himdel please re-review

Only show Price info on details screen when currency and price was set.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1602072
Also made changes to show `Price` as required field if user has made selection in a currency drop down.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1602072
@h-kataria
Copy link
Contributor Author

@Loicavenel cc

@Loicavenel
Copy link

Ok, it is fine but I think we need to be more precise.. Price / Month

@h-kataria
Copy link
Contributor Author

@Loicavenel updated labels to Price / Month , screenshots updated to reflect the change.

@himdel himdel added this to the Sprint 115 Ending Jul 8, 2019 milestone Jul 1, 2019
@himdel
Copy link
Contributor

himdel commented Jul 1, 2019

Creating a Generic catalog item...

  • there is no Price/Month field, only "Select Currency", the currency dropdown, and an unlabeled input on the next line (it's the price field, looks like you're only hiding the label, or not generating one without currency)
    EDIT: fixed

  • if I don't select a currency and try to save, I get an error:

 Error caught: [ActiveRecord::RecordNotFound] Couldn't find ChargebackRateDetailCurrency with 'id'=
/home/himdel/.rbenv/versions/2.4.4/lib/ruby/gems/2.4.0/gems/activerecord-5.1.7/lib/active_record/core.rb:189:in `find'
/home/himdel/manageiq-ui-classic/app/controllers/catalog_controller.rb:1284:in `code_currency_label'
/home/himdel/manageiq-ui-classic/app/controllers/catalog_controller.rb:1275:in `get_form_vars'
/home/himdel/manageiq-ui-classic/app/controllers/catalog_controller.rb:881:in `atomic_req_submit'
/home/himdel/manageiq-ui-classic/app/controllers/catalog_controller.rb:100:in `atomic_st_edit'
/home/himdel/manageiq-ui-classic/app/controllers/catalog_controller.rb:76:in `servicetemplate_edit'

As far as I understand, price & currency are not required fields, so, creating a catalog item without currency & price should still be working, right?

(If i do chose a currency, the price is required, that probably makes sense.)

@himdel
Copy link
Contributor

himdel commented Jul 1, 2019

Creating an Ansible Playbook (angular) item works as expected, price is only required when a currency was chosen, saving seems to work fine.

The only issue I'm seeing is that ggg is a valid price now ;)

That is also present in the non-angular version.

EDIT: fixed

@himdel himdel removed this from the Sprint 115 Ending Jul 8, 2019 milestone Jul 1, 2019
@h-kataria
Copy link
Contributor Author

Screenshot from 2019-07-01 19-29-58

Screenshot from 2019-07-01 19-29-15

@himdel addressed your feedback

@himdel
Copy link
Contributor

himdel commented Jul 2, 2019

Trying to add a generic catalog item without price or currency now fails on Price must be a numeric value. Missing a currency present condition for that validation?

(I think that's the last issue, assuming it doesn't still throw Couldn't find ChargebackRateDetailCurrency with 'id'= afterwards.)

Fixed to show label on Price field appropriately when adding new record, and fixed saving of new record when currency/price are both not entered by user.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1602072
@h-kataria h-kataria force-pushed the add_pricing_to_service_catalog branch from d127bca to 11a9375 Compare July 2, 2019 18:56
@himdel
Copy link
Contributor

himdel commented Jul 3, 2019

  • can add a generic item without price or currency
  • can add a ansible playbook item without price or currency
  • can add a generic item with price and currency
  • can add a ansible playbook item with price and currency
  • validation fails for generic item with currency but no price
  • validation fails for generic item with currency but non-float price
  • validation fails for ansible playbook item with currency but no price
  • validation fails for ansible playbook item with currency but non-float price
  • can unset currency in generic
  • can unset currency in angular
  • can see currency & price in detail screen when set

Two potential issues when unsetting an already set currency:

  • create a generic item with price and currency, save, edit, unset only currency.
    The item will fail validation with Price must be a numeric value, which is surprising because there is a numeric price, just no currency.
    (unsetting both works as expected)

  • do the same in angular
    Unsetting a currency works (does not clear the price, but does not show it in detail either),
    but gives an error in the console:

miq_debug.self-1351f771c2cd2fab0d83069fd3f62aadbb7effc11a01ed4942f52308793453be.js?body=1:30 TypeError: Cannot read property 'id' of null
    at catalog_item_form_controller.self-76ca03f2b26e2a8aa63b5e81271612a457ca56b30e7df8717628ad06eb469529.js?body=1:472


function idWatch(name) {
var fieldName = 'vm._' + name;
$scope.$watch(fieldName, function(value) {
vm.catalogItemModel[name + '_id'] = value ? value.id : '';
if (name == 'currency' && typeof value !== 'undefined') {
Copy link
Contributor

Choose a reason for hiding this comment

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

if (name == 'currency' && ! value) should fix the problem when unsetting currency. (It goes from undefined to a currency, but then it goes to null, not undefined, when unselecting.)

@miq-bot
Copy link
Member

miq-bot commented Jul 3, 2019

Checked commits h-kataria/manageiq-ui-classic@9ad9add~...12a460a with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 👍

@himdel
Copy link
Contributor

himdel commented Jul 4, 2019

LGTM,
removing currency autoremoves the price on save,
removing just price warns about price required,
angular and ruby are consistent now :)

@himdel himdel merged commit db9aeca into ManageIQ:master Jul 4, 2019
@himdel himdel added this to the Sprint 115 Ending Jul 8, 2019 milestone Jul 4, 2019
@h-kataria h-kataria deleted the add_pricing_to_service_catalog branch July 29, 2019 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants