Change UserCallout feautre_name to enum

8 files changed, 42 insertions, 22 deletions

diff --git a/app/controllers/user_callouts_controller.rb b/app/controllers/user_callouts_controller.rb
index 69dc444f13a..aa048a9a1fc 100644
--- a/app/controllers/user_callouts_controller.rb
+++ b/app/controllers/user_callouts_controller.rb
@@ -1,6 +1,6 @@
 class UserCalloutsController < ApplicationController
   def create
-    if ensure_callout
+    if check_feature_name && ensure_callout
       respond_to do |format|
         format.json { head :ok }
       end
@@ -13,6 +13,10 @@ class UserCalloutsController < ApplicationController
 
   private
 
+  def check_feature_name
+    UserCallout.feature_names.keys.include?(callout_param)
+  end
+
   def ensure_callout
     current_user.callouts.find_or_create_by(feature_name: callout_param)
   end
diff --git a/app/helpers/user_callouts_helper.rb b/app/helpers/user_callouts_helper.rb
index 3725545d4cc..a292f676331 100644
--- a/app/helpers/user_callouts_helper.rb
+++ b/app/helpers/user_callouts_helper.rb
@@ -1,11 +1,6 @@
 module UserCalloutsHelper
   GKE_CLUSTER_INTEGRATION = 'gke_cluster_integration'.freeze
 
-  # Higher value = higher priority
-  PRIORITY = {
-    GKE_CLUSTER_INTEGRATION: 0
-  }.freeze
-
   def show_gke_cluster_integration_callout?(project)
     current_user && !user_dismissed?(GKE_CLUSTER_INTEGRATION) &&
       can?(current_user, :create_cluster, project)
diff --git a/app/models/user_callout.rb b/app/models/user_callout.rb
index 8b31aa6fa5c..a7cfe5df7c0 100644
--- a/app/models/user_callout.rb
+++ b/app/models/user_callout.rb
@@ -1,6 +1,13 @@
 class UserCallout < ActiveRecord::Base
   belongs_to :user
 
+  enum feature_name: {
+    gke_cluster_integration: 0
+  }
+
   validates :user, presence: true
-  validates :feature_name, presence: true, uniqueness: { scope: :user_id }
+  validates :feature_name,
+    presence: true,
+    uniqueness: { scope: :user_id },
+    inclusion: { in: UserCallout.feature_names.keys }
 end
diff --git a/db/migrate/20180125214301_create_user_callouts.rb b/db/migrate/20180125214301_create_user_callouts.rb
index 945f4181274..856eff36ae0 100644
--- a/db/migrate/20180125214301_create_user_callouts.rb
+++ b/db/migrate/20180125214301_create_user_callouts.rb
@@ -7,7 +7,7 @@ class CreateUserCallouts < ActiveRecord::Migration
 
   def change
     create_table :user_callouts do |t|
-      t.string :feature_name, null: false
+      t.integer :feature_name, null: false
       t.references :user, index: true, foreign_key: { on_delete: :cascade }, null: false
     end
 
diff --git a/db/schema.rb b/db/schema.rb
index 6e50b7681d0..14024164359 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -1770,7 +1770,7 @@ ActiveRecord::Schema.define(version: 20180202111106) do
   add_index "user_agent_details", ["subject_id", "subject_type"], name: "index_user_agent_details_on_subject_id_and_subject_type", using: :btree
 
   create_table "user_callouts", force: :cascade do |t|
-    t.string "feature_name", null: false
+    t.integer "feature_name", null: false
     t.integer "user_id", null: false
   end
 
diff --git a/spec/controllers/user_callouts_controller_spec.rb b/spec/controllers/user_callouts_controller_spec.rb
index ac295aa72bb..48e2ff75cac 100644
--- a/spec/controllers/user_callouts_controller_spec.rb
+++ b/spec/controllers/user_callouts_controller_spec.rb
@@ -8,27 +8,41 @@ describe UserCalloutsController do
   end
 
   describe "POST #create" do
-    subject { post :create, feature_name: 'feature_name', format: :json }
+    subject { post :create, feature_name: feature_name, format: :json }
 
-    context 'when callout entry does not exist' do
-      it 'should create a callout entry with dismissed state' do
-        expect { subject }.to change { UserCallout.count }.by(1)
+    context 'with valid feature name' do
+      let(:feature_name) { UserCallout.feature_names.keys.first }
+
+      context 'when callout entry does not exist' do
+        it 'should create a callout entry with dismissed state' do
+          expect { subject }.to change { UserCallout.count }.by(1)
+        end
+
+        it 'should return success' do
+          subject
+
+          expect(response).to have_gitlab_http_status(:ok)
+        end
       end
 
-      it 'should return success' do
-        subject
+      context 'when callout entry already exists' do
+        let!(:callout) { create(:user_callout, feature_name: UserCallout.feature_names.keys.first, user: user) }
+
+        it 'should return success' do
+          subject
 
-        expect(response).to have_gitlab_http_status(:ok)
+          expect(response).to have_gitlab_http_status(:ok)
+        end
       end
     end
 
-    context 'when callout entry already exists' do
-      let!(:callout) { create(:user_callout, feature_name: 'feature_name', user: user) }
+    context 'with invalid feature name' do
+      let(:feature_name) { 'bogus_feature_name' }
 
-      it 'should return success' do
+      it 'should return bad request' do
         subject
 
-        expect(response).to have_gitlab_http_status(:ok)
+        expect(response).to have_gitlab_http_status(:bad_request)
       end
     end
   end
diff --git a/spec/factories/user_callouts.rb b/spec/factories/user_callouts.rb
index ae83d65da2e..528e442c14b 100644
--- a/spec/factories/user_callouts.rb
+++ b/spec/factories/user_callouts.rb
@@ -1,6 +1,6 @@
 FactoryBot.define do
   factory :user_callout do
-    feature_name 'test_callout'
+    feature_name :gke_cluster_integration
 
     user
   end
diff --git a/spec/models/user_callout_spec.rb b/spec/models/user_callout_spec.rb
index b39d12e48ac..64ba17c81fe 100644
--- a/spec/models/user_callout_spec.rb
+++ b/spec/models/user_callout_spec.rb
@@ -11,6 +11,6 @@ describe UserCallout do
     it { is_expected.to validate_presence_of(:user) }
 
     it { is_expected.to validate_presence_of(:feature_name) }
-    it { is_expected.to validate_uniqueness_of(:feature_name).scoped_to(:user_id) }
+    it { is_expected.to validate_uniqueness_of(:feature_name).scoped_to(:user_id).ignoring_case_sensitivity }
   end
 end