Merge branch 'release/11.0' into dev

app/models/design_color.rb

@@ -28,10 +28,10 @@
 
 class DesignColor < ApplicationRecord
   after_commit -> do
-    # CustomStyle.current.updated_at determins the cache key for inline_css
+    # CustomStyle.current.updated_at determines the cache key for inline_css
     # in which the CSS color variables will be overwritten. That is why we need
     # to ensure that a CustomStyle.current exists and that the time stamps change
-    # whenever we chagen a color_variable.
+    # whenever we change a color_variable.
     if CustomStyle.current
       CustomStyle.current.touch
     else

db/migrate/20200114091135_add_theme_name_to_custom_styles.rb

@@ -1,5 +1,8 @@
 class AddThemeNameToCustomStyles < ActiveRecord::Migration[6.0]
   def change
-    add_column :custom_styles, :theme, :string, default: "OpenProject"
+    add_column :custom_styles,
+               :theme,
+               :string,
+               default: OpenProject::CustomStyles::ColorThemes::DEFAULT_THEME_NAME
   end
 end

docs/installation-and-operations/installation/packaged/README.md

@@ -75,6 +75,8 @@ sudo apt-get install openproject
 
 Then finish the installation by reading the [*Initial configuration*][initial-config] section.
 
+<iframe width="560" height="315" src="https://www.youtube.com/embed/MTHZVQ_89-k?controls=0" frameborder="0" allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture" allowfullscreen></iframe>
+
 ### Ubuntu 18.04
 
 Import the PGP key used to sign our packages:

docs/release-notes/11-0-3/README.md

@@ -0,0 +1,22 @@
+---
+title: OpenProject 11.0.3
+sidebar_navigation:
+    title: 11.0.3
+release_version: 11.0.3
+release_date: 2020-11-16
+---
+
+# OpenProject 11.0.3
+
+We released [OpenProject 11.0.3](https://community.openproject.com/versions/1456).
+The release contains several bug fixes and we recommend updating to the newest version.
+
+<!--more-->
+#### Bug fixes and changes
+
+- Fixed: Bulk edit: Custom field long text cut off \[[#34829](https://community.openproject.com/wp/34829)\]
+- Fixed: "Estimates and time" not translated to German when not logged in on community.openproject.com \[[#35009](https://community.openproject.com/wp/35009)\]
+- Fixed: IFC viewer can't load models when OP installed on subpath \[[#35129](https://community.openproject.com/wp/35129)\]
+- Fixed: Elder BIM instances get "OpenProject" theme instead of the "OpenProject BIM" theme after migration. \[[#35131](https://community.openproject.com/wp/35131)\]
+- Fixed: Transitive Relations embedded in work package representer -> poor performance \[[#35168](https://community.openproject.com/wp/35168)\]
+- Fixed: LDAP Group sync deletes users when removed from group \[[#35265](https://community.openproject.com/wp/35265)\]

docs/release-notes/README.md

@@ -12,6 +12,13 @@ Stay up to date and get an overview of the new features included in the releases
 <!--- New release notes are generated below. Do not remove comment. -->
 <!--- RELEASE MARKER -->
 
+## 11.0.3
+
+Release date: 2020-11-16
+
+[Release Notes](11-0-3/)
+
+
 ## 11.0.2
 
 Release date: 2020-11-06

docs/user-guide/projects/README.md

@@ -149,6 +149,9 @@ Navigate to the [project settings](project-settings) and click **Set as template
 You can create a new project by using an existing template. This causes the properties of the project template to be copied to the new project. Find out in our Getting started guide how to [create a new project](../../getting-started/projects/#create-a-new-project) in OpenProject. 
 Another way for using a template project would be to [copy it](#copy-a-project).
 
+<iframe width="560" height="315" src="https://www.youtube-nocookie.com/embed/rhXYBrQQLBg?controls=0" frameborder="0" allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture" allowfullscreen></iframe>
+
+
 
 ### Copy a project
 

frontend/src/app/modules/bim/ifc_models/ifc-viewer/ifc-viewer.service.ts

@@ -47,7 +47,7 @@ export class IFCViewerService extends ViewerBridgeService {
 
   public newViewer(elements:XeokitElements, projects:any[]) {
     import('@xeokit/xeokit-bim-viewer/dist/main').then((XeokitViewerModule:any) => {
-      let server = new XeokitServer();
+      let server = new XeokitServer(this.pathHelper);
       let viewerUI = new XeokitViewerModule.BIMViewer(server, elements);
 
       viewerUI.on("queryPicked", (event:any) => {

frontend/src/app/modules/bim/ifc_models/xeokit/xeokit-server.d.ts

@@ -1 +0,0 @@
-export class XeokitServer {}

frontend/src/app/modules/bim/ifc_models/xeokit/xeokit-server.js

@@ -1,78 +0,0 @@
-import {utils} from "@xeokit/xeokit-sdk/src/viewer/scene/utils";
-/**
- * Default server client which loads content via HTTP from the file system.
- */
-class XeokitServer {
-
-  /**
-   *
-   * @param cfg
-   * @param.cfg.dataDir Base directory for content.
-   */
-  constructor(cfg = {}) {
-    this._dataDir = cfg.dataDir || "";
-  }
-
-  /**
-   * Gets the manifest of all projects.
-   * @param done
-   * @param error
-   */
-  getProjects(done, _error) {
-    done({ projects: window.gon.ifc_models.projects });
-  }
-
-  /**
-   * Gets a manifest for a project.
-   * @param projectId
-   * @param done
-   * @param error
-   */
-  getProject(projectData, done, _error) {
-    var manifestData = {
-      id: projectData[0].id,
-      name: projectData[0].name,
-      models: window.gon.ifc_models.models,
-      viewerContent: {
-        modelsLoaded: window.gon.ifc_models.shown_models
-      },
-      viewerConfigs: {
-        saoEnabled: true // Needs to be enabled by default if we want to use it selectively on the available models.
-      }
-    };
-
-    done(manifestData);
-  }
-
-  /**
-   * Gets metadata for a model within a project.
-   * @param projectId
-   * @param modelId
-   * @param done
-   * @param error
-   */
-  getMetadata(_projectId, modelId, done, error) {
-    const attachmentId = window.gon.ifc_models.metadata_attachment_ids[modelId];
-    console.log(`Loading model metadata for: ${attachmentId}`);
-    utils.loadJSON(this.attachmentUrl(attachmentId), done, error);
-  }
-
-  /**
-   * Gets geometry for a model within a project.
-   * @param projectId
-   * @param modelId
-   * @param done
-   * @param error
-   */
-  getGeometry(projectId, modelId, done, error) {
-    const attachmentId = window.gon.ifc_models.xkt_attachment_ids[modelId];
-    console.log(`Loading model geometry for: ${attachmentId}`);
-    utils.loadArraybuffer(this.attachmentUrl(attachmentId), done, error);
-  }
-
-  attachmentUrl(attachmentId) {
-    return "/api/v3/attachments/" + attachmentId + "/content";
-  }
-}
-
-export {XeokitServer};

frontend/src/app/modules/bim/ifc_models/xeokit/xeokit-server.ts

@@ -0,0 +1,76 @@
+// @ts-ignore
+import {utils} from "@xeokit/xeokit-sdk/src/viewer/scene/utils";
+import {PathHelperService} from "../../../common/path-helper/path-helper.service";
+import {IFCGonDefinition} from "../pages/viewer/ifc-models-data.service";
+
+/**
+ * Default server client which loads content via HTTP from the file system.
+ */
+export class XeokitServer {
+  private ifcModels:IFCGonDefinition;
+  /**
+   *
+   * @param config
+   * @param.config.pathHelper instance of PathHelperService.
+   */
+  constructor(private pathHelper:PathHelperService) {
+    this.ifcModels = window.gon.ifc_models;
+  }
+
+  /**
+   * Gets the manifest of all projects.
+   * @param done
+   * @param error
+   */
+  getProjects(done:Function, _error:Function) {
+    done({ projects: this.ifcModels.projects });
+  }
+
+  /**
+   * Gets a manifest for a project.
+   * @param projectId
+   * @param done
+   * @param error
+   */
+  getProject(projectData:any, done:Function, _error:Function) {
+    var manifestData = {
+      id: projectData[0].id,
+      name: projectData[0].name,
+      models: this.ifcModels.models,
+      viewerContent: {
+        modelsLoaded: this.ifcModels.shown_models
+      },
+      viewerConfigs: {
+        saoEnabled: true // Needs to be enabled by default if we want to use it selectively on the available models.
+      }
+    };
+
+    done(manifestData);
+  }
+
+  /**
+   * Gets metadata for a model within a project.
+   * @param projectId
+   * @param modelId
+   * @param done
+   * @param error
+   */
+  getMetadata(_projectId:string, modelId:number, done:Function, error:Function) {
+    const attachmentId = this.ifcModels.metadata_attachment_ids[modelId];
+    console.log(`Loading model metadata for: ${attachmentId}`);
+    utils.loadJSON(this.pathHelper.attachmentContentPath(attachmentId), done, error);
+  }
+
+  /**
+   * Gets geometry for a model within a project.
+   * @param projectId
+   * @param modelId
+   * @param done
+   * @param error
+   */
+  getGeometry(projectId:string, modelId:number, done:Function, error:Function) {
+    const attachmentId = this.ifcModels.xkt_attachment_ids[modelId];
+    console.log(`Loading model geometry for: ${attachmentId}`);
+    utils.loadArraybuffer(this.pathHelper.attachmentContentPath(attachmentId), done, error);
+  }
+}

frontend/src/app/modules/common/gon/gon.service.ts

@@ -27,6 +27,7 @@
 // ++
 
 import {Injectable} from "@angular/core";
+import {IFCGonDefinition} from "../../bim/ifc_models/pages/viewer/ifc-models-data.service";
 
 declare global {
   interface Window {
@@ -36,6 +37,7 @@ declare global {
 
 export interface GonType {
  [key:string]:unknown;
+ ifc_models:IFCGonDefinition;
 }
 
 @Injectable({ providedIn: 'root' })

frontend/src/app/modules/common/path-helper/path-helper.service.ts

@@ -45,10 +45,10 @@ class Apiv3Paths {
    * @param context
    */
   public previewMarkup(context:string) {
-    let base = this.apiV3Base + '/render/markdown';
+    let base = `${this.apiV3Base}/render/markdown`;
 
     if (context) {
-      return base + `?context=${context}`;
+      return `${base}?context=${context}`;
     } else {
       return base;
     }
@@ -85,7 +85,7 @@ class Apiv3Paths {
 
 @Injectable({ providedIn: 'root' })
 export class PathHelperService {
-  public readonly appBasePath = window.appBasePath ? window.appBasePath : '';
+  public readonly appBasePath = window.appBasePath || '';
   public readonly api = {
     v3: new Apiv3Paths(this.appBasePath)
   };
@@ -95,21 +95,25 @@ export class PathHelperService {
   }
 
   public attachmentDownloadPath(attachmentIdentifier:string, slug:string|undefined) {
-    let path = this.staticBase + '/attachments/' + attachmentIdentifier;
+    let path = `${this.staticBase}/attachments/${attachmentIdentifier}`;
 
     if (slug) {
-      return path + "/" + slug;
+      return `${path}/${slug}`;
     } else {
       return path;
     }
   }
 
+  public attachmentContentPath(attachmentIdentifier:number|string) {
+    return `${this.staticBase}/attachments/${attachmentIdentifier}/content`;
+  }
+
   public ifcModelsPath(projectIdentifier:string) {
-    return this.staticBase + `/projects/${projectIdentifier}/ifc_models`;
+    return `${this.staticBase}/projects/${projectIdentifier}/ifc_models`;
   }
 
   public bimDetailsPath(projectIdentifier:string, workPackageId:string, viewpoint:number|string|null = null) {
-    let path = this.projectPath(projectIdentifier) + `/bcf/split/details/${workPackageId}`;
+    let path = `${this.projectPath(projectIdentifier)}/bcf/split/details/${workPackageId}`;
 
     if (viewpoint !== null) {
       path += `?viewpoint=${viewpoint}`;
@@ -119,95 +123,95 @@ export class PathHelperService {
   }
 
   public highlightingCssPath() {
-    return this.staticBase + '/highlighting/styles';
+    return `${this.staticBase}/highlighting/styles`;
   }
 
   public forumPath(projectIdentifier:string, forumIdentifier:string) {
-    return this.projectForumPath(projectIdentifier) + '/' + forumIdentifier;
+    return `${this.projectForumPath(projectIdentifier)}/${forumIdentifier}`;
   }
 
   public keyboardShortcutsHelpPath() {
-    return this.staticBase + '/help/keyboard_shortcuts';
+    return `${this.staticBase}/help/keyboard_shortcuts`;
   }
 
   public messagePath(messageIdentifier:string) {
-    return this.staticBase + '/topics/' + messageIdentifier;
+    return `${this.staticBase}/topics/${messageIdentifier}`;
   }
 
   public myPagePath() {
-    return this.staticBase + '/my/page';
+    return `${this.staticBase}/my/page`;
   }
 
   public newsPath(newsId:string) {
-    return this.staticBase + '/news/' + newsId;
+    return `${this.staticBase}/news/${newsId}`;
   }
 
   public loginPath() {
-    return this.staticBase + '/login';
+    return `${this.staticBase}/login`;
   }
 
   public projectsPath() {
-    return this.staticBase + '/projects';
+    return `${this.staticBase}/projects`;
   }
 
   public projectPath(projectIdentifier:string) {
-    return this.projectsPath() + '/' + projectIdentifier;
+    return `${this.projectsPath()}/${projectIdentifier}`;
   }
 
   public projectActivityPath(projectIdentifier:string) {
-    return this.projectPath(projectIdentifier) + '/activity';
+    return `${this.projectPath(projectIdentifier)}/activity`;
   }
 
   public projectForumPath(projectIdentifier:string) {
-    return this.projectPath(projectIdentifier) + '/forums';
+    return `${this.projectPath(projectIdentifier)}/forums`;
   }
 
   public projectCalendarPath(projectId:string) {
-    return this.projectPath(projectId) + '/work_packages/calendar';
+    return `${this.projectPath(projectId)}/work_packages/calendar`;
   }
 
   public projectMembershipsPath(projectId:string) {
-    return this.projectPath(projectId) + '/members';
+    return `${this.projectPath(projectId)}/members`;
   }
 
   public projectNewsPath(projectId:string) {
-    return this.projectPath(projectId) + '/news';
+    return `${this.projectPath(projectId)}/news`;
   }
 
   public projectTimeEntriesPath(projectIdentifier:string) {
-    return this.projectPath(projectIdentifier) + '/cost_reports';
+    return `${this.projectPath(projectIdentifier)}/cost_reports`;
   }
 
   public projectWikiPath(projectId:string) {
-    return this.projectPath(projectId) + '/wiki';
+    return `${this.projectPath(projectId)}/wiki`;
   }
 
   public projectWorkPackagePath(projectId:string, wpId:string|number) {
-    return this.projectWorkPackagesPath(projectId) + '/' + wpId;
+    return `${this.projectWorkPackagesPath(projectId)}/${wpId}`;
   }
 
   public projectWorkPackagesPath(projectId:string) {
-    return this.projectPath(projectId) + '/work_packages';
+    return `${this.projectPath(projectId)}/work_packages`;
   }
 
   public projectWorkPackageNewPath(projectId:string) {
-    return this.projectWorkPackagesPath(projectId) + '/new';
+    return `${this.projectWorkPackagesPath(projectId)}/new`;
   }
 
   public projectBoardsPath(projectIdentifier:string|null) {
     if (projectIdentifier) {
-      return this.projectPath(projectIdentifier) + '/boards';
+      return `${this.projectPath(projectIdentifier)}/boards`;
     } else {
-      return this.staticBase + '/boards';
+      return `${this.staticBase}/boards`;
     }
   }
 
   public projectDashboardsPath(projectIdentifier:string) {
-    return this.projectPath(projectIdentifier) + '/dashboards';
+    return `${this.projectPath(projectIdentifier)}/dashboards`;
   }
 
   public timeEntriesPath(workPackageId:string|number) {
-    var suffix = '/time_entries';
+    let suffix = '/time_entries';
 
     if (workPackageId) {
       return this.workPackagePath(workPackageId) + suffix;
@@ -217,51 +221,50 @@ export class PathHelperService {
   }
 
   public usersPath() {
-    return this.staticBase + '/users';
+    return `${this.staticBase}/users`;
   }
 
   public userPath(id:string|number) {
-    return this.usersPath() + '/' + id;
+    return `${this.usersPath()}/${id}`;
   }
 
   public versionsPath() {
-    return this.staticBase + '/versions';
+    return `${this.staticBase}/versions`;
   }
 
   public versionEditPath(id:string|number) {
-    return this.staticBase + '/versions/' + id + '/edit';
+    return `${this.staticBase}/versions/${id}/edit`;
   }
 
   public versionShowPath(id:string|number) {
-    return this.staticBase + '/versions/' + id;
+    return `${this.staticBase}/versions/${id}`;
   }
 
   public workPackagesPath() {
-    return this.staticBase + '/work_packages';
+    return `${this.staticBase}/work_packages`;
   }
 
   public workPackagePath(id:string|number) {
-    return this.staticBase + '/work_packages/' + id;
+    return `${this.staticBase}/work_packages/${id}`;
   }
 
   public workPackageCopyPath(workPackageId:string|number) {
-    return this.workPackagePath(workPackageId) + '/copy';
+    return `${this.workPackagePath(workPackageId)}/copy`;
   }
 
   public workPackageDetailsCopyPath(projectIdentifier:string, workPackageId:string|number) {
-    return this.projectWorkPackagesPath(projectIdentifier) + '/details/' + workPackageId + '/copy';
+    return `${this.projectWorkPackagesPath(projectIdentifier)}/details/${workPackageId}/copy`;
   }
 
   public workPackagesBulkDeletePath() {
-    return this.workPackagesPath() + '/bulk';
+    return `${this.workPackagesPath()}/bulk`;
   }
 
   public projectLevelListPath() {
-    return this.projectsPath() + '/level_list.json';
+    return `${this.projectsPath()}/level_list.json`;
   }
 
   public textFormattingHelp() {
-    return this.staticBase + '/help/text_formatting';
+    return `${this.staticBase}/help/text_formatting`;
   }
-
 }

frontend/src/global_styles/content/_forms.sass

@@ -498,6 +498,7 @@ fieldset.form--fieldset
 %form--field-element-container
   display: block
   flex: 1 1
+  max-width: 100%
 
   &:nth-last-of-type(n+2)
     padding-right: $block-padding

lib/open_project/custom_styles/color_themes.rb

@@ -29,9 +29,11 @@
 
 module OpenProject::CustomStyles
   class ColorThemes
+    OpenProject::CustomStyles::ColorThemes::DEFAULT_THEME_NAME = 'OpenProject'.freeze
+
     THEMES = [
       {
-        theme: 'OpenProject',
+        theme: OpenProject::CustomStyles::ColorThemes::DEFAULT_THEME_NAME,
         colors: {
           'primary-color' => "#1A67A3",
           'primary-color-dark' => "#175A8E",

modules/bim/db/migrate/20201105154216_seed_custom_style_with_bim_theme.rb

@@ -0,0 +1,101 @@
+#-- encoding: UTF-8
+
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) 2012-2020 the OpenProject GmbH
+#
+# 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 docs/COPYRIGHT.rdoc for more details.
+#++
+
+# This migration cleans up messed up themes. Sometimes in the past
+# the BIM theme was not set where it should have been set.
+class SeedCustomStyleWithBimTheme < ActiveRecord::Migration[6.0]
+
+  def up
+    # When
+    #   migrating BIM instances
+    #     that did not have any custom styles OR
+    #       that do not have any design colors set and no custom logo/touch-icon/favicon
+    #       (this basically means that no theme actually got applied)
+    # then
+    #   add a custom style with the BIM theme set. This will write the theme's colors
+    #   as DesignColor entries to the DB which is necessary for the theme to actually
+    #   have an effect.
+    if OpenProject::Configuration.bim? &&
+       (CustomStyle.current.nil? ||
+           (DesignColor.count == 0 &&
+               CustomStyle.current.favicon.nil? &&
+               CustomStyle.current.logo.nil? &&
+               CustomStyle.current.touch_icon.nil?))
+      seed_bim_theme
+    end
+  end
+
+  def down
+    # nop
+  end
+
+  private
+
+  def seed_bim_theme
+    CustomStyle.transaction do
+      set_custom_style
+      set_design_colors
+    end
+  end
+
+  def set_design_colors
+    # There should not be any DesignColors present. However, we want to make sure.
+    DesignColor.delete_all
+
+    theme[:colors].each do |param_variable, param_hexcode|
+      DesignColor.create variable: param_variable, hexcode: param_hexcode
+    end
+  end
+
+  def set_custom_style
+    custom_style = (CustomStyle.current || CustomStyle.create!)
+    custom_style.attributes = { theme: theme[:theme], theme_logo: theme[:logo] }
+    custom_style.save!
+    custom_style
+  end
+
+  def theme
+    {
+      theme: 'OpenProject BIM',
+      colors: {
+        'primary-color' => "#3270DB",
+        'primary-color-dark' => "#163473",
+        'alternative-color' => "#349939",
+        'header-bg-color' => "#05002C",
+        'header-item-bg-hover-color' => "#163473",
+        'content-link-color' => "#275BB5",
+        'main-menu-bg-color' => "#0E2045",
+        'main-menu-bg-selected-background' => "#3270DB",
+        'main-menu-bg-hover-background' => "#163473"
+      },
+      logo: 'bim/logo_openproject_bim_big.png'
+    }
+  end
+end

modules/ldap_groups/app/models/ldap_groups/membership.rb

@@ -5,5 +5,7 @@ module LdapGroups
                class_name: '::LdapGroups::SynchronizedGroup',
                foreign_key: 'group_id',
                counter_cache: :users_count
+
+    validates_uniqueness_of :user_id, scope: :group_id
   end
 end

modules/ldap_groups/app/models/ldap_groups/synchronized_group.rb

@@ -23,31 +23,59 @@ module LdapGroups
     before_destroy :remove_all_members
 
     ##
-    # Add a set of new members to the internal group
+    # Add a set of new members to the synchronized group as well as the internal group.
+    #
+    # @param new_users [Array<User> | Array<Integer>] Users (or User IDs) to add to the group.
     def add_members!(new_users)
+      return if new_users.empty?
+
       self.class.transaction do
-        users << new_users.map { |u| Membership.new group: self, user: u }
-        group.add_members!(new_users)
+        # create synchronized group memberships
+        memberships = new_users.map { |user| { group_id: self.id, user_id: user_id(user) } }
+        # Bulk insert the memberships to improve performance
+        ::LdapGroups::Membership.insert_all memberships
+
+        # add users to users collection of internal group
+        group.add_members! new_users
       end
     end
 
     ##
-    # Remove a set of users from the internal group
+    # Remove a set of users from the synchronized group as well as the internal group.
+    #
+    # @param users_to_remove [Array<User> | Array<Integer>] Users (or User IDs) to remove from the group.
     def remove_members!(users_to_remove)
-      self.class.transaction do
-        user_ids = users_to_remove.pluck(:user_id)
+      return if users_to_remove.empty?
 
-        # We don't have access to the join table
-        # so we need to ensure we delete the users that are still present in the group
-        # since users MAY want to remove users manually
-        group.users.where(id: user_ids).destroy_all
+      self.class.transaction do
+        # 1) Delete synchronized group MEMBERSHIPS from collection.
+        # 2) Remove users from users collection of internal group.
+        if users_to_remove.first.is_a? User
+          users.delete users.where(user: users_to_remove).select(:id)
+          group.users.delete users_to_remove
+        elsif users_to_remove.first.is_a? Integer
+          users.delete users.where(user_id: users_to_remove).select(:id)
+          group.users.delete group.users.where(id: users_to_remove).select(:id)
+        else
+          raise ArgumentError, "Expected collection of Users or User IDs, got collection of #{users_to_remove.map(&:class).map(&:name).uniq.join(", ")}"
+        end
       end
     end
 
     private
 
+    def user_id(user)
+      if user.is_a? Integer
+        user
+      elsif user.is_a? User
+        user.id
+      else
+        raise ArgumentError, "Expected User or User ID (Integer) but got #{user}"
+      end
+    end
+
     def remove_all_members
-      remove_members!(users)
+      remove_members! User.find(users.pluck(:user_id))
     end
   end
 end

modules/ldap_groups/lib/open_project/ldap_groups/synchronization.rb

@@ -100,15 +100,7 @@ module OpenProject::LdapGroups
 
       Rails.logger.info { "[LDAP groups] Adding #{new_member_ids.length} users to #{sync.dn}" }
 
-      # Bulk insert the memberships
-      memberships = new_member_ids.map do |user_id|
-        {
-          group_id: sync.id,
-          user_id: user_id
-        }
-      end
-      ::LdapGroups::Membership.insert_all memberships
-      sync.group.add_members! new_member_ids
+      sync.add_members! new_member_ids
     end
 
     ##
@@ -120,8 +112,8 @@ module OpenProject::LdapGroups
       end
 
       Rails.logger.info "[LDAP groups] Removing users #{memberships.pluck(:user_id)} from #{sync.dn}"
-      sync.remove_members!(memberships)
-      memberships.delete_all
+
+      sync.remove_members! memberships.pluck(:user_id)
     end
   end
 end

modules/ldap_groups/spec/factories/synchronized_group_factory.rb

@@ -5,4 +5,3 @@ FactoryBot.define do
     auth_source factory: :ldap_auth_source
   end
 end
-

modules/ldap_groups/spec/models/synchronized_group_spec.rb

@@ -1,9 +1,9 @@
 require 'spec_helper'
 
 describe LdapGroups::SynchronizedGroup, type: :model do
-  subject { FactoryBot.build :ldap_synchronized_group }
-
   describe 'validations' do
+    subject { FactoryBot.build :ldap_synchronized_group }
+
     context 'correct attributes' do
       it 'saves the record' do
         expect(subject.save).to eq true
@@ -20,4 +20,91 @@ describe LdapGroups::SynchronizedGroup, type: :model do
       end
     end
   end
-end
+
+  describe 'manipulating members' do
+    let(:users) { [user_1, user_2] }
+    let(:user_1) { FactoryBot.create :user }
+    let(:user_2) { FactoryBot.create :user }
+
+    describe '.add_members!' do
+      let(:synchronized_group) { FactoryBot.create :ldap_synchronized_group, group: group }
+      let(:group) { FactoryBot.create :group }
+
+      shared_examples 'it adds users to the synchronized group and the internal one' do
+        let(:members) { raise "define me!" }
+
+        before do
+          expect(synchronized_group.users).to be_empty
+          expect(group.users).to be_empty
+
+          User.system.run_given do
+            synchronized_group.add_members! members
+          end
+        end
+
+        it 'adds the user(s) to the internal group' do
+          expect(group.reload.users).to eq users
+        end
+
+        it 'adds the user(s) to the synchronized group' do
+          expect(synchronized_group.reload.users.map(&:user)).to eq users
+        end
+      end
+
+      context 'called with user records' do
+        it_behaves_like 'it adds users to the synchronized group and the internal one' do
+          let(:members) { users }
+        end
+      end
+
+      context 'called just with user IDs' do
+        it_behaves_like 'it adds users to the synchronized group and the internal one' do
+          let(:members) { users.pluck(:id) }
+        end
+      end
+    end
+
+    describe '.remove_members!' do
+      let(:synchronized_group) do
+        FactoryBot.create(:ldap_synchronized_group, group: group).tap do |sg|
+          group.users.each do |user|
+            sg.users.create user: user
+          end
+        end
+      end
+      let(:group) { FactoryBot.create :group, members: users }
+
+      shared_examples 'it removes the users from the synchronized group and the internal one' do
+        let(:members) { raise "define me!" }
+
+        before do
+          synchronized_group.remove_members! members
+        end
+
+        it 'removes the user(s) from the internal group' do
+          expect(group.reload.users).to be_empty
+        end
+
+        it 'removes the users(s) from the synchronized group' do
+          expect(synchronized_group.users).to be_empty
+        end
+
+        it 'does not, however, delete the actual users!' do
+          expect(User.find(users.map(&:id))).to eq users
+        end
+      end
+
+      context 'called with user records' do
+        it_behaves_like 'it removes the users from the synchronized group and the internal one' do
+          let(:members) { group.users }
+        end
+      end
+
+      context 'called just with user IDs' do
+        it_behaves_like 'it removes the users from the synchronized group and the internal one' do
+          let(:members) { group.users.pluck(:id) }
+        end
+      end
+    end
+  end
+end