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?

I tested your patch and it works as expected
Thanks for taking the time to rework it.

There is one thing I would still prefer to do differently regarding @common_group_ids .
In the current form it performs an extra query per selected user, and it also applies an exclusive logic:
when I select user 1 and user 2 and both are members of different groups, I end up with nothing to select.

For me, it would be clearer and more consistent to handle this the same way as the Add group selection works with your patch, where all groups are always offered regardless of existing memberships. I actually think this behavior is much better and clearer from a UX point of view.
The following code would therefore be my suggestion here, as it better aligns with that behavior.

@common_group_ids = Group.givable.joins(:groups_users).where(groups_users: { user_id: @users.map(&:id) }).distinct.pluck(:id).to_set

Starting from the 0003 patch (and changed in the 0004 patch), I'm not sure where the initial @back_url variable comes from which is accessed from the view.

In any case, the back_url generally needs to be validated before it can be used. At least the Cancel link does not do this in the patch series and would thus allow an attacker to add an unvalidated link there (and thus a possible XSS).