Merge pull request #7552 from dmarcoux/issue-7546

Align usage of flash messages for groups/users_controller

src/api/app/controllers/webui/groups/users_controller.rb

@@ -10,18 +10,19 @@ def create
 
         user = User.find_by(login: params[:user_login])
         if user.nil?
-          flash.now[:error] = "User '#{params[:user_login]}' not found"
-          render 'webui2/webui/webui/flash', status: :not_found
+          flash[:error] = "User '#{params[:user_login]}' not found"
+          redirect_to group_show_path(@group)
           return
         end
+
         group_user = GroupsUser.create(user: user, group: @group)
         if group_user.valid?
-          flash[:success] = "Added user '#{user}' to group"
-          redirect_to group_show_path(@group)
+          flash[:success] = "Added user '#{user}' to group '#{@group}'"
         else
-          flash.now[:error] = "Couldn't add user '#{user}' to group '#{@group}': #{group_user.errors.full_messages.to_sentence}"
-          render 'webui2/webui/webui/flash', status: :bad_request
+          flash[:error] = "Couldn't add user '#{user}' to group '#{@group}': #{group_user.errors.full_messages.to_sentence}"
         end
+
+        redirect_to group_show_path(@group)
       end
 
       def destroy
@@ -44,15 +45,15 @@ def update
           # FIXME: This should be an attribute of GroupsUser
           group_maintainer = GroupMaintainer.create(user: @user, group: @group)
           if group_maintainer.valid?
-            flash.now[:success] = "Gave maintainer rights to #{@user}"
+            flash.now[:success] = "Gave maintainer rights to '#{@user}'"
             render 'webui2/webui/webui/flash', status: :ok
           else
             flash.now[:error] = "Couldn't make user '#{user}' maintainer: #{group_maintainer.errors.full_messages.to_sentence}"
             render 'webui2/webui/webui/flash', status: :bad_request
           end
         else
           @group.group_maintainers.where(user: @user).destroy_all
-          flash.now[:success] = "Removed maintainer rights from #{@user}"
+          flash.now[:success] = "Removed maintainer rights from '#{@user}'"
           render 'webui2/webui/webui/flash', status: :ok
         end
       end
@@ -71,7 +72,7 @@ def set_user
         @user = @group.users.find_by(login: params[:user_login])
         return if @user
 
-        flash.now[:error] = "User '#{params[:user_login]}' not found"
+        flash.now[:error] = "User '#{params[:user_login]}' not found in group '#{@group}'"
         render 'webui2/webui/webui/flash', status: :not_found
       end
     end

src/api/spec/bootstrap/features/webui/groups_spec.rb

@@ -70,7 +70,7 @@ def group_in_datatable(page, group)
       check('Maintainer', allow_label_click: true)
     end
 
-    expect(page).to have_content("Gave maintainer rights to #{admin}")
+    expect(page).to have_content("Gave maintainer rights to '#{admin}'")
   end
 
   scenario 'add a group member' do
@@ -87,7 +87,7 @@ def group_in_datatable(page, group)
       end.to change { group_2.users.count }.by(1)
     end
 
-    expect(page).to have_content("Added user '#{admin}' to group")
+    expect(page).to have_content("Added user '#{admin}' to group '#{group_2}'")
 
     visit groups_path
     group_in_datatable(page, group_2)

src/api/spec/controllers/webui/groups/users_controller_spec.rb

@@ -5,57 +5,49 @@
   let(:user) { create(:confirmed_user) }
   let(:admin) { create(:admin_user) }
 
-  RSpec.shared_examples 'response for non existing user or group' do
-    it 'reports an error' do
-      expect(response).to have_http_status(:not_found)
-      expect(flash[:error]).not_to be_nil
-    end
-  end
-
   describe 'POST create' do
     before do
       login(admin)
     end
 
     context 'when the user exists' do
       subject! do
-        post :create, params: { group_title: group.title, user_login: user.login }, format: :js
+        post :create, params: { group_title: group.title, user_login: user.login }
       end
 
       it 'adds the user to the group' do
         expect(response).to redirect_to(group_show_path(title: group.title))
-        expect(flash[:success]).not_to be_nil
+        expect(flash[:success]).to eq("Added user '#{user}' to group '#{group}'")
         expect(group.users.where(groups_users: { user: user })).to exist
       end
     end
 
     context 'when the user does not exist' do
       subject! do
-        post :create, params: { group_title: group.title, user_login: 'unknown_user' }, format: :js
+        post :create, params: { group_title: group.title, user_login: 'unknown_user' }
       end
 
-      include_examples 'response for non existing user or group'
+      it { expect(flash[:error]).to eq("User 'unknown_user' not found") }
       it { expect(group.users.where(groups_users: { user: user })).not_to exist }
     end
 
     context 'when the group does not exist' do
       subject! do
-        post :create, params: { group_title: 'unknown_group', user_login: user.login }, format: :js
+        post :create, params: { group_title: 'unknown_group', user_login: user.login }
       end
 
-      include_examples 'response for non existing user or group'
+      it { expect(flash[:error]).to eq("Group 'unknown_group' not found") }
     end
 
     context 'when there is an error during user creation' do
       let!(:group_user) { create(:groups_user, group: group, user: user) }
 
       subject! do
-        post :create, params: { group_title: group.title, user_login: user.login }, format: :js
+        post :create, params: { group_title: group.title, user_login: user.login }
       end
 
       it 'reports the error' do
-        expect(response).to have_http_status(:bad_request)
-        expect(flash[:error]).not_to be_nil
+        expect(flash[:error]).to eq("Couldn't add user '#{user}' to group '#{group}': User User already has this group")
       end
     end
   end
@@ -71,7 +63,7 @@
       end
 
       it 'responds with an error message' do
-        expect(flash[:error]).not_to be_nil
+        expect(flash[:error]).to eq("User '#{user}' not found in group '#{group}'")
         expect(response).to have_http_status(:not_found)
       end
     end
@@ -85,7 +77,7 @@
 
       it 'removes the user from the group' do
         expect(response).to have_http_status(:success)
-        expect(flash[:success]).not_to be_nil
+        expect(flash[:success]).to eq("Removed user from group '#{group}'")
         expect(group.users.where(groups_users: { user: user })).not_to exist
       end
     end
@@ -95,7 +87,8 @@
         delete :destroy, params: { group_title: group.title, user_login: 'unknown_user' }, format: :js
       end
 
-      include_examples 'response for non existing user or group'
+      it { expect(response).to have_http_status(:not_found) }
+      it { expect(flash[:error]).to eq("User 'unknown_user' not found in group '#{group}'") }
       it { expect(group.users.where(groups_users: { user: user })).not_to exist }
     end
 
@@ -104,7 +97,8 @@
         delete :destroy, params: { group_title: 'unknown_group', user_login: user.login }, format: :js
       end
 
-      include_examples 'response for non existing user or group'
+      it { expect(response).to have_http_status(:not_found) }
+      it { expect(flash[:error]).to eq("Group 'unknown_group' not found") }
     end
   end
 
@@ -135,7 +129,7 @@
 
         it 'removes maintainer rights from the group' do
           expect(response).to have_http_status(:success)
-          expect(flash[:success]).not_to be_nil
+          expect(flash[:success]).to eq("Removed maintainer rights from '#{user}'")
           expect(group.maintainer?(user)).to be false
         end
       end
@@ -147,7 +141,7 @@
 
         it 'gives maintainer rights to the user' do
           expect(response).to have_http_status(:success)
-          expect(flash[:success]).not_to be_nil
+          expect(flash[:success]).to eq("Gave maintainer rights to '#{user}'")
           expect(group.maintainer?(user)).to be true
         end
       end
@@ -158,7 +152,8 @@
         post :update, params: { group_title: group.title, user_login: 'unknown_user', maintainer: 'true' }, format: :js
       end
 
-      include_examples 'response for non existing user or group'
+      it { expect(response).to have_http_status(:not_found) }
+      it { expect(flash[:error]).to eq("User 'unknown_user' not found in group '#{group}'") }
       it { expect(group.users.where(groups_users: { user: user })).not_to exist }
     end
 
@@ -167,7 +162,8 @@
         post :update, params: { group_title: 'unknown_group', user_login: user.login, maintainer: 'true' }, format: :js
       end
 
-      include_examples 'response for non existing user or group'
+      it { expect(response).to have_http_status(:not_found) }
+      it { expect(flash[:error]).to eq("Group 'unknown_group' not found") }
     end
   end
 end