From 72f8a0cc5f334d91e5424c2b2ef37b63bfec2063 Mon Sep 17 00:00:00 2001
From: Richard Wilson <richard@freerunningtech.com>
Date: Mon, 17 Aug 2015 10:58:29 -0700
Subject: [PATCH] Handle edgecase where a user cannot have all roles removed.

When updating a user, I cannot remove the last role from that user. This
is due to an assumption that was made that the form still submitted the
param `spree_role_ids` with an empty array if none of the checkboxes
were checked.

This assumption was false. We have to add behavior around whether or not
the roles can be cleared if that parameter is not present which is not
ideal as explained in the comment in users_controller#set_roles.
---
 .../spree/admin/users_controller.rb           | 13 ++++++--
 .../spree/admin/users_controller_spec.rb      | 31 +++++++++++++++++--
 2 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/backend/app/controllers/spree/admin/users_controller.rb b/backend/app/controllers/spree/admin/users_controller.rb
index 3464f7f5853..a454ea5e2c1 100644
--- a/backend/app/controllers/spree/admin/users_controller.rb
+++ b/backend/app/controllers/spree/admin/users_controller.rb
@@ -159,9 +159,16 @@ def load_stock_locations
         end
 
         def set_roles
-          return unless user_params[:spree_role_ids]
-
-          @user.spree_roles = Spree::Role.where(id: user_params[:spree_role_ids])
+          # FIXME: user_params permits the roles that can be set, if spree_role_ids is set.
+          # when submitting a user with no roles, the param is not present. Because users can be updated
+          # with some users being able to set roles, and some users not being able to set roles, we have to check
+          # if the roles should be cleared, or unchanged again here. The roles form should probably hit a seperate
+          # action or controller to remedy this.
+          if user_params[:spree_role_ids]
+            @user.spree_roles = Spree::Role.where(id: user_params[:spree_role_ids])
+          elsif can?(:manage, Spree::Role)
+            @user.spree_roles = []
+          end
         end
 
         def set_stock_locations
diff --git a/backend/spec/controllers/spree/admin/users_controller_spec.rb b/backend/spec/controllers/spree/admin/users_controller_spec.rb
index efef5e75448..6871016acb2 100644
--- a/backend/spec/controllers/spree/admin/users_controller_spec.rb
+++ b/backend/spec/controllers/spree/admin/users_controller_spec.rb
@@ -77,7 +77,12 @@
     context "when the user can manage roles" do
       it "can set roles" do
         expect(mock_user).to receive(:spree_roles=).with([dummy_role])
-        spree_post :create, { user: { spree_role_ids: [dummy_role.id] } }
+        spree_post :create, { id: mock_user.id, user: { first_name: "Bob", spree_role_ids: [dummy_role.id] } }
+      end
+
+      it "can clear roles" do
+        expect(mock_user).to receive(:spree_roles=).with([])
+        spree_post :create, { id: mock_user.id, user: { first_name: "Bob"}  }
       end
     end
 
@@ -90,6 +95,16 @@
         expect(mock_user).to_not receive(:spree_roles=)
         spree_post :create, { user: { spree_role_ids: [dummy_role.id] } }
       end
+
+      it "cannot set roles" do
+        expect(mock_user).to_not receive(:spree_roles=)
+        spree_post :create, { id: mock_user.id, user: { first_name: "Bob", spree_role_ids: [dummy_role.id] } }
+      end
+
+      it "cannot clear roles" do
+        expect(mock_user).to_not receive(:spree_roles=)
+        spree_post :create, { id: mock_user.id, user: { first_name: "Bob"}  }
+      end
     end
 
     it "can create a shipping_address" do
@@ -125,7 +140,12 @@
     context "when the user can manage roles" do
       it "can set roles" do
         expect(mock_user).to receive(:spree_roles=).with([dummy_role])
-        spree_put :update, { id: mock_user.id, user: { spree_role_ids: [dummy_role.id] } }
+        spree_put :update, { id: mock_user.id, user: { first_name: "Bob", spree_role_ids: [dummy_role.id] } }
+      end
+
+      it "can clear roles" do
+        expect(mock_user).to receive(:spree_roles=).with([])
+        spree_put :update, { id: mock_user.id, user: { first_name: "Bob"}  }
       end
     end
 
@@ -136,7 +156,12 @@
 
       it "cannot set roles" do
         expect(mock_user).to_not receive(:spree_roles=)
-        spree_put :update, { id: mock_user.id, user: { spree_role_ids: [dummy_role.id] } }
+        spree_put :update, { id: mock_user.id, user: { first_name: "Bob", spree_role_ids: [dummy_role.id] } }
+      end
+
+      it "cannot clear roles" do
+        expect(mock_user).to_not receive(:spree_roles=)
+        spree_put :update, { id: mock_user.id, user: { first_name: "Bob" }  }
       end
     end