Redmine

Generally, I like this feature. I would significantly improve usability for admins with a lots of users.

Some random notes:

• Instead of post 'groups/:id/remove_users', you could also use delete 'groups/:id/users' in config/routes.rb to match the existing POST URL. We have proper HTTP verbs, let's use them :)
• Why are you manually checking the referer in the controller? Can't you just use the redirect_back_or_default helper?
• Why are you rendering an API error in GroupsController#remove_users if no user matched? An empty set seems entirely correct and I believe this is also valid everywhere else (including the HTML part)
• The generic confirm: l(:text_are_you_sure) Alert is rather pointless. Yes, there are a few instances of these in Redmine, but we should not add more. Better add a real confirmation which specifically confirms the action taken, including a full list of users being added / deleted.
• The context menu option to delete users from a group should filter the groups the selected users are actually in instead of offering all groups. And maybe even disable the option unless there are any groups which each include all of the selected users.
• In the tests, both the existing route as well as the new route should be tested.

Thank you, Holger, for your suggestions.

I will address them in the next patch. Please let me know if I understood everything correctly:

• Switch the route from POST to `DELETE /groups/:id/users`
  

  delete 'groups/:id/users', :to => 'groups#remove_users', :id => /\d+/, :as => 'group_remove_users'
  

• Use `redirect_back_or_default` and show the flash notice only when returning to the Users view.
• For the API, return success even if no users matched, to keep the operation idempotent.
  Should I also change something in the add_users method? I copied it from there.
  

    def remove_users
      @users = User.in_group(@group).where(:id => (params[:user_id] || params[:user_ids])).to_a
      @group.users.delete(@users)
      respond_to do |format|
        format.html do
          flash[:notice] = l(:notice_successful_delete) unless request.referer.presence == edit_group_path(@group, :tab => 'users')
          redirect_back_or_default edit_group_path(@group, tab: 'users'), referer: true
        end
        format.js
        format.api {render_api_ok}
      end
    end
  

• Add a specific confirmation dialog that lists the affected users (or shows a count when there are more than 10).
  i will add a new helper, i can not find any in the redmine core
• Filter the “Delete from group” context-menu options to only show groups that actually include the selected users (and disable the action when none do).
• In the current test patch, which test is missing here?

Does this align with your expectations before I submit the patch?