Update docs to put prepend as the last linedocs/update-prepend-documentation

This updates the documentation for adding EE specific features to
clarify that `prepend` should be on the last line, and how to deal with
code that depends on class methods redefined in EE.

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/53919

1 files changed, 124 insertions, 29 deletions

diff --git a/doc/development/ee_features.md b/doc/development/ee_features.md
index b6f053ff0e9..9aea03139ee 100644
--- a/doc/development/ee_features.md
+++ b/doc/development/ee_features.md
@@ -119,10 +119,20 @@ This also applies to views.
 
 ### EE features based on CE features
 
-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.
+For features that build on existing CE features, write a module in the `EE`
+namespace and `prepend` it in the CE class, on the last line of the file that
+the class resides in. 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. For
+example, to prepend a module into the `User` class you would use the following
+approach:
+
+```ruby
+class User < ActiveRecord::Base
+  # ... lots of code here ...
+end
+
+User.prepend(EE::User)
+```
 
 Since the module would require an `EE` namespace, the file should also be
 put in an `ee/` sub-directory. For example, we want to extend the user model
@@ -231,7 +241,6 @@ the existing file:
 
 ```ruby
 class ApplicationController < ActionController::Base
-  prepend EE::ApplicationController
   # ...
 
   def after_sign_out_path_for(resource)
@@ -240,6 +249,8 @@ class ApplicationController < ActionController::Base
 
   # ...
 end
+
+ApplicationController.prepend(EE::ApplicationController)
 ```
 
 And create a new file in the `ee/` sub-directory with the altered
@@ -533,8 +544,6 @@ module API
       end
     end
 
-    prepend EE::API::MergeRequests
-
     params :optional_params do
       # CE specific params go here...
 
@@ -542,6 +551,8 @@ module API
     end
   end
 end
+
+API::MergeRequests.prepend(EE::API::MergeRequests)
 ```
 
 And then we could override it in EE module:
@@ -582,10 +593,10 @@ module API
         authorize_read_builds!
       end
     end
-
-    prepend EE::API::JobArtifacts
   end
 end
+
+API::JobArtifacts.prepend(EE::API::JobArtifacts)
 ```
 
 And then we can follow regular object-oriented practices to override it:
@@ -626,8 +637,6 @@ module API
       end
     end
 
-    prepend EE::API::MergeRequests
-
     put ':id/merge_requests/:merge_request_iid/merge' do
       merge_request = find_project_merge_request(params[:merge_request_iid])
 
@@ -639,6 +648,8 @@ module API
     end
   end
 end
+
+API::MergeRequests.prepend(EE::API::MergeRequests)
 ```
 
 Note that `update_merge_request_ee` doesn't do anything in CE, but
@@ -676,27 +687,37 @@ or not we really need to extend it from EE. For now we're not using it much.
 
 Sometimes we need to use different arguments for a particular API route, and we
 can't easily extend it with an EE module because Grape has different context in
-different blocks. In order to overcome this, we could use class methods from the
-API class.
+different blocks. In order to overcome this, we need to move the data to a class
+method that resides in a separate module or class. This allows us to extend that
+module or class before its data is used, without having to place a `prepend` in
+the middle of CE code.
 
 For example, in one place we need to pass an extra argument to
 `at_least_one_of` so that the API could consider an EE-only argument as the
-least argument. This is not quite beautiful but it's working:
+least argument. We would approach this as follows:
 
 ```ruby
+# api/merge_requests/parameters.rb
 module API
   class MergeRequests < Grape::API
-    def self.update_params_at_least_one_of
-      %i[
-        assignee_id
-        description
-      ]
+    module Parameters
+      def self.update_params_at_least_one_of
+        %i[
+          assignee_id
+          description
+        ]
+      end
     end
+  end
+end
 
-    prepend EE::API::MergeRequests
+API::MergeRequests::Parameters.prepend(EE::API::MergeRequests::Parameters)
 
+# api/merge_requests.rb
+module API
+  class MergeRequests < Grape::API
     params do
-      at_least_one_of(*::API::MergeRequests.update_params_at_least_one_of)
+      at_least_one_of(*Parameters.update_params_at_least_one_of)
     end
   end
 end
@@ -708,16 +729,18 @@ And then we could easily extend that argument in the EE class method:
 module EE
   module API
     module MergeRequests
-      extend ActiveSupport::Concern
+      module Parameters
+        extend ActiveSupport::Concern
 
-      class_methods do
-        extend ::Gitlab::Utils::Override
+        class_methods do
+          extend ::Gitlab::Utils::Override
 
-        override :update_params_at_least_one_of
-        def update_params_at_least_one_of
-          super.push(*%i[
-            squash
-          ])
+          override :update_params_at_least_one_of
+          def update_params_at_least_one_of
+            super.push(*%i[
+              squash
+            ])
+          end
         end
       end
     end
@@ -728,6 +751,78 @@ end
 It could be annoying if we need this for a lot of routes, but it might be the
 simplest solution right now.
 
+This approach can also be used when models define validations that depend on
+class methods. For example:
+
+```ruby
+# app/models/identity.rb
+class Identity < ActiveRecord::Base
+  def self.uniqueness_scope
+    [:provider]
+  end
+
+  prepend EE::Identity
+
+  validates :extern_uid,
+    allow_blank: true,
+    uniqueness: { scope: uniqueness_scope, case_sensitive: false }
+end
+
+# ee/app/models/ee/identity.rb
+module EE
+  module Identity
+    extend ActiveSupport::Concern
+
+    class_methods do
+      extend ::Gitlab::Utils::Override
+
+      def uniqueness_scope
+        [*super, :saml_provider_id]
+      end
+    end
+  end
+end
+```
+
+Instead of taking this approach, we would refactor our code into the following:
+
+```ruby
+# ee/app/models/ee/identity/uniqueness_scopes.rb
+module EE
+  module Identity
+    module UniquenessScopes
+      extend ActiveSupport::Concern
+
+      class_methods do
+        extend ::Gitlab::Utils::Override
+
+        def uniqueness_scope
+          [*super, :saml_provider_id]
+        end
+      end
+    end
+  end
+end
+
+# app/models/identity/uniqueness_scopes.rb
+class Identity < ActiveRecord::Base
+  module UniquenessScopes
+    def self.uniqueness_scope
+      [:provider]
+    end
+  end
+end
+
+Identity::UniquenessScopes.prepend(EE::Identity::UniquenessScopes)
+
+# app/models/identity.rb
+class Identity < ActiveRecord::Base
+  validates :extern_uid,
+    allow_blank: true,
+    uniqueness: { scope: Identity::UniquenessScopes.scopes, case_sensitive: false }
+end
+```
+
 ### Code in `spec/`
 
 When you're testing EE-only features, avoid adding examples to the