From 0af2ab18fa7914380150c0403289a9d99ea45ded Mon Sep 17 00:00:00 2001
From: Shinya Maeda <shinya@gitlab.com>
Date: Tue, 8 May 2018 14:57:41 +0900
Subject: [PATCH] Decouple to_params from AtomicInternalId concern

---
 app/models/ci/pipeline.rb                 | 2 +-
 app/models/concerns/atomic_internal_id.rb | 6 +-----
 app/models/concerns/iid_routes.rb         | 9 +++++++++
 app/models/deployment.rb                  | 1 +
 app/models/issue.rb                       | 1 +
 app/models/merge_request.rb               | 1 +
 app/models/milestone.rb                   | 1 +
 7 files changed, 15 insertions(+), 6 deletions(-)
 create mode 100644 app/models/concerns/iid_routes.rb

diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 6bd2c42bbd3..d542868f01f 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -14,7 +14,7 @@ module Ci
     belongs_to :auto_canceled_by, class_name: 'Ci::Pipeline'
     belongs_to :pipeline_schedule, class_name: 'Ci::PipelineSchedule'
 
-    has_internal_id :iid, scope: :project, presence: false, to_param: false, init: -> do |s|
+    has_internal_id :iid, scope: :project, presence: false, init: -> do |s|
       s&.project&.pipelines&.maximum(:iid) || s&.project&.pipelines.count
     end
 
diff --git a/app/models/concerns/atomic_internal_id.rb b/app/models/concerns/atomic_internal_id.rb
index 709589a9128..3d867df544f 100644
--- a/app/models/concerns/atomic_internal_id.rb
+++ b/app/models/concerns/atomic_internal_id.rb
@@ -25,7 +25,7 @@ module AtomicInternalId
   extend ActiveSupport::Concern
 
   module ClassMethods
-    def has_internal_id(column, scope:, init:, presence: true, to_param: true) # rubocop:disable Naming/PredicateName
+    def has_internal_id(column, scope:, init:, presence: true) # rubocop:disable Naming/PredicateName
       before_validation :"ensure_#{column}!", on: :create
       validates column, presence: presence, numericality: true
 
@@ -42,10 +42,6 @@ module AtomicInternalId
 
         read_attribute(column)
       end
-
-      define_method("to_param") do
-        read_attribute(column)
-      end if to_param
     end
   end
 end
diff --git a/app/models/concerns/iid_routes.rb b/app/models/concerns/iid_routes.rb
new file mode 100644
index 00000000000..50957e0230d
--- /dev/null
+++ b/app/models/concerns/iid_routes.rb
@@ -0,0 +1,9 @@
+module IIDRoutes
+  ##
+  # This automagically enforces all related routes to use `iid` instead of `id`
+  # If you want to use `iid` for some routes and `id` for other routes, this module should not to be included,
+  # instead you should define `iid` or `id` explictly at each route generators. e.g. pipeline_path(project.id, pipeline.iid)
+  def to_param
+    iid.to_s
+  end
+end
diff --git a/app/models/deployment.rb b/app/models/deployment.rb
index 254764eefde..dac97676348 100644
--- a/app/models/deployment.rb
+++ b/app/models/deployment.rb
@@ -1,5 +1,6 @@
 class Deployment < ActiveRecord::Base
   include AtomicInternalId
+  include IIDRoutes
 
   belongs_to :project, required: true
   belongs_to :environment, required: true
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 0332bfa9371..6d33a67845f 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -2,6 +2,7 @@ require 'carrierwave/orm/activerecord'
 
 class Issue < ActiveRecord::Base
   include AtomicInternalId
+  include IIDRoutes
   include Issuable
   include Noteable
   include Referable
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 628c61d5d69..a14681897fd 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -1,5 +1,6 @@
 class MergeRequest < ActiveRecord::Base
   include AtomicInternalId
+  include IIDRoutes
   include Issuable
   include Noteable
   include Referable
diff --git a/app/models/milestone.rb b/app/models/milestone.rb
index d14e3a4ded5..f143b8452a2 100644
--- a/app/models/milestone.rb
+++ b/app/models/milestone.rb
@@ -9,6 +9,7 @@ class Milestone < ActiveRecord::Base
 
   include CacheMarkdownField
   include AtomicInternalId
+  include IIDRoutes
   include Sortable
   include Referable
   include StripAttribute