Merge branch 'dblessing/gitlab-ee-ee_module_docs' into 'master'

Improve the "Guidelines for implementing Enterprise Edition feature" document

See merge request !910

app/helpers/ee/issuables_helper.rb

+module EE
+  module IssuablesHelper
+    def weight_dropdown_label(weight)
+      if Issue.weight_options.include?(weight)
+        weight
+      else
+        h(weight.presence || 'Weight')
+      end
+    end
+  end
+end

app/policies/ee/project_policy.rb

+module EE
+  module ProjectPolicy
+    def disabled_features!
+      raise NotImplementedError unless defined?(super)
+
+      super
+
+      if License.block_changes?
+        cannot! :create_issue
+        cannot! :create_merge_request
+        cannot! :push_code
+        cannot! :push_code_to_protected_branches
+      end
+    end
+  end
+end

app/policies/project_policy.rb

 class ProjectPolicy < BasePolicy
+  prepend EE::ProjectPolicy
+
   def rules
     team_access!(user)
 
@@ -267,19 +269,4 @@ class ProjectPolicy < BasePolicy
       :"admin_#{name}"
     ]
   end
-
-  # EE-specific
-  module EeDisabledFeaturePermissions
-    def disabled_features!
-      super
-
-      if License.block_changes?
-        cannot! :create_issue
-        cannot! :create_merge_request
-        cannot! :push_code
-        cannot! :push_code_to_protected_branches
-      end
-    end
-  end
-  prepend EeDisabledFeaturePermissions
 end

config/initializers/inflections.rb

@@ -11,4 +11,5 @@
 #
 ActiveSupport::Inflector.inflections do |inflect|
   inflect.uncountable %w(award_emoji)
+  inflect.acronym 'EE'
 end

db/migrate/20161021185735_migrate_ee_sidekiq_queues_from_default.rb

 # See http://doc.gitlab.com/ce/development/migration_style_guide.html
 # for more information on how to write migrations for GitLab.
 
-class MigrateEeSidekiqQueuesFromDefault < ActiveRecord::Migration
+class MigrateEESidekiqQueuesFromDefault < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 
   DOWNTIME = true

doc/development/ee_features.md

 # Guidelines for implementing Enterprise Edition feature
 
-## Write the code and the tests
+- **Write the code and the tests.**: As with any code, EE features should have
+  good test coverage to prevent regressions.
+- **Write documentation.**: Add documentation to the `doc/` directory. Describe
+  the feature and include screenshots, if applicable.
+- **Submit a MR to the `www-gitlab-com` project.**: Add the new feature to the
+  [EE features list][ee-features-list].
 
-Implement the wanted feature.
-Implemented feature needs to have the full test coverage.
-For now, exception is the code that needs to query a LDAP server.
+## Separation of EE code
 
-## Write the documentation
+Merging changes from GitLab CE to EE can result in numerous conflicts.
+To reduce conflicts, EE code should be separated in to the `EE` module
+as much as possible.
 
-Any feature needs to be well documented. Add the documentation to `/doc` directory, describe the main use of the newly implemented feature and, if applicable, add screenshots.
+### Code in `app/`
 
-## Submit the MR to `about.gitlab.com`
+Place EE-specific controllers, finders, helpers, mailers, models, policies,
+serializers/entities, services, validators and workers in the top-level
+`EE` module namespace, and in a specific `/ee/` sub-folder:
 
-Submit the MR to [about.gitlab.com site repository](https://gitlab.com/gitlab-com/www-gitlab-com) to add the new feature to the [EE feature comparison page](https://gitlab.com/gitlab-com/www-gitlab-com/blob/master/source/gitlab-ee/index.html)
+- `app/controllers/ee/foos_controller.rb`
+- `app/finders/ee/foos_finder.rb`
+- `app/helpers/ee/foos_helper.rb`
+- `app/mailers/ee/foos_mailer.rb`
+- `app/models/ee/foo.rb`
+- `app/policies/ee/foo_policy.rb`
+- `app/serializers/ee/foo_entity.rb`
+- `app/serializers/ee/foo_serializer.rb`
+- `app/services/ee/foo/create_service.rb`
+- `app/validators/ee/foo_attr_validator.rb`
+- `app/workers/ee/foo_worker.rb`
+
+If you modify an existing part of a CE controller, model, service, worker etc.
+one simple solution is to use the `prepend` strategy ([presented below](#overriding-ce-methods)).
+
+For example to override the CE implementation of
+`ApplicationController#after_sign_out_path_for`:
+
+```ruby
+def after_sign_out_path_for(resource)
+  current_application_settings.after_sign_out_path.presence || new_user_session_path
+end
+```
+
+Instead of modifying the method in place, you should do the following:
+
+```ruby
+class ApplicationController < ActionController::Base
+  prepend EE::ApplicationController
+  [...]
+
+  def after_sign_out_path_for(resource)
+    current_application_settings.after_sign_out_path.presence || new_user_session_path
+  end
+
+  [...]
+end
+
+module EE
+  class ApplicationController    
+    def after_sign_out_path_for(resource)
+      raise NotImplementedError unless defined?(super)
+
+      if Gitlab::Geo.secondary?
+        Gitlab::Geo.primary_node.oauth_logout_url(@geo_logout_state)
+      else
+        super
+      end
+    end
+  end
+end
+```
+
+#### Code in `app/controllers/`
+
+In controllers, the most common type of conflict is with `before_action` that
+has a list of actions in CE but EE adds some actions to that list.
+
+The same problem often occurs for `params.require` / `params.permit` calls.
+
+**Mitigations**
+
+Separate CE and EE actions/keywords. For instance for `params.require` in
+`ProjectsController`:
+
+```ruby
+def project_params
+  params.require(:project).permit(project_params_ce)
+  # On EE, this is always:
+  # params.require(:project).permit(project_params_ce << project_params_ee)
+end
+
+# Always returns an array of symbols, created however best fits the use case.
+# It _should_ be sorted alphabetically.
+def project_params_ce
+  %i[
+    description
+    name
+    path
+  ]
+end
+
+# (On EE)
+def project_params_ee
+  %i[
+    approvals_before_merge
+    approver_group_ids
+    approver_ids
+    ...
+  ]
+end
+```
+
+#### Code in `app/models/`
+
+EE-specific models should `extend EE::Model`.
+
+For example, if EE has a specific `Tanuki` model, you would
+place it in `app/models/ee/tanuki.rb`.
+
+#### Code in `app/views/`
+
+It's a very frequent problem that EE is adding some specific view code in a CE
+view. For instance the approval code in the project's settings page.
+
+**Mitigations**
+
+Blocks of code that are EE-specific should be moved to partials as much as
+possible to avoid conflicts with big chunks of HAML code that that are not
+fun to resolve when you add the indentation to the equation.
+
+### Code in `lib/`
+
+Place EE-specific logic in the top-level `EE` module namespace. Namespace the
+class beneath the `EE` module just as you would normally.
+
+For example, if CE has LDAP classes in `lib/gitlab/ldap/` then you would place
+EE-specific LDAP classes in `lib/ee/gitlab/ldap`.
+
+### Classes vs. Module Mixins
+
+If the feature being developed is not present in any form in CE, separation is
+easier - build the class entirely in the `EE` namespace. For features that build
+on existing CE features, write a module in the `EE` namespace and include it
+in the CE class. This makes conflicts less likely during CE to EE merges
+because only one line is added to the CE class - the `include` statement.
+
+#### Overriding CE methods
+
+There are two ways for overriding a method that's defined in CE:
+
+- changing the method's body in place
+- override the method's body by using `prepend` which lets you override a
+  method in a class with a method from a module, and still access the class's
+  implementation with `super`.
+
+The `prepend` method should always be preferred but there are a few gotchas with it:
+
+- you should always add a `raise NotImplementedError unless defined?(super)`
+  guard clause in the "overrider" method to ensure that if the method gets renamed in CE, the EE override won't be silently forgotten.
+- when the override would add a line in the middle of the CE implementation,
+  it usually means that you'd better refactor the method to split it in
+  smaller methods that can be more easily and automatically overriden.
+- sometimes for one-liner methods that don't change often it can be more
+  pragmatic to just change the method in place since conflicts resolution
+  should be trivial in this case. Use your best judgement!
+
+[ee-features-list]: https://gitlab.com/gitlab-com/www-gitlab-com/blob/master/data/features.yml

lib/ci/model.rb

@@ -5,7 +5,7 @@ module Ci
     end
 
     def model_name
-      @model_name ||= ActiveModel::Name.new(self, nil, self.name.split("::").last)
+      @model_name ||= ActiveModel::Name.new(self, nil, self.name.demodulize)
     end
   end
 end

lib/ee/legacy_model.rb

+module EE
+  module LegacyModel
+    def table_name_prefix
+      ''
+    end
+
+    def model_name
+      @model_name ||= ActiveModel::Name.new(self, nil, self.name.demodulize)
+    end
+  end
+end

lib/ee/model.rb

+module EE
+  module Model
+    def table_name_prefix
+      'ee_'
+    end
+
+    def model_name
+      @model_name ||= ActiveModel::Name.new(self, nil, self.name.demodulize)
+    end
+  end
+end