From 0673f9a5ae968f68e1ae83b6125ebae8a5337e1b Mon Sep 17 00:00:00 2001 From: Cillian O'Ruanaidh Date: Fri, 15 Jul 2022 15:42:19 +0100 Subject: [PATCH] Use a more simple layout on the order cycle checkout options form Before there was a row for each distributor and a 'shared' row for shipping methods which were shared among more than one distributor. This layout displays a single list of shipping methods with the distributor or distributors it belongs to beside it as suggested by @lin-d-hop --- app/helpers/admin/order_cycles_helper.rb | 21 ++-- app/models/spree/shipping_method.rb | 4 + .../order_cycles/checkout_options.html.haml | 114 ++++++++---------- .../controllers/select_all_controller.js | 15 +++ config/locales/en.yml | 2 - .../stimulus/select_all_controller_test.js | 82 +++++++++++++ 6 files changed, 165 insertions(+), 73 deletions(-) create mode 100644 app/webpacker/controllers/select_all_controller.js create mode 100644 spec/javascripts/stimulus/select_all_controller_test.js diff --git a/app/helpers/admin/order_cycles_helper.rb b/app/helpers/admin/order_cycles_helper.rb index 6bc3fea258..9ce47e6f4c 100644 --- a/app/helpers/admin/order_cycles_helper.rb +++ b/app/helpers/admin/order_cycles_helper.rb @@ -2,18 +2,19 @@ module Admin module OrderCyclesHelper - def order_cycle_shared_payment_methods(order_cycle) - order_cycle.attachable_payment_methods. - where("distributor_id IN (?)", order_cycle.distributors.select(:id)). - group("spree_payment_methods.id"). - having("COUNT(DISTINCT(distributor_id)) > 1") + def order_cycle_distributors_payment_methods(order_cycle) + Spree::PaymentMethod. + joins(:distributors). + includes(:distributors). + available(:both). + where("distributor_id IN (?)", order_cycle.distributors.select(:id)) end - def order_cycle_shared_shipping_methods(order_cycle) - order_cycle.attachable_shipping_methods. - where("distributor_id IN (?)", order_cycle.distributors.select(:id)). - group("spree_shipping_methods.id"). - having("COUNT(DISTINCT(distributor_id)) > 1") + def order_cycle_distributors_shipping_methods(order_cycle) + Spree::ShippingMethod. + joins(:distributors). + includes(:distributors). + where("distributor_id IN (?)", order_cycle.distributors.select(:id)) end end end diff --git a/app/models/spree/shipping_method.rb b/app/models/spree/shipping_method.rb index 589bc8ce8c..44cac5eab0 100644 --- a/app/models/spree/shipping_method.rb +++ b/app/models/spree/shipping_method.rb @@ -71,6 +71,10 @@ module Spree spree_calculators.__send__ model_name_without_spree_namespace end + def backend? + !frontend? + end + # Some shipping methods are only meant to be set via backend def frontend? display_on != "back_end" diff --git a/app/views/admin/order_cycles/checkout_options.html.haml b/app/views/admin/order_cycles/checkout_options.html.haml index 9b87b807c0..dd5841a020 100644 --- a/app/views/admin/order_cycles/checkout_options.html.haml +++ b/app/views/admin/order_cycles/checkout_options.html.haml @@ -3,8 +3,8 @@ - content_for :page_title do = t :edit_order_cycle -- shared_payment_methods = order_cycle_shared_payment_methods(@order_cycle) -- shared_shipping_methods = order_cycle_shared_shipping_methods(@order_cycle) +- payment_methods = order_cycle_distributors_payment_methods(@order_cycle) +- shipping_methods = order_cycle_distributors_shipping_methods(@order_cycle) = form_for [main_app, :admin, @order_cycle], html: { class: "order_cycle" } do |f| @@ -15,66 +15,58 @@ = hidden_field_tag "order_cycle[selected_shipping_method_ids][]", "" - %table.checkout-options - %thead - %tr - %th= t('.distributor') - %th= t('.shipping_methods') - %th= t('.payment_methods') - - @order_cycle.distributors.each do |distributor| - - payment_methods = @order_cycle.attachable_payment_methods.where("distributor_id = ?", distributor.id) - shared_payment_methods - - shipping_methods = @order_cycle.attachable_shipping_methods.where("distributor_id = ?", distributor.id) - shared_shipping_methods - %tr - %td= distributor.name - %td - - shipping_methods.each do |shipping_method| - %p - %label - = check_box_tag "order_cycle[selected_shipping_method_ids][]", - shipping_method.id, @order_cycle.shipping_methods.include?(shipping_method), - id: "order_cycle_selected_shipping_method_ids_#{shipping_method.id}" - = shipping_method.name - - distributor.shipping_methods.backend.each do |shipping_method| - %label.disabled - = check_box_tag nil, nil, false, disabled: true - = shipping_method.name - = "(#{t('.back_end')})" - - if distributor.shipping_methods.frontend.none? - %p - = t('.no_shipping_methods') - %td - - if distributor.payment_methods.available(:both).any? - %ul - - payment_methods.each do |payment_method| - %li= payment_method.name - - else - %p - = t('.no_payment_methods') - - if shared_payment_methods.any? || shared_shipping_methods.any? - %tr - %td= t('.shared') - %td - - shared_shipping_methods.each do |shared_shipping_method| - %p - %label - = check_box_tag "order_cycle[selected_shipping_method_ids][]", - shared_shipping_method.id, @order_cycle.shipping_methods.include?(shared_shipping_method), - id: "order_cycle_selected_shipping_method_ids_#{shared_shipping_method.id}" - = shared_shipping_method.name - %p - — - %em - = shared_shipping_method.distributors.where(id: @order_cycle.distributor_ids).map(&:name).join(", ") - %td - - if shared_payment_methods.any? - %ul - - shared_payment_methods.each do |shared_payment_method| - %li - = shared_payment_method.name - %p + .row + .three.columns +   + .ten.columns + %table.checkout-options + %thead + %tr + %th{ colspan: 2 }= t('.shipping_methods') + %tr{ "data-controller": "select-all" } + %td.text-center + %label + = check_box_tag "bla", nil, shipping_methods == @order_cycle.shipping_methods, { "data-action": "change->select-all#toggleAll", "data-select-all-target": "all" } + = "Select all" + %td + - if shipping_methods.any? + - shipping_methods.each do |shipping_method| + %p + %label{ class: ("disabled" if shipping_method.backend?) } + = check_box_tag "order_cycle[selected_shipping_method_ids][]", + shipping_method.id, @order_cycle.shipping_methods.include?(shipping_method), + disabled: shipping_method.backend?, + id: "order_cycle_selected_shipping_method_ids_#{shipping_method.id}", + data: ({ "action" => "change->select-all#toggleCheckbox", "select-all-target" => "checkbox" } unless shipping_method.backend?) + = shipping_method.name + - if shipping_method.backend? + = "(#{t('.back_end')})" + - if @order_cycle.distributors.many? — - %em - = shared_payment_method.distributors.where(id: @order_cycle.distributor_ids).map(&:name).join(", ") + %small + %em + = shipping_method.distributors.map(&:name).join(", ") + - else + %p + = t('.no_shipping_methods') + %tr + %th{ colspan: 2 }= t('.payment_methods') + %tr + %td + %td + - if payment_methods.any? + %ul + - payment_methods.each do |payment_method| + %li + = payment_method.name + - if @order_cycle.distributors.many? + — + %small + %em + = payment_method.distributors.map(&:name).join(", ") + - else + %p + = t('.no_payment_methods') %div#save-bar %div.container diff --git a/app/webpacker/controllers/select_all_controller.js b/app/webpacker/controllers/select_all_controller.js new file mode 100644 index 0000000000..84c1cc2e63 --- /dev/null +++ b/app/webpacker/controllers/select_all_controller.js @@ -0,0 +1,15 @@ +import { Controller } from "stimulus"; + +export default class extends Controller { + static targets = ["all", "checkbox"]; + + toggleAll() { + this.checkboxTargets.forEach(checkbox => { + checkbox.checked = this.allTarget.checked; + }); + } + + toggleCheckbox() { + this.allTarget.checked = this.checkboxTargets.every(checkbox => checkbox.checked); + } +} diff --git a/config/locales/en.yml b/config/locales/en.yml index 9d2171d249..bc9be69a68 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1166,13 +1166,11 @@ en: back_end: "Back office only" cancel: "Cancel" checkout_options: "Checkout options" - distributor: "Distributor" no_payment_methods: Each distributor on this order cycle requires at least one payment method. no_shipping_methods: Each distributor on this order cycle requires at least one shipping method. payment_methods: "Payment Methods" save: "Save" save_and_back_to_list: "Save and Back to List" - shared: "Shared" shipping_methods: "Shipping Methods" wizard_progress: edit: "1. General Settings" diff --git a/spec/javascripts/stimulus/select_all_controller_test.js b/spec/javascripts/stimulus/select_all_controller_test.js new file mode 100644 index 0000000000..01731cb238 --- /dev/null +++ b/spec/javascripts/stimulus/select_all_controller_test.js @@ -0,0 +1,82 @@ +/** + * @jest-environment jsdom + */ + +import { Application } from "stimulus"; +import select_all_controller from "../../../app/webpacker/controllers/select_all_controller"; + +describe("SelectAllController", () => { + beforeAll(() => { + const application = Application.start(); + application.register("select-all", select_all_controller); + }); + + beforeEach(() => { + document.body.innerHTML = ` +
+ + + +
+ `; + }); + + describe("#toggleAll", () => { + it("checks all checkboxes when it's checked and unchecks them all when unchecked", () => { + const selectAllCheckbox = document.getElementById("selectAllCheckbox"); + const checkboxA = document.getElementById("checkboxA"); + const checkboxB = document.getElementById("checkboxB"); + expect(selectAllCheckbox.checked).toBe(false); + expect(checkboxA.checked).toBe(false); + expect(checkboxB.checked).toBe(false); + + selectAllCheckbox.click() + + expect(selectAllCheckbox.checked).toBe(true); + expect(checkboxA.checked).toBe(true); + expect(checkboxB.checked).toBe(true); + + selectAllCheckbox.click() + + expect(selectAllCheckbox.checked).toBe(false); + expect(checkboxA.checked).toBe(false); + expect(checkboxB.checked).toBe(false); + }); + }); + + describe("#toggleCheckbox", () => { + it("checks the individual checkbox and checks the select all checkbox if all checkboxes are checked and vice versa", () => { + const selectAllCheckbox = document.getElementById("selectAllCheckbox"); + const checkboxA = document.getElementById("checkboxA"); + const checkboxB = document.getElementById("checkboxB"); + checkboxA.click() + expect(selectAllCheckbox.checked).toBe(false); + expect(checkboxA.checked).toBe(true); + expect(checkboxB.checked).toBe(false); + + checkboxB.click() + + expect(selectAllCheckbox.checked).toBe(true); + expect(checkboxA.checked).toBe(true); + expect(checkboxB.checked).toBe(true); + + checkboxB.click() + + expect(selectAllCheckbox.checked).toBe(false); + expect(checkboxA.checked).toBe(true); + expect(checkboxB.checked).toBe(false); + }); + }); +});