Extract editing member into a separate service

Would have liked to use the contract here to validate the role_ids
before they're being written to attributes, but that doesn't seem to
work.

app/controllers/groups_controller.rb

@@ -143,8 +143,11 @@ class GroupsController < ApplicationController
 
   def create_memberships
     membership_params = permitted_params.group_membership
-    @membership = Member.edit_membership(membership_params[:membership_id], membership_params[:membership], @group)
-    @membership.save
+    membership_id = membership_params[:membership_id]
+    @membership = membership_id.present? ? Member.find(membership_id) : Member.new(principal: @group)
+
+    service = ::Members::EditMembershipService.new(@membership, save: true, current_user: current_user)
+    service.call(attributes: membership_params[:membership])
 
     respond_to do |format|
       format.html do redirect_to controller: '/groups', action: 'edit', id: @group, tab: 'memberships' end

app/controllers/users/memberships_controller.rb

@@ -61,8 +61,10 @@ class Users::MembershipsController < ApplicationController
   private
 
   def update_or_create(save_record)
-    @membership = Member.edit_membership(params[:id], permitted_params.membership, @user)
-    @membership.save if save_record
+    @membership = params[:id].present? ? Member.find(params[:id]) : Member.new(principal: @user)
+    service = ::Members::EditMembershipService.new(@membership, save: save_record, current_user: current_user)
+    service.call(attributes: permitted_params.membership)
+
     respond_to do |format|
       format.html do
         redirect_to controller: '/users', action: 'edit', id: @user, tab: 'memberships'

app/models/member.rb

@@ -130,19 +130,6 @@ class Member < ActiveRecord::Base
       .update_all 'assigned_to_id = NULL' if user
   end
 
-  # Find or initialize a Member with an id, attributes, and for a Principal
-  def self.edit_membership(id, new_attributes, principal = nil)
-    @membership = id.present? ? Member.find(id) : Member.new(principal: principal)
-
-    # Reject any blank values from unselected values
-    if new_attributes.key? :role_ids
-      new_attributes[:role_ids].reject!(&:blank?)
-    end
-
-    @membership.attributes = new_attributes
-    @membership
-  end
-
   ##
   # Returns true if this user can be deleted as they have no other memberships
   # and haven't been activated yet. Only applies if the member is actually a user

app/services/members/edit_membership_service.rb

@@ -0,0 +1,93 @@
+#-- encoding: UTF-8
+#-- copyright
+# OpenProject is a project management system.
+# Copyright (C) 2012-2017 the OpenProject Foundation (OPF)
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2017 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See doc/COPYRIGHT.rdoc for more details.
+#++
+
+module Members
+  class EditMembershipService
+    attr_reader :current_user, :member, :do_save
+
+    def initialize(member, save:, current_user:)
+      @current_user = current_user
+      @member = member
+      @do_save = do_save
+    end
+
+    def call(attributes: {})
+      User.execute_as current_user do
+
+        process_attributes! attributes
+
+        unless validate_attributes! attributes
+          return make_result(success: false)
+        end
+
+        set_attributes(attributes)
+
+        success =
+          if do_save
+            member.save
+          else
+            member.validate
+          end
+
+        make_result(success: success)
+      end
+    end
+
+    private
+
+    def make_result(success:)
+      ServiceResult.new(success: success, errors: member.errors, result: member)
+    end
+
+    def process_attributes!(attributes)
+      # Reject any blank values from unselected values
+      if attributes.key? :role_ids
+        attributes[:role_ids].reject!(&:blank?)
+      end
+    end
+
+    ##
+    # We need to validate attributes before passing them to user
+    # because role_ids are modified instantly and may cause errors if empty
+    def validate_attributes!(attributes)
+      # We need to check for empty roles here because that _implicitly_
+      # deletes the membership and causes failures
+      if attributes[:role_ids].empty?
+        member.errors.add :roles, :role_blank
+        return false
+      end
+
+      true
+    end
+
+    def set_attributes(attributes)
+      member.attributes = attributes
+    end
+  end
+end

app/views/users/memberships/update_or_create.js.erb

@@ -32,7 +32,7 @@ See doc/COPYRIGHT.rdoc for more details.
 
   $('#tab-content-memberships').html(memberships);
 
-  <% if @membership.valid? %>
+  <% if @membership.errors.empty? %>
     var notice = $("<%= escape_javascript(render(partial: 'members/common_notice',
                                                  locals: { message: l(:notice_successful_update) })) %>");
     $('#tab-content-memberships').prepend(notice);