Commit 91a7b933 authored by Z.J. van de Weg's avatar Z.J. van de Weg

Incorportate feedback

parent cbd7801b
...@@ -133,6 +133,10 @@ module Issuable ...@@ -133,6 +133,10 @@ module Issuable
opened? || reopened? opened? || reopened?
end end
def user_notes_count
notes.user.count
end
def subscribed_without_subscriptions?(user) def subscribed_without_subscriptions?(user)
participants(user).include?(user) participants(user).include?(user)
end end
......
...@@ -110,6 +110,10 @@ class LegacyDiffNote < Note ...@@ -110,6 +110,10 @@ class LegacyDiffNote < Note
@active @active
end end
def award_emoji_supported?
false
end
private private
def find_diff def find_diff
......
...@@ -21,10 +21,8 @@ class Note < ActiveRecord::Base ...@@ -21,10 +21,8 @@ class Note < ActiveRecord::Base
delegate :name, :email, to: :author, prefix: true delegate :name, :email, to: :author, prefix: true
delegate :title, to: :noteable, allow_nil: true delegate :title, to: :noteable, allow_nil: true
before_validation :clear_blank_line_code!
validates :note, :project, presence: true validates :note, :project, presence: true
validates :line_code, line_code: true, allow_blank: true
# Attachments are deprecated and are handled by Markdown uploader # Attachments are deprecated and are handled by Markdown uploader
validates :attachment, file_size: { maximum: :max_attachment_size } validates :attachment, file_size: { maximum: :max_attachment_size }
...@@ -173,10 +171,6 @@ class Note < ActiveRecord::Base ...@@ -173,10 +171,6 @@ class Note < ActiveRecord::Base
Event.reset_event_cache_for(self) Event.reset_event_cache_for(self)
end end
def system?
read_attribute(:system)
end
def editable? def editable?
!system? !system?
end end
...@@ -193,14 +187,8 @@ class Note < ActiveRecord::Base ...@@ -193,14 +187,8 @@ class Note < ActiveRecord::Base
self.line_code = nil if self.line_code.blank? self.line_code = nil if self.line_code.blank?
end 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? def award_emoji_supported?
noteable.is_a?(Awardable) && !line_code.present? noteable.is_a?(Awardable)
end end
def contains_emoji_only? def contains_emoji_only?
......
...@@ -8,7 +8,7 @@ module Notes ...@@ -8,7 +8,7 @@ module Notes
def execute def execute
# Skip system notes, like status changes and cross-references and awards # Skip system notes, like status changes and cross-references and awards
unless @note.system unless @note.system?
EventCreateService.new.leave_note(@note, @note.author) EventCreateService.new.leave_note(@note, @note.author)
@note.create_cross_references! @note.create_cross_references!
execute_note_hooks execute_note_hooks
......
...@@ -130,7 +130,7 @@ class NotificationService ...@@ -130,7 +130,7 @@ class NotificationService
# ignore gitlab service messages # ignore gitlab service messages
return true if note.note.start_with?('Status changed to closed') 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 target = note.noteable
......
- grouped_emojis = awardable.grouped_awards(with_thumbs: inline) - 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| - 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) = emoji_icon(emoji)
%span.award-control-text.js-counter %span.award-control-text.js-counter
= awards.count = awards.count
...@@ -12,7 +12,7 @@ ...@@ -12,7 +12,7 @@
.award-menu-holder.js-award-holder .award-menu-holder.js-award-holder
%button.btn.award-control.js-add-award{ type: "button", data: { award_menu_url: emojis_path } } %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('smile-o', class: "award-control-icon award-control-icon-normal")
= icon('spinner spin', {class: "award-control-icon award-control-icon-loading"}) = icon('spinner spin', class: "award-control-icon award-control-icon-loading")
%span.award-control-text %span.award-control-text
Add Add
- 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
...@@ -754,7 +754,6 @@ Rails.application.routes.draw do ...@@ -754,7 +754,6 @@ Rails.application.routes.draw do
resources :notes, only: [:index, :create, :destroy, :update], constraints: { id: /\d+/ } do resources :notes, only: [:index, :create, :destroy, :update], constraints: { id: /\d+/ } do
member do member do
delete :delete_attachment delete :delete_attachment
post :toggle_award_emoji
end end
end end
......
...@@ -9,7 +9,6 @@ class AddAwardEmoji < ActiveRecord::Migration ...@@ -9,7 +9,6 @@ class AddAwardEmoji < ActiveRecord::Migration
end end
add_index :award_emoji, :user_id add_index :award_emoji, :user_id
add_index :award_emoji, :awardable_type add_index :award_emoji, [:awardable_type, :awardable_id]
add_index :award_emoji, :awardable_id
end end
end end
...@@ -2,6 +2,8 @@ class ConvertAwardNoteToEmojiAward < ActiveRecord::Migration ...@@ -2,6 +2,8 @@ class ConvertAwardNoteToEmojiAward < ActiveRecord::Migration
def change def change
def up 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 "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 end
end end
...@@ -108,8 +108,7 @@ ActiveRecord::Schema.define(version: 20160528043124) do ...@@ -108,8 +108,7 @@ ActiveRecord::Schema.define(version: 20160528043124) do
t.datetime "updated_at" t.datetime "updated_at"
end end
add_index "award_emoji", ["awardable_id"], name: "index_award_emoji_on_awardable_id", 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", ["awardable_type"], name: "index_award_emoji_on_awardable_type", using: :btree
add_index "award_emoji", ["user_id"], name: "index_award_emoji_on_user_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| create_table "broadcast_messages", force: :cascade do |t|
......
...@@ -186,7 +186,7 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps ...@@ -186,7 +186,7 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
step 'merge request "Bug NS-06" have 1 upvote and 2 downvotes' do step 'merge request "Bug NS-06" have 1 upvote and 2 downvotes' do
awardable = MergeRequest.find_by(title: 'Bug NS-06') awardable = MergeRequest.find_by(title: 'Bug NS-06')
create(:award_emoji, awardable: awardable) create(:award_emoji, awardable: awardable)
create_list(:award_emoji, 2, awardable: awardable, name: "thumbsdown") create_list(:award_emoji, 2, :downvote, awardable: awardable)
end end
step 'The list should be sorted by "Least popular"' do step 'The list should be sorted by "Least popular"' do
......
...@@ -175,6 +175,7 @@ module API ...@@ -175,6 +175,7 @@ module API
expose :subscribed do |issue, options| expose :subscribed do |issue, options|
issue.subscribed?(options[:current_user]) issue.subscribed?(options[:current_user])
end end
expose :user_notes_count
expose :upvotes, :downvotes expose :upvotes, :downvotes
end end
...@@ -192,6 +193,7 @@ module API ...@@ -192,6 +193,7 @@ module API
expose :subscribed do |merge_request, options| expose :subscribed do |merge_request, options|
merge_request.subscribed?(options[:current_user]) merge_request.subscribed?(options[:current_user])
end end
expose :user_notes_count
end end
class MergeRequestChanges < MergeRequest class MergeRequestChanges < MergeRequest
......
...@@ -47,14 +47,16 @@ module Gitlab ...@@ -47,14 +47,16 @@ module Gitlab
end end
def self.emojis def self.emojis
@emojis ||= begin @emojis ||=
begin
json_path = File.join(Rails.root, 'fixtures', 'emojis', 'index.json' ) json_path = File.join(Rails.root, 'fixtures', 'emojis', 'index.json' )
JSON.parse(File.read(json_path)) JSON.parse(File.read(json_path))
end end
end end
def self.aliases def self.aliases
@aliases ||= begin @aliases ||=
begin
json_path = File.join(Rails.root, 'fixtures', 'emojis', 'aliases.json' ) json_path = File.join(Rails.root, 'fixtures', 'emojis', 'aliases.json' )
JSON.parse(File.read(json_path)) JSON.parse(File.read(json_path))
end end
......
...@@ -33,7 +33,7 @@ describe GroupsController do ...@@ -33,7 +33,7 @@ describe GroupsController do
before do before do
create_list(:award_emoji, 3, awardable: issue_2) create_list(:award_emoji, 3, awardable: issue_2)
create_list(:award_emoji, 2, awardable: issue_1) 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) sign_in(user)
end end
...@@ -58,7 +58,7 @@ describe GroupsController do ...@@ -58,7 +58,7 @@ describe GroupsController do
before do before do
create_list(:award_emoji, 3, awardable: merge_request_2) 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_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) sign_in(user)
end end
......
...@@ -261,7 +261,7 @@ describe Projects::IssuesController do ...@@ -261,7 +261,7 @@ describe Projects::IssuesController do
expect do expect do
post(:toggle_award_emoji, namespace_id: project.namespace.path, post(:toggle_award_emoji, namespace_id: project.namespace.path,
project_id: project.path, id: issue.iid, name: "thumbsup") project_id: project.path, id: issue.iid, name: "thumbsup")
end.to change { AwardEmoji.count }.by(1) end.to change { issue.award_emoji.count }.by(1)
expect(response.status).to eq(200) expect(response.status).to eq(200)
end end
......
...@@ -4,13 +4,7 @@ FactoryGirl.define do ...@@ -4,13 +4,7 @@ FactoryGirl.define do
user user
awardable factory: :issue awardable factory: :issue
trait :thumbs_up
trait :upvote trait :upvote
trait :thumbs_down do
name "thumbsdown"
end
trait :downvote do trait :downvote do
name "thumbsdown" name "thumbsdown"
end end
......
...@@ -4,7 +4,6 @@ feature 'Issue awards', js: true, feature: true do ...@@ -4,7 +4,6 @@ feature 'Issue awards', js: true, feature: true do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:project) { create(:project, :public) } let(:project) { create(:project, :public) }
let(:issue) { create(:issue, project: project) } let(:issue) { create(:issue, project: project) }
let!(:note) { create(:note_on_issue, project: project, noteable: issue, note: 'Looks good!') }
describe 'logged in' do describe 'logged in' do
before do before do
...@@ -16,12 +15,18 @@ feature 'Issue awards', js: true, feature: true do ...@@ -16,12 +15,18 @@ feature 'Issue awards', js: true, feature: true do
first('.js-emoji-btn').click first('.js-emoji-btn').click
expect(page).to have_selector('.js-emoji-btn.active') expect(page).to have_selector('.js-emoji-btn.active')
expect(first('.js-emoji-btn')).to have_content '1' 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 end
it 'should remove award from issue' do it 'should remove award from issue' do
first('.js-emoji-btn').click first('.js-emoji-btn').click
find('.js-emoji-btn.active').click find('.js-emoji-btn.active').click
expect(first('.js-emoji-btn')).to have_content '0' 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 end
it 'should only have one menu on the page' do it 'should only have one menu on the page' do
...@@ -40,33 +45,5 @@ feature 'Issue awards', js: true, feature: true do ...@@ -40,33 +45,5 @@ feature 'Issue awards', js: true, feature: true do
it 'should not see award menu button' do it 'should not see award menu button' do
expect(page).not_to have_selector('.js-award-holder') expect(page).not_to have_selector('.js-award-holder')
end 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
end end
...@@ -3,8 +3,7 @@ require 'rails_helper' ...@@ -3,8 +3,7 @@ require 'rails_helper'
feature 'Merge request awards', js: true, feature: true do feature 'Merge request awards', js: true, feature: true do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:project) { create(:project, :public) } let(:project) { create(:project, :public) }
let(:merge_request) { create(:merge_request_with_diffs, source_project: project) } let(:merge_request) { create(:merge_request, source_project: project) }
let!(:note) { create(:note_on_merge_request, project: project, noteable: merge_request, note: 'Looks good!') }
describe 'logged in' do describe 'logged in' do
before do before do
...@@ -16,12 +15,18 @@ feature 'Merge request awards', js: true, feature: true do ...@@ -16,12 +15,18 @@ feature 'Merge request awards', js: true, feature: true do
first('.js-emoji-btn').click first('.js-emoji-btn').click
expect(page).to have_selector('.js-emoji-btn.active') expect(page).to have_selector('.js-emoji-btn.active')
expect(first('.js-emoji-btn')).to have_content '1' 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 end
it 'should remove award from merge request' do it 'should remove award from merge request' do
first('.js-emoji-btn').click first('.js-emoji-btn').click
find('.js-emoji-btn.active').click find('.js-emoji-btn.active').click
expect(first('.js-emoji-btn')).to have_content '0' 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 end
it 'should only have one menu on the page' do it 'should only have one menu on the page' do
...@@ -40,33 +45,5 @@ feature 'Merge request awards', js: true, feature: true do ...@@ -40,33 +45,5 @@ feature 'Merge request awards', js: true, feature: true do
it 'should not see award menu button' do it 'should not see award menu button' do
expect(page).not_to have_selector('.js-award-holder') expect(page).not_to have_selector('.js-award-holder')
end 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
end end
...@@ -4,20 +4,6 @@ describe 'Comments', feature: true do ...@@ -4,20 +4,6 @@ describe 'Comments', feature: true do
include RepoHelpers include RepoHelpers
include WaitForAjax 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 describe 'On a merge request', js: true, feature: true do
let!(:project) { create(:project) } let!(:project) { create(:project) }
let!(:merge_request) do let!(:merge_request) do
...@@ -147,17 +133,6 @@ describe 'Comments', feature: true do ...@@ -147,17 +133,6 @@ describe 'Comments', feature: true do
end end
end 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 end
describe 'On a merge request diff', js: true, feature: true do describe 'On a merge request diff', js: true, feature: true do
......
...@@ -14,7 +14,6 @@ describe AwardEmoji, models: true do ...@@ -14,7 +14,6 @@ describe AwardEmoji, models: true do
it { is_expected.to validate_presence_of(:awardable) } it { is_expected.to validate_presence_of(:awardable) }
it { is_expected.to validate_presence_of(:user) } it { is_expected.to validate_presence_of(:user) }
it { is_expected.to validate_presence_of(:name) } it { is_expected.to validate_presence_of(:name) }
it { is_expected.to validate_presence_of(:awardable) }
# To circumvent a bug in the shoulda matchers # To circumvent a bug in the shoulda matchers
describe "scoped uniqueness validation" do describe "scoped uniqueness validation" do
...@@ -22,7 +21,7 @@ describe AwardEmoji, models: true do ...@@ -22,7 +21,7 @@ describe AwardEmoji, models: true do
user = create(:user) user = create(:user)
issue = create(:issue) issue = create(:issue)
create(:award_emoji, user: user, awardable: 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 expect(new_award).not_to be_valid
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment