diff options
author | Z.J. van de Weg <zegerjan@gitlab.com> | 2016-06-01 12:23:09 +0300 |
---|---|---|
committer | Z.J. van de Weg <zegerjan@gitlab.com> | 2016-06-01 13:10:08 +0300 |
commit | 91a7b9333b660abc866e52e1a614151cb529413d (patch) | |
tree | c7ee15fd37e703229f6f197207304479251b5856 | |
parent | cbd7801b3d1d435a95ec70032c5acc9df33b0337 (diff) |
Incorportate feedback
-rw-r--r-- | app/models/concerns/issuable.rb | 4 | ||||
-rw-r--r-- | app/models/legacy_diff_note.rb | 4 | ||||
-rw-r--r-- | app/models/note.rb | 16 | ||||
-rw-r--r-- | app/services/notes/post_process_service.rb | 2 | ||||
-rw-r--r-- | app/services/notification_service.rb | 2 | ||||
-rw-r--r-- | app/views/award_emoji/_awards_block.html.haml | 8 | ||||
-rw-r--r-- | app/views/emoji_awards/_awards_block.html.haml | 18 | ||||
-rw-r--r-- | config/routes.rb | 1 | ||||
-rw-r--r-- | db/migrate/20160416180807_add_award_emoji.rb | 3 | ||||
-rw-r--r-- | db/migrate/20160416182152_convert_award_note_to_emoji_award.rb | 2 | ||||
-rw-r--r-- | db/schema.rb | 3 | ||||
-rw-r--r-- | features/steps/project/merge_requests.rb | 2 | ||||
-rw-r--r-- | lib/api/entities.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/award_emoji.rb | 18 | ||||
-rw-r--r-- | spec/controllers/groups_controller_spec.rb | 4 | ||||
-rw-r--r-- | spec/controllers/projects/issues_controller_spec.rb | 6 | ||||
-rw-r--r-- | spec/factories/award_emoji.rb | 6 | ||||
-rw-r--r-- | spec/features/issues/award_spec.rb | 35 | ||||
-rw-r--r-- | spec/features/merge_requests/award_spec.rb | 37 | ||||
-rw-r--r-- | spec/features/notes_on_merge_requests_spec.rb | 25 | ||||
-rw-r--r-- | spec/models/award_emoji_spec.rb | 3 |
21 files changed, 52 insertions, 149 deletions
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 57ded0f91ad..1fc0e002f47 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -133,6 +133,10 @@ module Issuable opened? || reopened? end + def user_notes_count + notes.user.count + end + def subscribed_without_subscriptions?(user) participants(user).include?(user) end diff --git a/app/models/legacy_diff_note.rb b/app/models/legacy_diff_note.rb index bbefc911b29..95fd510eb3a 100644 --- a/app/models/legacy_diff_note.rb +++ b/app/models/legacy_diff_note.rb @@ -110,6 +110,10 @@ class LegacyDiffNote < Note @active end + def award_emoji_supported? + false + end + private def find_diff diff --git a/app/models/note.rb b/app/models/note.rb index bbe5545dc80..f99d327a5b8 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -21,10 +21,8 @@ class Note < ActiveRecord::Base delegate :name, :email, to: :author, prefix: true delegate :title, to: :noteable, allow_nil: true - before_validation :clear_blank_line_code! - validates :note, :project, presence: true - validates :line_code, line_code: true, allow_blank: true + # Attachments are deprecated and are handled by Markdown uploader validates :attachment, file_size: { maximum: :max_attachment_size } @@ -173,10 +171,6 @@ class Note < ActiveRecord::Base Event.reset_event_cache_for(self) end - def system? - read_attribute(:system) - end - def editable? !system? end @@ -193,14 +187,8 @@ class Note < ActiveRecord::Base self.line_code = nil if self.line_code.blank? end - # Find the diff on noteable that matches our own - def find_noteable_diff - diffs = noteable.diffs(Commit.max_diff_options) - diffs.find { |d| d.new_path == self.diff.new_path } - end - def award_emoji_supported? - noteable.is_a?(Awardable) && !line_code.present? + noteable.is_a?(Awardable) end def contains_emoji_only? diff --git a/app/services/notes/post_process_service.rb b/app/services/notes/post_process_service.rb index c1bf46bdfb3..534c48aefff 100644 --- a/app/services/notes/post_process_service.rb +++ b/app/services/notes/post_process_service.rb @@ -8,7 +8,7 @@ module Notes def execute # Skip system notes, like status changes and cross-references and awards - unless @note.system + unless @note.system? EventCreateService.new.leave_note(@note, @note.author) @note.create_cross_references! execute_note_hooks diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 703636658b7..91ca82ed3b7 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -130,7 +130,7 @@ class NotificationService # ignore gitlab service messages return true if note.note.start_with?('Status changed to closed') - return true if note.cross_reference? && note.system == true + return true if note.cross_reference? && note.system? target = note.noteable diff --git a/app/views/award_emoji/_awards_block.html.haml b/app/views/award_emoji/_awards_block.html.haml index 19d1275a926..f1f93c1375b 100644 --- a/app/views/award_emoji/_awards_block.html.haml +++ b/app/views/award_emoji/_awards_block.html.haml @@ -1,7 +1,7 @@ - grouped_emojis = awardable.grouped_awards(with_thumbs: inline) -.awards.js-awards-block{ class: ("hidden" if !inline && grouped_emojis.size == 0), data: { award_url: url_for([:toggle_award_emoji, @project.namespace.becomes(Namespace), @project, awardable]) } } +.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{ type: "button", class: (award_active_class(awards, current_user)), data: { placement: "bottom", title: award_user_list(awards, current_user) } } = emoji_icon(emoji) %span.award-control-text.js-counter = awards.count @@ -12,7 +12,7 @@ .award-menu-holder.js-award-holder %button.btn.award-control.js-add-award{ type: "button", data: { award_menu_url: emojis_path } } - = icon('smile-o', {class: "award-control-icon award-control-icon-normal"}) - = icon('spinner spin', {class: "award-control-icon award-control-icon-loading"}) + = icon('smile-o', class: "award-control-icon award-control-icon-normal") + = icon('spinner spin', class: "award-control-icon award-control-icon-loading") %span.award-control-text Add diff --git a/app/views/emoji_awards/_awards_block.html.haml b/app/views/emoji_awards/_awards_block.html.haml deleted file mode 100644 index e9b286b7c3f..00000000000 --- a/app/views/emoji_awards/_awards_block.html.haml +++ /dev/null @@ -1,18 +0,0 @@ -- grouped_emojis = awardable.grouped_awards(inline) -.awards.js-awards-block{ class: ("hidden" if !inline && grouped_emojis.size == 0), data: { award_url: url_for([:toggle_emoji_award, @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)), title: award_user_list(awards, current_user), data: { placement: "bottom" } } - = emoji_icon(emoji) - %span.award-control-text.js-counter - = awards.count - - - if current_user - :javascript - gl.awardMenuUrl = emojis_path - - .award-menu-holder.js-award-holder - %button.btn.award-control.js-add-award{ type: "button", data: { award_menu_url: emojis_path } } - = icon('smile-o', {class: "award-control-icon award-control-icon-normal"}) - = icon('spinner spin', {class: "award-control-icon award-control-icon-loading"}) - %span.award-control-text - Add diff --git a/config/routes.rb b/config/routes.rb index 85760409c1f..cd28c421fe2 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -754,7 +754,6 @@ Rails.application.routes.draw do resources :notes, only: [:index, :create, :destroy, :update], constraints: { id: /\d+/ } do member do delete :delete_attachment - post :toggle_award_emoji end end diff --git a/db/migrate/20160416180807_add_award_emoji.rb b/db/migrate/20160416180807_add_award_emoji.rb index 3177b86a133..2ead181921b 100644 --- a/db/migrate/20160416180807_add_award_emoji.rb +++ b/db/migrate/20160416180807_add_award_emoji.rb @@ -9,7 +9,6 @@ class AddAwardEmoji < ActiveRecord::Migration end add_index :award_emoji, :user_id - add_index :award_emoji, :awardable_type - add_index :award_emoji, :awardable_id + add_index :award_emoji, [:awardable_type, :awardable_id] end end diff --git a/db/migrate/20160416182152_convert_award_note_to_emoji_award.rb b/db/migrate/20160416182152_convert_award_note_to_emoji_award.rb index d2efbd0abea..073bbc0fc2a 100644 --- a/db/migrate/20160416182152_convert_award_note_to_emoji_award.rb +++ b/db/migrate/20160416182152_convert_award_note_to_emoji_award.rb @@ -2,6 +2,8 @@ class ConvertAwardNoteToEmojiAward < ActiveRecord::Migration def change def up execute "INSERT INTO award_emoji (awardable_type, awardable_id, user_id, name, created_at, updated_at) (SELECT noteable_type, noteable_id, author_id, note, created_at, updated_at FROM notes WHERE is_award = true)" + + execute "DELETE FROM notes WHERE is_award = true" end end end diff --git a/db/schema.rb b/db/schema.rb index 9ef68efc4e5..e784f24bb8e 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -108,8 +108,7 @@ ActiveRecord::Schema.define(version: 20160528043124) do t.datetime "updated_at" end - add_index "award_emoji", ["awardable_id"], name: "index_award_emoji_on_awardable_id", using: :btree - add_index "award_emoji", ["awardable_type"], name: "index_award_emoji_on_awardable_type", using: :btree + add_index "award_emoji", ["awardable_type", "awardable_id"], name: "index_award_emoji_on_awardable_type_and_awardable_id", using: :btree add_index "award_emoji", ["user_id"], name: "index_award_emoji_on_user_id", using: :btree create_table "broadcast_messages", force: :cascade do |t| diff --git a/features/steps/project/merge_requests.rb b/features/steps/project/merge_requests.rb index f6fafe81194..11978aa65a1 100644 --- a/features/steps/project/merge_requests.rb +++ b/features/steps/project/merge_requests.rb @@ -186,7 +186,7 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps step 'merge request "Bug NS-06" have 1 upvote and 2 downvotes' do awardable = MergeRequest.find_by(title: 'Bug NS-06') create(:award_emoji, awardable: awardable) - create_list(:award_emoji, 2, awardable: awardable, name: "thumbsdown") + create_list(:award_emoji, 2, :downvote, awardable: awardable) end step 'The list should be sorted by "Least popular"' do diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 43e0ba388d0..a5582490a3a 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -175,6 +175,7 @@ module API expose :subscribed do |issue, options| issue.subscribed?(options[:current_user]) end + expose :user_notes_count expose :upvotes, :downvotes end @@ -192,6 +193,7 @@ module API expose :subscribed do |merge_request, options| merge_request.subscribed?(options[:current_user]) end + expose :user_notes_count end class MergeRequestChanges < MergeRequest diff --git a/lib/gitlab/award_emoji.rb b/lib/gitlab/award_emoji.rb index 0ae220a86bc..51b1df9ecbd 100644 --- a/lib/gitlab/award_emoji.rb +++ b/lib/gitlab/award_emoji.rb @@ -47,17 +47,19 @@ module Gitlab end def self.emojis - @emojis ||= begin - json_path = File.join(Rails.root, 'fixtures', 'emojis', 'index.json' ) - JSON.parse(File.read(json_path)) - end + @emojis ||= + begin + json_path = File.join(Rails.root, 'fixtures', 'emojis', 'index.json' ) + JSON.parse(File.read(json_path)) + end end def self.aliases - @aliases ||= begin - json_path = File.join(Rails.root, 'fixtures', 'emojis', 'aliases.json' ) - JSON.parse(File.read(json_path)) - end + @aliases ||= + begin + json_path = File.join(Rails.root, 'fixtures', 'emojis', 'aliases.json' ) + JSON.parse(File.read(json_path)) + end end # Returns an Array of Emoji names and their asset URLs. diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index 82b25702172..cd98fecd0c7 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -33,7 +33,7 @@ describe GroupsController do before do create_list(:award_emoji, 3, awardable: issue_2) create_list(:award_emoji, 2, awardable: issue_1) - create_list(:award_emoji, 2, awardable: issue_2, name: "thumbsdown") + create_list(:award_emoji, 2, :downvote, awardable: issue_2,) sign_in(user) end @@ -58,7 +58,7 @@ describe GroupsController do before do create_list(:award_emoji, 3, awardable: merge_request_2) create_list(:award_emoji, 2, awardable: merge_request_1) - create_list(:award_emoji, 2, awardable: merge_request_2, name: "thumbsdown") + create_list(:award_emoji, 2, :downvote, awardable: merge_request_2) sign_in(user) end diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 849816b9caf..78be7e3dc35 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -258,10 +258,10 @@ describe Projects::IssuesController do end it "toggles the award emoji" do - expect do + expect do post(:toggle_award_emoji, namespace_id: project.namespace.path, - project_id: project.path, id: issue.iid, name: "thumbsup") - end.to change { AwardEmoji.count }.by(1) + project_id: project.path, id: issue.iid, name: "thumbsup") + end.to change { issue.award_emoji.count }.by(1) expect(response.status).to eq(200) end diff --git a/spec/factories/award_emoji.rb b/spec/factories/award_emoji.rb index b09f8b0bc74..4b858df52c9 100644 --- a/spec/factories/award_emoji.rb +++ b/spec/factories/award_emoji.rb @@ -4,13 +4,7 @@ FactoryGirl.define do user awardable factory: :issue - trait :thumbs_up trait :upvote - - trait :thumbs_down do - name "thumbsdown" - end - trait :downvote do name "thumbsdown" end diff --git a/spec/features/issues/award_spec.rb b/spec/features/issues/award_spec.rb index eafd517c840..63efecf8780 100644 --- a/spec/features/issues/award_spec.rb +++ b/spec/features/issues/award_spec.rb @@ -4,7 +4,6 @@ feature 'Issue awards', js: true, feature: true do let(:user) { create(:user) } let(:project) { create(:project, :public) } let(:issue) { create(:issue, project: project) } - let!(:note) { create(:note_on_issue, project: project, noteable: issue, note: 'Looks good!') } describe 'logged in' do before do @@ -16,12 +15,18 @@ feature 'Issue awards', js: true, feature: true do first('.js-emoji-btn').click expect(page).to have_selector('.js-emoji-btn.active') expect(first('.js-emoji-btn')).to have_content '1' + + visit namespace_project_issue_path(project.namespace, project, issue) + expect(first('.js-emoji-btn')).to have_content '1' end it 'should remove award from issue' do first('.js-emoji-btn').click find('.js-emoji-btn.active').click expect(first('.js-emoji-btn')).to have_content '0' + + visit namespace_project_issue_path(project.namespace, project, issue) + expect(first('.js-emoji-btn')).to have_content '0' end it 'should only have one menu on the page' do @@ -40,33 +45,5 @@ feature 'Issue awards', js: true, feature: true do it 'should not see award menu button' do expect(page).not_to have_selector('.js-award-holder') end - - it 'should not see award menu button in note' do - page.within('.note') do - expect(page).not_to have_selector('.js-award-action-btn') - end - end - end - - def show_note_award_menu - page.within('.note') do - find('.js-add-award').click - end - expect(page).to have_selector('.emoji-menu') - end - - def award_on_note(index = 1) - page.within('.emoji-menu') do - buttons = all('.js-emoji-btn') - buttons[index].click - end - end - - def remove_award_on_note - page.within('.note') do - page.within('.js-awards-block') do - first('.js-emoji-btn').click - end - end end end diff --git a/spec/features/merge_requests/award_spec.rb b/spec/features/merge_requests/award_spec.rb index 4d3e8173ebe..007f67d6080 100644 --- a/spec/features/merge_requests/award_spec.rb +++ b/spec/features/merge_requests/award_spec.rb @@ -3,8 +3,7 @@ require 'rails_helper' feature 'Merge request awards', js: true, feature: true do let(:user) { create(:user) } let(:project) { create(:project, :public) } - let(:merge_request) { create(:merge_request_with_diffs, source_project: project) } - let!(:note) { create(:note_on_merge_request, project: project, noteable: merge_request, note: 'Looks good!') } + let(:merge_request) { create(:merge_request, source_project: project) } describe 'logged in' do before do @@ -16,12 +15,18 @@ feature 'Merge request awards', js: true, feature: true do first('.js-emoji-btn').click expect(page).to have_selector('.js-emoji-btn.active') expect(first('.js-emoji-btn')).to have_content '1' + + visit namespace_project_merge_request_path(project.namespace, project, merge_request) + expect(first('.js-emoji-btn')).to have_content '1' end it 'should remove award from merge request' do first('.js-emoji-btn').click find('.js-emoji-btn.active').click expect(first('.js-emoji-btn')).to have_content '0' + + visit namespace_project_merge_request_path(project.namespace, project, merge_request) + expect(first('.js-emoji-btn')).to have_content '0' end it 'should only have one menu on the page' do @@ -40,33 +45,5 @@ feature 'Merge request awards', js: true, feature: true do it 'should not see award menu button' do expect(page).not_to have_selector('.js-award-holder') end - - it 'should not see award menu button in note' do - page.within('.note') do - expect(page).not_to have_selector('.js-award-action-btn') - end - end - end - - def show_note_award_menu - page.within('.note') do - find('.js-add-award').click - end - expect(page).to have_selector('.emoji-menu') - end - - def award_on_note(index = 1) - page.within('.emoji-menu') do - buttons = all('.js-emoji-btn') - buttons[index].click - end - end - - def remove_award_on_note - page.within('.note') do - page.within('.js-awards-block') do - first('.js-emoji-btn').click - end - end end end diff --git a/spec/features/notes_on_merge_requests_spec.rb b/spec/features/notes_on_merge_requests_spec.rb index de52460f15f..737efcef45d 100644 --- a/spec/features/notes_on_merge_requests_spec.rb +++ b/spec/features/notes_on_merge_requests_spec.rb @@ -4,20 +4,6 @@ describe 'Comments', feature: true do include RepoHelpers include WaitForAjax - describe 'On merge requests page', feature: true do - it 'excludes award_emoji from comment count' do - merge_request = create(:merge_request) - project = merge_request.source_project - create(:award_emoji, awardable: merge_request) - - login_as :admin - visit namespace_project_merge_requests_path(project.namespace, project) - - expect(merge_request.mr_and_commit_notes.count).to eq 0 - expect(page.all('.merge-request-no-comments').first.text).to eq "0" - end - end - describe 'On a merge request', js: true, feature: true do let!(:project) { create(:project) } let!(:merge_request) do @@ -147,17 +133,6 @@ describe 'Comments', feature: true do end end end - - describe 'comment info' do - it 'excludes award_emoji from comment count' do - create(:award_emoji, awardable: merge_request) - - visit namespace_project_merge_request_path(project.namespace, project, merge_request) - - expect(merge_request.mr_and_commit_notes.count).to eq 1 - expect(find('.notes-tab span.badge').text).to eq "1" - end - end end describe 'On a merge request diff', js: true, feature: true do diff --git a/spec/models/award_emoji_spec.rb b/spec/models/award_emoji_spec.rb index fd3712b7d43..cb3c592f8cd 100644 --- a/spec/models/award_emoji_spec.rb +++ b/spec/models/award_emoji_spec.rb @@ -14,7 +14,6 @@ describe AwardEmoji, models: true do it { is_expected.to validate_presence_of(:awardable) } it { is_expected.to validate_presence_of(:user) } it { is_expected.to validate_presence_of(:name) } - it { is_expected.to validate_presence_of(:awardable) } # To circumvent a bug in the shoulda matchers describe "scoped uniqueness validation" do @@ -22,7 +21,7 @@ describe AwardEmoji, models: true do user = create(:user) issue = create(:issue) create(:award_emoji, user: user, awardable: issue) - new_award = AwardEmoji.new(user: user, awardable: issue, name: "thumbsup") + new_award = build(:award_emoji, user: user, awardable: issue) expect(new_award).not_to be_valid end |