Merge pull request #1747 from tvdeyen/fix-admin-users-role-update

Do not clear roles when no role params are present

CHANGELOG.md

@@ -28,6 +28,9 @@
 *   New method Spree.formatMoney(amount, currency) is available to javascript in
     the admin.
 
+*   Fix an issue where updating a user in the admin without specifying roles in
+    the params would clear the user's roles.
+
 *   Deprecations
 
     * `cache_key_for_taxons` helper has been deprecated in favour of `cache [I18n.locale, @taxons]`

backend/app/controllers/spree/admin/users_controller.rb

@@ -148,15 +148,8 @@ def load_stock_locations
       end
 
       def set_roles
-        # 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]
+        if user_params[:spree_role_ids] && can?(:manage, Spree::Role)
           @user.spree_roles = Spree::Role.where(id: user_params[:spree_role_ids])
-        elsif can?(:manage, Spree::Role)
-          @user.spree_roles = []
         end
       end
 

backend/app/views/spree/admin/users/_form.html.erb

@@ -16,6 +16,7 @@
         <%= label_tag nil, plural_resource_name(Spree::Role) %>
         <ul>
           <% if can? :manage, Spree::Role %>
+            <%= hidden_field_tag('user[spree_role_ids][]', nil) %>
             <% @roles.each do |role| %>
               <li>
                 <%= check_box_tag 'user[spree_role_ids][]', role.id, @user_roles.include?(role), id: "user_spree_role_#{role.name}" %>

backend/spec/controllers/spree/admin/users_controller_spec.rb

@@ -149,9 +149,17 @@ def user
       it "can clear roles" do
         user.spree_roles << dummy_role
         expect {
-          put :update, params: { id: user.id, user: { first_name: "Bob", spree_role_ids: [] } }
+          put :update, params: { id: user.id, user: { first_name: "Bob", spree_role_ids: [""] } }
         }.to change { user.reload.spree_roles.to_a }.to([])
       end
+
+      context 'when no role params are present' do
+        it 'does not clear all present user roles' do
+          user.spree_roles << dummy_role
+          put :update, params: { id: user.id, user: { first_name: "Bob" } }
+          expect(user.reload.spree_roles).to_not be_empty
+        end
+      end
     end
 
     context "when the user cannot manage roles" do

backend/spec/features/admin/users_spec.rb

@@ -128,6 +128,20 @@
       expect(find_field('user_spree_role_admin')).to be_checked
     end
 
+    it 'can delete user roles' do
+      user_a.spree_roles << Spree::Role.create(name: "dummy")
+      click_link 'Account'
+
+      user_a.spree_roles.each do |role|
+        uncheck "user_spree_role_#{role.name}"
+      end
+
+      click_button 'Update'
+      expect(page).to have_text 'Account updated'
+      expect(find_field('user_spree_role_dummy')).not_to be_checked
+      expect(user_a.reload.spree_roles).to be_empty
+    end
+
     it 'can edit user shipping address' do
       click_link "Addresses"