From d74a8900e27a98f1c921052d2fc44f38cc63a2ea Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Thu, 9 Jun 2016 17:03:33 +0200 Subject: [PATCH 1/2] Added simple feature flag system This can be used to enable/disable certain features using the ApplicationSetting model. Features can be toggled from the application settings UI, or via a Ruby script of sorts. Since this re-uses the ApplicationSetting model the cached settings are flushed automatically. --- CHANGELOG | 1 + .../admin/application_settings_controller.rb | 1 + .../application_settings/_form.html.haml | 17 +++++ ...0160609133359_add_initial_feature_flags.rb | 20 ++++++ db/schema.rb | 7 +- lib/gitlab/feature.rb | 70 +++++++++++++++++++ spec/lib/gitlab/feature_spec.rb | 47 +++++++++++++ 7 files changed, 162 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20160609133359_add_initial_feature_flags.rb create mode 100644 lib/gitlab/feature.rb create mode 100644 spec/lib/gitlab/feature_spec.rb diff --git a/CHANGELOG b/CHANGELOG index b00c149a753c..cda487aa4100 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -61,6 +61,7 @@ v 8.9.0 (unreleased) - Markdown editor now correctly resets the input value on edit cancellation !4175 - Toggling a task list item in a issue/mr description does not creates a Todo for mentions - Improved UX of date pickers on issue & milestone forms + - Added a simple feature flag system to allow enabling/disabling of certain features v 8.8.5 (unreleased) - Ensure branch cleanup regardless of whether the GitHub import process succeeds diff --git a/app/controllers/admin/application_settings_controller.rb b/app/controllers/admin/application_settings_controller.rb index f4eda864aac9..85c6db7ecccd 100644 --- a/app/controllers/admin/application_settings_controller.rb +++ b/app/controllers/admin/application_settings_controller.rb @@ -109,6 +109,7 @@ def application_setting_params :metrics_packet_size, :send_user_confirmation_email, :container_registry_token_expire_delay, + *Gitlab::Feature.column_names, restricted_visibility_levels: [], import_sources: [], disabled_oauth_sign_in_sources: [] diff --git a/app/views/admin/application_settings/_form.html.haml b/app/views/admin/application_settings/_form.html.haml index c883e8f97da5..527e13824539 100644 --- a/app/views/admin/application_settings/_form.html.haml +++ b/app/views/admin/application_settings/_form.html.haml @@ -328,6 +328,23 @@ .help-block If you got a lot of false alarms from repository checks you can choose to clear all repository check information from the database. + %fieldset + %legend Enabled Features + %p + Enabling or disabling these features can be useful to those wanting more + fine grained control over the behaviour of their GitLab instance. + Disabling certain features can also be useful when doing a deploy. For + example, should a deploy modify the data used for displaying/creating + comments it can be useful to disable the feature + creating_notes to prevent users from creating new notes while + the deploy is running. + .form-group + .col-sm-offset-2.col-sm-10 + - Gitlab::Feature.features_with_columns.each do |feature, column| + .checkbox + = f.label(column) do + = f.check_box(column) + #{feature} .form-actions = f.submit 'Save', class: 'btn btn-save' diff --git a/db/migrate/20160609133359_add_initial_feature_flags.rb b/db/migrate/20160609133359_add_initial_feature_flags.rb new file mode 100644 index 000000000000..793b8cebf555 --- /dev/null +++ b/db/migrate/20160609133359_add_initial_feature_flags.rb @@ -0,0 +1,20 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddInitialFeatureFlags < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + def change + columns = [ + :enable_toggling_award_emoji, + :enable_creating_notes, + :enable_updating_notes, + :enable_removing_notes, + :enable_removing_note_attachments + ] + + columns.each do |column| + add_column :application_settings, column, :boolean, default: true + end + end +end diff --git a/db/schema.rb b/db/schema.rb index b7adf48fdb44..1815b5961e9d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20160608155312) do +ActiveRecord::Schema.define(version: 20160609133359) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -85,6 +85,11 @@ t.boolean "send_user_confirmation_email", default: false t.integer "container_registry_token_expire_delay", default: 5 t.text "after_sign_up_text" + t.boolean "enable_toggling_award_emoji", default: true + t.boolean "enable_creating_notes", default: true + t.boolean "enable_updating_notes", default: true + t.boolean "enable_removing_notes", default: true + t.boolean "enable_removing_note_attachments", default: true end create_table "audit_events", force: :cascade do |t| diff --git a/lib/gitlab/feature.rb b/lib/gitlab/feature.rb new file mode 100644 index 000000000000..e2918c539524 --- /dev/null +++ b/lib/gitlab/feature.rb @@ -0,0 +1,70 @@ +module Gitlab + # Module for checking if certain features are enabled or not. + # + # To check for a feature you can use an "enabled?" method generated for each + # registered feature: + # + # if Gitlab::Feature.updating_notes_enabled? + # ... + # end + # + # ## Adding Features + # + # Adding features works in two simple steps: + # + # 1. Add the feature name to the FEATURES array below + # 2. Add a migration that adds a column called "enable_FEATURE" (where + # "FEATURE" is the name of your feature) to "application_settings". The + # type should be a boolean with a default value of true. + module Feature + extend CurrentSettings + + FEATURES = [ + :toggling_award_emoji, + :creating_notes, + :updating_notes, + :removing_notes, + :removing_note_attachments + ].freeze + + class << self + FEATURES.each do |feature| + define_method("#{feature}_enabled?") do + feature_enabled?(feature) + end + end + + # Returns true if the given feature is enabled. + def feature_enabled?(feature) + settings = current_application_settings + method = column_name(feature) + + # If a feature column doesn't exist we still want to enable the + # feature. This for example allows the use of a fake application + # settings object without having to duplicate any feature related + # logic there. + return true unless settings.respond_to?(method) + + settings.__send__(method) + end + + # Returns a Hash containing all features and the corresponding column + # names. + def features_with_columns + FEATURES.each_with_object({}) do |feature, hash| + hash[feature] = column_name(feature) + end + end + + # Returns the names of the columns. + def column_names + FEATURES.map { |f| column_name(f) } + end + + # Returns the column name for a feature. + def column_name(feature) + :"enable_#{feature}" + end + end + end +end diff --git a/spec/lib/gitlab/feature_spec.rb b/spec/lib/gitlab/feature_spec.rb new file mode 100644 index 000000000000..b81dd7bf9dc8 --- /dev/null +++ b/spec/lib/gitlab/feature_spec.rb @@ -0,0 +1,47 @@ +require 'spec_helper' + +describe Gitlab::Feature do + Gitlab::Feature::FEATURES.each do |feature| + describe ".#{feature}_enabled?" do + it 'returns a boolean' do + expect(described_class.__send__("#{feature}_enabled?")). + to be_in([true, false]) + end + end + end + + describe '.feature_enabled?' do + it 'returns true when the column does not exist' do + settings = double(:settings) + + expect(described_class).to receive(:current_application_settings). + and_return(settings) + + expect(described_class.feature_enabled?(:foo)).to eq(true) + end + end + + describe '.features_with_columns' do + it 'returns a Hash mapping feature names to their columns' do + map = described_class.features_with_columns + + expect(map).to be_an_instance_of(Hash) + expect(map[:creating_notes]).to eq(:enable_creating_notes) + end + end + + describe '.column_names' do + it 'returns an Array of column names' do + names = described_class.column_names + + expect(names).to be_an_instance_of(Array) + expect(names).to include(:enable_creating_notes) + end + end + + describe '.column_name' do + it 'returns the column name of a feature flag' do + expect(described_class.column_name(:foo)).to eq(:enable_foo) + end + end +end -- GitLab From beed70c0248fea13bdf727e5f14dee362b0cd298 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Thu, 9 Jun 2016 17:17:01 +0200 Subject: [PATCH 2/2] Use feature flags for notes and award emojis This allows us to disable creating/updating/removing of notes as well as toggling of award emojis. --- app/controllers/application_controller.rb | 8 +++ .../concerns/toggle_award_emoji.rb | 10 ++-- app/controllers/projects/notes_controller.rb | 44 ++++++++------ app/views/award_emoji/_awards_block.html.haml | 4 +- app/views/projects/diffs/_line.html.haml | 2 +- app/views/projects/notes/_note.html.haml | 16 ++--- .../projects/notes/_notes_with_form.html.haml | 27 +++++---- lib/api/helpers.rb | 6 ++ lib/api/notes.rb | 3 + .../projects/notes_controller_spec.rb | 58 ++++++++++++++++++- spec/requests/api/notes_spec.rb | 38 ++++++++++++ 11 files changed, 172 insertions(+), 44 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 62f63701799a..713d552976c2 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -366,6 +366,14 @@ def u2f_app_id request.base_url end + def require_feature!(feature) + if Gitlab::Feature.feature_enabled?(feature) + yield + else + head(:service_unavailable) + end + end + private def set_default_sort diff --git a/app/controllers/concerns/toggle_award_emoji.rb b/app/controllers/concerns/toggle_award_emoji.rb index 036777c80c19..13c3de4ddbbe 100644 --- a/app/controllers/concerns/toggle_award_emoji.rb +++ b/app/controllers/concerns/toggle_award_emoji.rb @@ -6,12 +6,14 @@ module ToggleAwardEmoji end def toggle_award_emoji - name = params.require(:name) + require_feature!(:toggling_award_emoji) do + name = params.require(:name) - awardable.toggle_award_emoji(name, current_user) - TodoService.new.new_award_emoji(to_todoable(awardable), current_user) + awardable.toggle_award_emoji(name, current_user) + TodoService.new.new_award_emoji(to_todoable(awardable), current_user) - render json: { ok: true } + render json: { ok: true } + end end private diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb index 836f79ff0803..db046aaa7a41 100644 --- a/app/controllers/projects/notes_controller.rb +++ b/app/controllers/projects/notes_controller.rb @@ -22,39 +22,47 @@ def index end def create - @note = Notes::CreateService.new(project, current_user, note_params).execute + require_feature!(:creating_notes) do + @note = Notes::CreateService.new(project, current_user, note_params).execute - respond_to do |format| - format.json { render json: note_json(@note) } - format.html { redirect_back_or_default } + respond_to do |format| + format.json { render json: note_json(@note) } + format.html { redirect_back_or_default } + end end end def update - @note = Notes::UpdateService.new(project, current_user, note_params).execute(note) + require_feature!(:updating_notes) do + @note = Notes::UpdateService.new(project, current_user, note_params).execute(note) - respond_to do |format| - format.json { render json: note_json(@note) } - format.html { redirect_back_or_default } + respond_to do |format| + format.json { render json: note_json(@note) } + format.html { redirect_back_or_default } + end end end def destroy - if note.editable? - Notes::DeleteService.new(project, current_user).execute(note) - end - - respond_to do |format| - format.js { head :ok } + require_feature!(:removing_notes) do + if note.editable? + Notes::DeleteService.new(project, current_user).execute(note) + end + + respond_to do |format| + format.js { head :ok } + end end end def delete_attachment - note.remove_attachment! - note.update_attribute(:attachment, nil) + require_feature!(:removing_note_attachments) do + note.remove_attachment! + note.update_attribute(:attachment, nil) - respond_to do |format| - format.js { head :ok } + respond_to do |format| + format.js { head :ok } + end end end diff --git a/app/views/award_emoji/_awards_block.html.haml b/app/views/award_emoji/_awards_block.html.haml index 02efcecc8895..44263e97231d 100644 --- a/app/views/award_emoji/_awards_block.html.haml +++ b/app/views/award_emoji/_awards_block.html.haml @@ -1,12 +1,12 @@ - grouped_emojis = awardable.grouped_awards(with_thumbs: inline) .awards.js-awards-block{ class: ("hidden" if !inline && grouped_emojis.empty?), data: { award_url: url_for([:toggle_award_emoji, @project.namespace.becomes(Namespace), @project, awardable]) } } - awards_sort(grouped_emojis).each do |emoji, awards| - %button.btn.award-control.js-emoji-btn.has-tooltip{ type: "button", class: (award_active_class(awards, current_user)), data: { placement: "bottom", title: award_user_list(awards, current_user) } } + %button.btn.award-control.js-emoji-btn.has-tooltip{ disabled: !Gitlab::Feature.toggling_award_emoji_enabled?, type: "button", class: (award_active_class(awards, current_user)), data: { placement: "bottom", title: award_user_list(awards, current_user) } } = emoji_icon(emoji, sprite: false) %span.award-control-text.js-counter = awards.count - - if current_user + - if current_user && Gitlab::Feature.toggling_award_emoji_enabled? .award-menu-holder.js-award-holder %button.btn.award-control.js-add-award{ type: "button" } = icon('smile-o', class: "award-control-icon award-control-icon-normal") diff --git a/app/views/projects/diffs/_line.html.haml b/app/views/projects/diffs/_line.html.haml index f1577e8a47b4..9e46625378ab 100644 --- a/app/views/projects/diffs/_line.html.haml +++ b/app/views/projects/diffs/_line.html.haml @@ -15,7 +15,7 @@ = link_text - else = link_to "", "##{line_code}", id: line_code, data: { linenumber: link_text } - - if !@diff_notes_disabled && can?(current_user, :create_note, @project) + - if !@diff_notes_disabled && can?(current_user, :create_note, @project) && Gitlab::Feature.creating_notes_enabled? = link_to_new_diff_note(line_code) %td.new_line.diff-line-num{ class: type, data: { linenumber: line.new_pos } } - link_text = type == "old" ? " ".html_safe : line.new_pos diff --git a/app/views/projects/notes/_note.html.haml b/app/views/projects/notes/_note.html.haml index bcdbff080116..a2b614486ad5 100644 --- a/app/views/projects/notes/_note.html.haml +++ b/app/views/projects/notes/_note.html.haml @@ -21,20 +21,22 @@ - if access %span.note-role.hidden-xs= access - if current_user - = link_to '#', title: 'Award Emoji', class: 'note-action-button note-emoji-button js-add-award js-note-emoji', data: { position: 'right' } do - = icon('spinner spin') - = icon('smile-o') - - if note_editable + - if Gitlab::Feature.toggling_award_emoji_enabled? + = link_to '#', title: 'Award Emoji', class: 'note-action-button note-emoji-button js-add-award js-note-emoji', data: { position: 'right' } do + = icon('spinner spin') + = icon('smile-o') + - if note_editable && Gitlab::Feature.updating_notes_enabled? = link_to '#', title: 'Edit comment', class: 'note-action-button js-note-edit' do = icon('pencil') - = link_to namespace_project_note_path(note.project.namespace, note.project, note), title: 'Remove comment', method: :delete, data: { confirm: 'Are you sure you want to remove this comment?' }, remote: true, class: 'note-action-button hidden-xs js-note-delete danger' do - = icon('trash-o') + - if Gitlab::Feature.removing_notes_enabled? + = link_to namespace_project_note_path(note.project.namespace, note.project, note), title: 'Remove comment', method: :delete, data: { confirm: 'Are you sure you want to remove this comment?' }, remote: true, class: 'note-action-button hidden-xs js-note-delete danger' do + = icon('trash-o') .note-body{class: note_editable ? 'js-task-list-container' : ''} .note-text = preserve do = markdown(note.note, pipeline: :note, cache_key: [note, "note"], author: note.author) = edited_time_ago_with_tooltip(note, placement: 'bottom', html_class: 'note_edited_ago', include_author: true) - - if note_editable + - if note_editable && Gitlab::Feature.updating_notes_enabled? = render 'projects/notes/edit_form', note: note .note-awards = render 'award_emoji/awards_block', awardable: note, inline: false diff --git a/app/views/projects/notes/_notes_with_form.html.haml b/app/views/projects/notes/_notes_with_form.html.haml index 1c39ce897a3f..b8c99389b4a3 100644 --- a/app/views/projects/notes/_notes_with_form.html.haml +++ b/app/views/projects/notes/_notes_with_form.html.haml @@ -2,20 +2,25 @@ = render "projects/notes/notes" %ul.notes.notes-form.timeline %li.timeline-entry - - if can? current_user, :create_note, @project - .timeline-icon.hidden-xs.hidden-sm - %a.author_link{ href: user_path(current_user) } - = image_tag avatar_icon(current_user), alt: current_user.to_reference, class: 'avatar s40' - .timeline-content.timeline-content-form - = render "projects/notes/form", view: diff_view + - if Gitlab::Feature.creating_notes_enabled? + - if can? current_user, :create_note, @project + .timeline-icon.hidden-xs.hidden-sm + %a.author_link{ href: user_path(current_user) } + = image_tag avatar_icon(current_user), alt: current_user.to_reference, class: 'avatar s40' + .timeline-content.timeline-content-form + = render "projects/notes/form", view: diff_view + - else + .disabled-comment.text-center + .disabled-comment-text.inline + Please + = link_to "register",new_user_session_path + or + = link_to "login",new_user_session_path + to post a comment - else .disabled-comment.text-center .disabled-comment-text.inline - Please - = link_to "register",new_user_session_path - or - = link_to "login",new_user_session_path - to post a comment + Creating comments is currently disabled. :javascript var notes = new Notes("#{namespace_project_notes_path(namespace_id: @project.namespace, target_id: @noteable.id, target_type: @noteable.class.name.underscore)}", #{@notes.map(&:id).to_json}, #{Time.now.to_i}, "#{diff_view}") diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 2aaa0557ea30..bae7f35a77dd 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -281,6 +281,12 @@ def render_api_error!(message, status) error!({ 'message' => message }, status) end + def require_feature!(feature) + unless Gitlab::Feature.feature_enabled?(feature) + render_api_error!('503 Service Unavailable', 503) + end + end + # Projects helpers def filter_projects(projects) diff --git a/lib/api/notes.rb b/lib/api/notes.rb index d4fcfd3d4d3b..eeb7e87a3bd8 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -71,6 +71,7 @@ class Notes < Grape::API # POST /projects/:id/issues/:noteable_id/notes # POST /projects/:id/snippets/:noteable_id/notes post ":id/#{noteables_str}/:#{noteable_id_str}/notes" do + require_feature! :creating_notes required_attributes! [:body] opts = { @@ -103,6 +104,7 @@ class Notes < Grape::API # PUT /projects/:id/issues/:noteable_id/notes/:note_id # PUT /projects/:id/snippets/:noteable_id/notes/:node_id put ":id/#{noteables_str}/:#{noteable_id_str}/notes/:note_id" do + require_feature! :updating_notes required_attributes! [:body] note = user_project.notes.find(params[:note_id]) @@ -132,6 +134,7 @@ class Notes < Grape::API # DELETE /projects/:id/issues/:noteable_id/notes/:note_id # DELETE /projects/:id/snippets/:noteable_id/notes/:node_id delete ":id/#{noteables_str}/:#{noteable_id_str}/notes/:note_id" do + require_feature! :removing_notes note = user_project.notes.find(params[:note_id]) authorize! :admin_note, note diff --git a/spec/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb index 00bc38b60714..e71b22008468 100644 --- a/spec/controllers/projects/notes_controller_spec.rb +++ b/spec/controllers/projects/notes_controller_spec.rb @@ -4,7 +4,50 @@ let(:user) { create(:user) } let(:project) { create(:project) } let(:issue) { create(:issue, project: project) } - let(:note) { create(:note, noteable: issue, project: project) } + let(:note) { create(:note, noteable: issue, project: project, author: user) } + + describe 'POST #create' do + before do + sign_in(user) + project.team << [user, :developer] + end + + context 'when creating notes is disabled' do + it 'responds with a service unavailable error' do + expect(Gitlab::Feature).to receive(:feature_enabled?). + with(:creating_notes). + and_return(false) + + post(:create, + namespace_id: project.namespace.path, + project_id: project.path) + + expect(response.status).to eq(503) + end + end + end + + describe 'PUT #update' do + before do + sign_in(user) + project.team << [user, :developer] + end + + context 'when updating notes is disabled' do + it 'responds with a service unavailable error' do + expect(Gitlab::Feature).to receive(:feature_enabled?). + with(:updating_notes). + and_return(false) + + put(:update, + namespace_id: project.namespace.path, + project_id: project.path, + id: note.id) + + expect(response.status).to eq(503) + end + end + end describe 'POST #toggle_award_emoji' do before do @@ -32,5 +75,18 @@ expect(response.status).to eq(200) end + + context 'when toggling award emoji is disabled' do + it 'responds with a service unavailable error' do + expect(Gitlab::Feature).to receive(:feature_enabled?). + with(:toggling_award_emoji). + and_return(false) + + post(:toggle_award_emoji, namespace_id: project.namespace.path, + project_id: project.path, id: note.id, name: "thumbsup") + + expect(response.status).to eq(503) + end + end end end diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb index beb29a686920..6f4ad308946d 100644 --- a/spec/requests/api/notes_spec.rb +++ b/spec/requests/api/notes_spec.rb @@ -266,6 +266,18 @@ expect(private_issue.notes.reload).to be_empty end end + + context 'when creating notes is disabled' do + it 'responds with a service unavailable error' do + expect(Gitlab::Feature).to receive(:feature_enabled?). + with(:creating_notes). + and_return(false) + + post api("/projects/#{project.id}/issues/#{issue.id}/notes", user), body: 'hi!' + + expect(response.status).to eq(503) + end + end end describe "POST /projects/:id/noteable/:noteable_id/notes to test observer on create" do @@ -334,6 +346,19 @@ expect(response.status).to eq(404) end end + + context 'when updating notes is disabled' do + it 'responds with a service unavailable error' do + expect(Gitlab::Feature).to receive(:feature_enabled?). + with(:updating_notes). + and_return(false) + + put api("/projects/#{project.id}/issues/#{issue.id}/notes/"\ + "#{issue_note.id}", user), body: 'hi!' + + expect(response.status).to eq(503) + end + end end describe 'DELETE /projects/:id/noteable/:noteable_id/notes/:note_id' do @@ -395,6 +420,19 @@ expect(response.status).to eq(404) end end + + context 'when deleting notes is disabled' do + it 'responds with a service unavailable error' do + expect(Gitlab::Feature).to receive(:feature_enabled?). + with(:removing_notes). + and_return(false) + + delete api("/projects/#{project.id}/issues/#{issue.id}/notes/"\ + "#{issue_note.id}", user) + + expect(response.status).to eq(503) + end + end end end -- GitLab