Merge branch 'tc-add-ee-feature-docs' into 'master'

Extend documentation on writing EE features

See merge request !2248

doc/development/ee_features.md

@@ -7,15 +7,39 @@
 - **Submit a MR to the `www-gitlab-com` project.**: Add the new feature to the
   [EE features list][ee-features-list].
 
+## Act as CE when unlicensed
+
+Since the implementation of [GitLab CE features to work with unlicensed EE instance][ee-as-ce]
+GitLab Enterprise Edition should work like GitLab Community Edition
+when no license is active. So EE features always should be guarded by
+`project.feature_available?` or `group.feature_available?` (or
+`License.feature_available?` if it is a system-wide feature).
+
+CE specs should remain untouched as much as possible and extra specs
+should be added for EE. Licensed features can be stubbed using the
+spec helper `stub_licensed_features` in `EE::LicenseHelpers`.
+
+[ee-as-ce]: https://gitlab.com/gitlab-org/gitlab-ee/issues/2500
+
 ## Separation of EE code
 
 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.
 
-### Code in `app/`
+### Classes vs. Module Mixins
+
+If the feature being developed is not present in any form in CE,
+separation is easy - build the class entirely in the `EE` namespace.
+
+For features that build on existing CE features, write a module in the
+`EE` namespace and `prepend` it in the CE class. This makes conflicts
+less likely to happen during CE to EE merges because only one line is
+added to the CE class - the `prepend` line.
+
+### Adding EE-only files
 
-Place EE-specific controllers, finders, helpers, mailers, models, policies,
+Place EE-only controllers, finders, helpers, mailers, models, policies,
 serializers/entities, services, validators and workers in the top-level
 `EE` module namespace, and in the `ee/` specific sub-directory:
 
@@ -31,8 +55,26 @@ serializers/entities, services, validators and workers in the top-level
 - `ee/app/validators/ee/foo_attr_validator.rb`
 - `ee/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)).
+This applies to both EE-only classes and EE module mixins.
+
+#### Overriding CE methods
+
+To override a method present in the CE codebase, use `prepend`. It
+lets you override a method in a class with a method from a module, while
+still having access the class's implementation with `super`.
+
+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 "overrider" would add a line in the middle of the CE
+  implementation, you should refactor the CE method and split it in
+  smaller methods. Or create a "hook" method that is empty in CE,
+  and with the EE-specific implementation in EE.
+
+When prepending, place them in the `ee/` specific sub-directory, and
+wrap class or module in `module EE` to avoid naming conflicts.
 
 For example to override the CE implementation of
 `ApplicationController#after_sign_out_path_for`:
@@ -43,20 +85,26 @@ def after_sign_out_path_for(resource)
 end
 ```
 
-Instead of modifying the method in place, you should do the following:
+Instead of modifying the method in place, you should add `prepend` to
+the existing file:
 
 ```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
+```
+
+And create a new file in the `ee/` sub-directory with the altered
+implementation:
 
+```ruby
 module EE
   class ApplicationController
     def after_sign_out_path_for(resource)
@@ -72,7 +120,50 @@ module EE
 end
 ```
 
-#### Code in `app/controllers/`
+#### Use self-descriptive wrapper methods
+
+When it's not possible/logical to modify the implementation of a
+method. Wrap it in a self-descriptive method and use that method.
+
+For example, in CE only an `admin` is allowed to access all private
+projects/groups, but in EE also an `auditor` has full private
+access. It would be incorrect to override the implementation of
+`User#admin?`, so instead add a method `full_private_access?` to
+`app/models/users.rb`. The implementation in CE will be:
+
+```ruby
+def full_private_access?
+  admin?
+end
+```
+
+In EE, the implementation `ee/app/models/ee/users.rb` would be:
+
+```ruby
+def full_private_access?
+  raise NotImplementedError unless defined?(super)
+  super || auditor?
+end
+```
+
+In `lib/gitlab/visibility_level.rb` this method is used to return the
+allowed visibilty levels:
+
+```ruby
+def levels_for_user(user = nil)
+  if user.full_private_access?
+    [PRIVATE, INTERNAL, PUBLIC]
+  elsif # ...
+end
+```
+
+See [CE MR][ce-mr-full-private] and [EE MR][ee-mr-full-private] for
+full implementation details.
+
+[ce-mr-full-private]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12373
+[ee-mr-full-private]: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/2199
+
+### 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.
@@ -86,14 +177,12 @@ Separate CE and EE actions/keywords. For instance for `params.require` in
 
 ```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)
+  params.require(:project).permit(project_params_attributes)
 end
 
 # Always returns an array of symbols, created however best fits the use case.
 # It _should_ be sorted alphabetically.
-def project_params_ce
+def project_params_attributes
   %i[
     description
     name
@@ -101,8 +190,16 @@ def project_params_ce
   ]
 end
 
-# (On EE)
-def project_params_ee
+```
+
+In the `EE::ProjectsController` module:
+
+```ruby
+def project_params_attributes
+  super + project_params_attributes_ee
+end
+
+def project_params_attributes_ee
   %i[
     approvals_before_merge
     approver_group_ids
@@ -112,23 +209,26 @@ def project_params_ee
 end
 ```
 
-#### Code in `app/models/`
+### 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 `ee/app/models/ee/tanuki.rb`.
 
-#### Code in `app/views/`
+### 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.
+Blocks of code that are EE-specific should be moved to partials. This
+avoids conflicts with big chunks of HAML code that that are not fun to
+resolve when you add the indentation to the equation.
+
+EE-specific views should be placed in `ee/app/views/ee/`, using extra
+sub-directories if appropriate.
 
 ### Code in `lib/`
 
@@ -138,38 +238,77 @@ 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 `ee/lib/ee/gitlab/ldap`.
 
-### Classes vs. Module Mixins
+### Code in `spec/`
 
-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.
+When you're testing EE-only features, avoid adding examples to the
+existing CE specs. Also do no change existing CE examples, since they
+should remain working as-is when EE is running without a license.
 
-#### Overriding CE methods
+Instead place EE specs in the `spec/ee/spec` folder.
 
-There are two ways for overriding a method that's defined in CE:
+## JavaScript code in `assets/javascripts/`
 
-- 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`.
+To separate EE-specific JS-files we can also move the files into an `ee` folder.
 
-The `prepend` method should always be preferred but there are a few gotchas with it:
+For example there can be an
+`app/assets/javascripts/protected_branches/protected_branches_bundle.js` and an
+EE counterpart
+`ee/app/assets/javascripts/protected_branches/protected_branches_bundle.js`.
 
-- 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 "overrider" 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.
-- when the original implementation contains a guard clause (e.g.
-  `return unless condition`), it doesn't return from the overriden method (it's
-  actually the same behavior as with method overridding via inheritance). In
-  this case, it's usually better to create a "hook" method that is empty in CE,
-  and with the EE-specific implementation in EE
-- 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
+That way we can create a separate webpack bundle in `webpack.config.js`:
+
+```javascript
+    protected_branches:    '~/protected_branches',
+    ee_protected_branches: 'ee/protected_branches/protected_branches_bundle.js',
+```
+
+With the separate bundle in place, we can decide which bundle to load inside the
+view, using the `page_specific_javascript_bundle_tag` helper.
+
+```haml
+- content_for :page_specific_javascripts do
+  = page_specific_javascript_bundle_tag('protected_branches')
+```
+
+## SCSS code in `assets/stylesheets`
+
+To separate EE-specific styles in SCSS files, if a component you're adding styles for
+is limited to only EE, it is better to have a separate SCSS file in appropriate directory
+within `app/assets/stylesheets`.
+
+In some cases, this is not entirely possible or creating dedicated SCSS file is an overkill,
+e.g. a text style of some component is different for EE. In such cases,
+styles are usually kept in stylesheet that is common for both CE and EE, and it is wise
+to isolate such ruleset from rest of CE rules (along with adding comment describing the same)
+to avoid conflicts during CE to EE merge.
+
+#### Bad
+```scss
+.section-body {
+  .section-title {
+    background: $gl-header-color;
+  }
+
+  &.ee-section-body {
+    .section-title {
+      background: $gl-header-color-cyan;
+    }
+  }
+}
+```
+
+#### Good
+```scss
+.section-body {
+  .section-title {
+    background: $gl-header-color;
+  }
+}
+
+/* EE-specific styles */
+.section-body.ee-section-body {
+  .section-title {
+    background: $gl-header-color-cyan;
+  }
+}
+```

doc/development/limit_ee_conflicts.md

@@ -2,6 +2,10 @@
 
 This guide contains best-practices for avoiding conflicts between CE and EE.
 
+## Implementing EE feature
+
+Follow the [guidelines](ee_features.md) on how to implement a EE feature.
+
 ## Daily CE Upstream merge
 
 GitLab Community Edition is merged daily into the Enterprise Edition (look for
@@ -78,14 +82,12 @@ Separate CE and EE actions/keywords. For instance for `params.require` in
 
 ```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)
+  params.require(:project).permit(project_params_attributes)
 end
 
 # Always returns an array of symbols, created however best fits the use case.
 # It _should_ be sorted alphabetically.
-def project_params_ce
+def project_params_attributes
   %i[
     description
     name
@@ -93,8 +95,16 @@ def project_params_ce
   ]
 end
 
-# (On EE)
-def project_params_ee
+```
+
+In the `EE::ProjectsController` module:
+
+```ruby
+def project_params_attributes
+  super + project_params_attributes_ee
+end
+
+def project_params_attributes_ee
   %i[
     approvals_before_merge
     approver_group_ids