From beed70c0248fea13bdf727e5f14dee362b0cd298 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Thu, 9 Jun 2016 17:17:01 +0200 Subject: 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 +++ app/controllers/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 ++ spec/controllers/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 62f63701799..713d552976c 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -366,6 +366,14 @@ class ApplicationController < ActionController::Base 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 036777c80c1..13c3de4ddbb 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 836f79ff080..db046aaa7a4 100644 --- a/app/controllers/projects/notes_controller.rb +++ b/app/controllers/projects/notes_controller.rb @@ -22,39 +22,47 @@ class Projects::NotesController < Projects::ApplicationController 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 02efcecc889..44263e97231 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 f1577e8a47b..9e46625378a 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 bcdbff08011..a2b614486ad 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 1c39ce897a3..b8c99389b4a 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 2aaa0557ea3..bae7f35a77d 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -281,6 +281,12 @@ module API 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 d4fcfd3d4d3..eeb7e87a3bd 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -71,6 +71,7 @@ module 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 @@ module 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 @@ module 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 00bc38b6071..e71b2200846 100644 --- a/spec/controllers/projects/notes_controller_spec.rb +++ b/spec/controllers/projects/notes_controller_spec.rb @@ -4,7 +4,50 @@ describe Projects::NotesController do 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 @@ describe Projects::NotesController do 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 beb29a68692..6f4ad308946 100644 --- a/spec/requests/api/notes_spec.rb +++ b/spec/requests/api/notes_spec.rb @@ -266,6 +266,18 @@ describe API::API, api: true do 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 @@ describe API::API, api: true do 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 @@ describe API::API, api: true do 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 -- cgit v1.2.1