From 278a8b1ec21457c79a0909ff32c9945296970210 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Rodr=C3=ADguez?= <2887858+deivid-rodriguez@users.noreply.github.com> Date: Tue, 4 Nov 2025 11:06:40 +0100 Subject: [PATCH 1/7] Let save-bar properly track form state * Keep save bar visible as long as there's a customer form displayed. * Only display "You have unsaved changes" when there's any difference from the original values. If form changes are reverted, hide that note. * Similarly, only let the button be enabled if there are any actual changes to be saved. --- .../customers/controllers/customers_controller.js.coffee | 3 +++ .../admin/index_utils/services/pending_changes.js.coffee | 8 +++++++- app/views/admin/customers/index.html.haml | 4 ++-- spec/system/admin/customers_spec.rb | 7 +++++++ 4 files changed, 19 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/admin/customers/controllers/customers_controller.js.coffee b/app/assets/javascripts/admin/customers/controllers/customers_controller.js.coffee index 9fdd44672c..573468f9ab 100644 --- a/app/assets/javascripts/admin/customers/controllers/customers_controller.js.coffee +++ b/app/assets/javascripts/admin/customers/controllers/customers_controller.js.coffee @@ -11,6 +11,9 @@ angular.module("admin.customers").controller "customersCtrl", ($scope, $q, $filt $scope.confirmRefresh = (event) -> event.preventDefault() unless pendingChanges.unsavedCount() == 0 || confirm(t("unsaved_changes_warning")) + $scope.hasUnsavedChanges = -> + pendingChanges.yes() + $scope.$watch "shop_id", -> if $scope.shop_id? CurrentShop.shop = $filter('filter')($scope.shops, {id: parseInt($scope.shop_id)}, true)[0] diff --git a/app/assets/javascripts/admin/index_utils/services/pending_changes.js.coffee b/app/assets/javascripts/admin/index_utils/services/pending_changes.js.coffee index 69373cc7e9..dafab4a4c3 100644 --- a/app/assets/javascripts/admin/index_utils/services/pending_changes.js.coffee +++ b/app/assets/javascripts/admin/index_utils/services/pending_changes.js.coffee @@ -16,7 +16,10 @@ angular.module("admin.indexUtils").factory "pendingChanges", ($q, resources, Sta remove: (id, attr) => if @pendingChanges.hasOwnProperty("#{id}") delete @pendingChanges["#{id}"]["#{attr}"] - delete @pendingChanges["#{id}"] if @changeCount( @pendingChanges["#{id}"] ) < 1 + + if @changeCount( @pendingChanges["#{id}"] ) < 1 + delete @pendingChanges["#{id}"] + StatusMessage.clear() submitAll: (form=null) => all = [] @@ -47,5 +50,8 @@ angular.module("admin.indexUtils").factory "pendingChanges", ($q, resources, Sta unsavedCount: -> Object.keys(@pendingChanges).length + yes: -> + @unsavedCount() > 0 + changeCount: (objectChanges) -> Object.keys(objectChanges).length diff --git a/app/views/admin/customers/index.html.haml b/app/views/admin/customers/index.html.haml index 3ba03ca538..a61fc61cf9 100644 --- a/app/views/admin/customers/index.html.haml +++ b/app/views/admin/customers/index.html.haml @@ -44,8 +44,8 @@ %h1#no_results{ 'ng-show' => '!RequestMonitor.loading && filteredCustomers.length == 0' } =t :no_customers_found - %save-bar{ dirty: "customers_form.$dirty", persist: "false" } - %input.red{ type: "button", value: t(:save_changes), "ng-click": "submitAll(customers_form)" } + %save-bar{ persist: "filteredCustomers.length > 0" } + %input.red{ type: "button", value: t(:save_changes), "ng-click": "submitAll(customers_form)", "ng-disabled": "!hasUnsavedChanges()" } %table.index#customers{ 'ng-show' => '!RequestMonitor.loading && filteredCustomers.length > 0' } %col.email{ width: "20%", 'ng-show' => 'columns.email.visible' } diff --git a/spec/system/admin/customers_spec.rb b/spec/system/admin/customers_spec.rb index fdcc76b673..d9b8224514 100644 --- a/spec/system/admin/customers_spec.rb +++ b/spec/system/admin/customers_spec.rb @@ -259,6 +259,13 @@ RSpec.describe 'Customers' do it "allows updating of attributes" do select2_select managed_distributor1.name, from: "shop_id" + expect(page).to have_button "Save Changes", disabled: true + + # Editing attributes but undoing changes + within("tr#c_#{customer1.id}") { fill_in "first_name", with: "customer abc" } + expect(page).to have_content 'You have unsaved changes' + within("tr#c_#{customer1.id}") { fill_in "first_name", with: "John" } + expect(page).not_to have_content 'You have unsaved changes' within "tr#c_#{customer1.id}" do expect(find_field('first_name').value).to eq 'John' From 4b31352e4f0977dc980747d4a68cdc2f8c090661 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Rodr=C3=ADguez?= <2887858+deivid-rodriguez@users.noreply.github.com> Date: Tue, 4 Nov 2025 19:11:43 +0100 Subject: [PATCH 2/7] Wait for page before checking DB --- spec/system/admin/customers_spec.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/system/admin/customers_spec.rb b/spec/system/admin/customers_spec.rb index d9b8224514..66ce746ecc 100644 --- a/spec/system/admin/customers_spec.rb +++ b/spec/system/admin/customers_spec.rb @@ -214,6 +214,7 @@ RSpec.describe 'Customers' do expect(page).to have_content 'You have unsaved changes' click_button "Save Changes" + expect(page).to have_content 'All changes saved successfully' # changes are saved in the database expect(customer4.reload.code).to eq(nil) @@ -250,6 +251,7 @@ RSpec.describe 'Customers' do expect(page).to have_content 'You have unsaved changes' click_button "Save Changes" + expect(page).to have_content 'All changes saved successfully' expect(customer4.reload.tag_list).to be_empty end From 3d7207d8c51a8db3f5e08fbf9cd2646b7ca232bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Rodr=C3=ADguez?= <2887858+deivid-rodriguez@users.noreply.github.com> Date: Tue, 4 Nov 2025 11:27:27 +0100 Subject: [PATCH 3/7] Properly track changes in `code` attribute If the code was initially nil, some value is added, and then removed, we would not detect that the code has not actually changed. --- .../admin/index_utils/directives/obj_for_update.js.coffee | 7 ++++--- spec/system/admin/customers_spec.rb | 4 ++++ 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/admin/index_utils/directives/obj_for_update.js.coffee b/app/assets/javascripts/admin/index_utils/directives/obj_for_update.js.coffee index 3ed4e6c98f..bf1edfdc20 100644 --- a/app/assets/javascripts/admin/index_utils/directives/obj_for_update.js.coffee +++ b/app/assets/javascripts/admin/index_utils/directives/obj_for_update.js.coffee @@ -4,15 +4,16 @@ angular.module("admin.indexUtils").directive "objForUpdate", (switchClass, pendi type: "@objForUpdate" attr: "@attrForUpdate" link: (scope, element, attrs) -> - scope.savedValue = scope.object()[scope.attr] + scope.savedValue = scope.object()[scope.attr] || "" scope.$watch "object().#{scope.attr}", (value) -> - if value == scope.savedValue + strValue = value || "" + if strValue == scope.savedValue pendingChanges.remove(scope.object().id, scope.attr) scope.clear() else scope.pending() - addPendingChange(scope.attr, value ? "") + addPendingChange(scope.attr, strValue) scope.reset = (value) -> scope.savedValue = value diff --git a/spec/system/admin/customers_spec.rb b/spec/system/admin/customers_spec.rb index 66ce746ecc..2033654f41 100644 --- a/spec/system/admin/customers_spec.rb +++ b/spec/system/admin/customers_spec.rb @@ -268,6 +268,10 @@ RSpec.describe 'Customers' do expect(page).to have_content 'You have unsaved changes' within("tr#c_#{customer1.id}") { fill_in "first_name", with: "John" } expect(page).not_to have_content 'You have unsaved changes' + within("tr#c_#{customer1.id}") { fill_in "code", with: "new-customer-code" } + expect(page).to have_content 'You have unsaved changes' + within("tr#c_#{customer1.id}") { fill_in "code", with: "" } + expect(page).not_to have_content 'You have unsaved changes' within "tr#c_#{customer1.id}" do expect(find_field('first_name').value).to eq 'John' From e990e5ffd52e464a3dc9297d374f323f50512e6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Rodr=C3=ADguez?= <2887858+deivid-rodriguez@users.noreply.github.com> Date: Tue, 4 Nov 2025 11:29:51 +0100 Subject: [PATCH 4/7] Don't show flash messages in customer edition form They don't actually show up when the customer is saved, but the next time the page is reloaded. We already have the save bar for the same purpose so it's not necessary. --- app/controllers/admin/customers_controller.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/app/controllers/admin/customers_controller.rb b/app/controllers/admin/customers_controller.rb index ff5c43ab63..a7e0038e80 100644 --- a/app/controllers/admin/customers_controller.rb +++ b/app/controllers/admin/customers_controller.rb @@ -51,6 +51,18 @@ module Admin end end + # copy of Admin::ResourceController without flash notice + def update + if @object.update(permitted_resource_params) + respond_with(@object) do |format| + format.html { redirect_to location_after_save } + format.js { render layout: false } + end + else + respond_with(@object) + end + end + # copy of Admin::ResourceController without flash notice def destroy if @object.destroy From 186fe0503fb2fcf8e12bd89c8b6dd5485583f349 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Rodr=C3=ADguez?= <2887858+deivid-rodriguez@users.noreply.github.com> Date: Tue, 4 Nov 2025 11:32:39 +0100 Subject: [PATCH 5/7] Show orange border when input has changes Even if it's on focus. --- app/webpacker/css/admin/orders.scss | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/webpacker/css/admin/orders.scss b/app/webpacker/css/admin/orders.scss index 2e51e74936..5aec38c538 100644 --- a/app/webpacker/css/admin/orders.scss +++ b/app/webpacker/css/admin/orders.scss @@ -9,6 +9,10 @@ input, div { &.update-pending { border: solid 1px orange; + + &:focus { + border: solid 1px orange; + } } } From 6901323827a3001864194aa1bbf1f1ed527cdc5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Rodr=C3=ADguez?= <2887858+deivid-rodriguez@users.noreply.github.com> Date: Tue, 4 Nov 2025 12:51:45 +0100 Subject: [PATCH 6/7] Fix success message taking 5 seconds to show up This is pretty black magic to me, but my understanding is that: * When submitting customer forms, we use `$q.all()` on the result of submitting each form asynchronously in order to decide whether to display a success message (no errors) or a failure message. * The value returned for each particular form submission was the return value of either `change.scope.success()` or `change.scope.error()`. These use the `switchClass` factory, which changes a particular DOM element's class to the proper pending/success/error class, but in the success case, it also sets a timeout to remove the class using `$timeout()`, which is a promise, and that was its return value. * Because of the above, `$q.all()` was actually waiting for the `$timeout()` promise to be fulfilled before proceeding. The fix is to not return a `$timeout()` promise from the `switchClass` factory when a timeout is passed, but instead set a timeout on the element, but return the element itself regardless. --- .../admin/index_utils/services/switch_class.js.coffee | 1 + 1 file changed, 1 insertion(+) diff --git a/app/assets/javascripts/admin/index_utils/services/switch_class.js.coffee b/app/assets/javascripts/admin/index_utils/services/switch_class.js.coffee index 0be1f2ec40..a8e8a584b2 100644 --- a/app/assets/javascripts/admin/index_utils/services/switch_class.js.coffee +++ b/app/assets/javascripts/admin/index_utils/services/switch_class.js.coffee @@ -8,3 +8,4 @@ angular.module("admin.indexUtils").factory "switchClass", ($timeout) -> element.timeout = $timeout(-> element.removeClass classToAdd , timeout, true) + element From f6d605a3aa12c4cfbd75975a741f20a49e771a38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Rodr=C3=ADguez?= <2887858+deivid-rodriguez@users.noreply.github.com> Date: Tue, 4 Nov 2025 13:17:36 +0100 Subject: [PATCH 7/7] Dismiss success message automatically after 5 seconds We were already eventually removing the "success" border style on inputs. I think it makes sense to do the same for the success message itself. That's how our standard "flash messages" already work. --- .../admin/index_utils/services/switch_class.js.coffee | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/admin/index_utils/services/switch_class.js.coffee b/app/assets/javascripts/admin/index_utils/services/switch_class.js.coffee index a8e8a584b2..46eb0dc298 100644 --- a/app/assets/javascripts/admin/index_utils/services/switch_class.js.coffee +++ b/app/assets/javascripts/admin/index_utils/services/switch_class.js.coffee @@ -1,4 +1,4 @@ -angular.module("admin.indexUtils").factory "switchClass", ($timeout) -> +angular.module("admin.indexUtils").factory "switchClass", ($timeout, StatusMessage) -> return (element, classToAdd, removeClasses, timeout) -> $timeout.cancel element.timeout if element.timeout element.removeClass className for className in removeClasses @@ -7,5 +7,6 @@ angular.module("admin.indexUtils").factory "switchClass", ($timeout) -> if timeout && intRegex.test(timeout) element.timeout = $timeout(-> element.removeClass classToAdd + StatusMessage.clear() , timeout, true) element