Add Vouchers to OC Customer Total Report #11999
Conversation
| def initialize(name:, options:, selected:) | ||
| # @param id [String] uniquely identifies the MultipleCheckedSelect (mcs) field. '_mcs_field' will be appended | ||
| def initialize(id:, name:, options:, selected:) | ||
| @id = "#{id}_mcs_field" |
There was a problem hiding this comment.
adding id to the mcs field, it would make it easier to test and identify each field uniquely in case we use multiple mcs on a page
There was a problem hiding this comment.
If we use multiple fields, they should have different labels.
chahmedejaz
changed the title
lib/reporting/reports/orders_and_fulfillment/order_cycle_customer_totals.rb
Outdated
Show resolved
Hide resolved
| end | ||
|
|
||
| def voucher_amount(order) | ||
| return '' unless voucher_applicable?(order) |
There was a problem hiding this comment.
I am wondering if actually need this check ?
If I am not mistaken (order.total - order.pre_discount_total).abs would return 0 if no voucher has been applied to the order.
There was a problem hiding this comment.
Yes, but for consistency between voucher label vs amount's empty state, I added this check to return empty strings as their empty state value. Thanks. :)
| @@ -27,5 +27,5 @@ | |||
| .row | |||
| .alpha.two.columns= label_tag nil, t(:report_columns) | |||
| .omega.fourteen.columns | |||
| = render MultipleCheckedSelectComponent.new(name: "fields_to_show", options: @report.available_headers, selected: @rendering_options.options[:fields_to_show]) | |||
| = render MultipleCheckedSelectComponent.new(id: 'fields_to_show', name: "fields_to_show", options: @report.available_headers, selected: @rendering_options.options[:fields_to_show]) | |||
There was a problem hiding this comment.
Needing to change the code just to test it can be a code smell. And system specs should test form the perspective of the user which can't know DOM ids. It's better to click on something visible. I'll add a commit to show you an alternative.
There was a problem hiding this comment.
I'll make sure to keep that in mind. Thanks :)
* This partly reverts 3a957fb.
…mer_totals.rb Co-authored-by: Gaetan Craig-Riou <40413322+rioug@users.noreply.github.com>
|
Hi @chahmedejaz, After staging the PR I could see the new columns, which were inactive (hidden) by default. ✔️ The numbers I checked are correct. I also compared them to the numbers of the order details page. ✔️ Comparing the report before and after the PR showed no difference - apart from the new columns, of course. ✔️ I think this one is ready to go! Excellent! 🎉 |
What? Why?
What should we test?
Release notes
Changelog Category (reviewers may add a label for the release notes):