Address suggestions from last code review.

* Added new serializers for Issue and MergeRequest.

app/controllers/projects/issues_controller.rb

@@ -75,7 +75,7 @@ class Projects::IssuesController < Projects::ApplicationController
     respond_to do |format|
       format.html
       format.json do
-        render json: @issue.to_json(include: [:milestone, :labels], methods: [:total_time_spent, :human_total_time_spent, :human_time_estimate])
+        render json: IssueSerializer.new.represent(@issue)
       end
     end
   end

app/controllers/projects/merge_requests_controller.rb

@@ -62,7 +62,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
       format.html { define_discussion_vars }
 
       format.json do
-        render json: @merge_request, methods: :rebase_in_progress?
+        render json: MergeRequestSerializer.new.represent(@merge_request)
       end
 
       format.patch  do

app/models/concerns/time_trackable.rb

@@ -27,15 +27,10 @@ module TimeTrackable
     if seconds == :reset
       reset_spent_time
     else
-      add_or_susbtract_spent_time
+      add_or_subtract_spent_time
     end
   end
 
-  def spend_time!(seconds, user)
-    spend_time(seconds, user)
-    save!
-  end
-
   def total_time_spent
     timelogs.sum(:time_spent)
   end
@@ -54,7 +49,7 @@ module TimeTrackable
     timelogs.new(time_spent: total_time_spent * -1, user: @time_spent_user)
   end
 
-  def add_or_susbtract_spent_time
+  def add_or_subtract_spent_time
     # Exit if time to subtract exceeds the total time spent.
     return if time_spent < 0 && (time_spent.abs > total_time_spent)
 

app/models/note.rb

@@ -19,7 +19,7 @@ class Note < ActiveRecord::Base
   # Banzai::ObjectRenderer
   attr_accessor :user_visible_reference_count
 
-  # Attributes used to store the attributes that have ben changed by slash commands.
+  # Attribute used to store the attributes that have ben changed by slash commands.
   attr_accessor :commands_changes
 
   default_value_for :system, false

app/models/timelog.rb

 class Timelog < ActiveRecord::Base
-  validates :time_spent, presence: true
+  validates :time_spent, :user, presence: true
 
   belongs_to :trackable, polymorphic: true
   belongs_to :user

app/serializers/issuable_entity.rb

+class IssuableEntity < Grape::Entity
+  expose :id
+  expose :iid
+  expose :assignee_id
+  expose :author_id
+  expose :description
+  expose :lock_version
+  expose :milestone_id
+  expose :position
+  expose :state
+  expose :title
+  expose :updated_by_id
+  expose :created_at
+  expose :updated_at
+  expose :deleted_at
+  expose :time_estimate
+  expose :total_time_spent
+  expose :human_time_estimate
+  expose :human_total_time_spent
+end
+

app/serializers/issue_entity.rb

+class IssueEntity < IssuableEntity
+  expose :branch_name
+  expose :confidential
+  expose :due_date
+  expose :moved_to_id
+  expose :project_id
+  expose :updated_by_id
+  expose :weight
+  expose :milestone, using: API::Entities::Milestone
+  expose :labels, using: LabelEntity
+end

app/serializers/issue_serializer.rb

+ class IssueSerializer < BaseSerializer
+   entity IssueEntity
+ end

app/serializers/label_entity.rb

+class LabelEntity < Grape::Entity
+  expose :id
+  expose :title
+  expose :color
+  expose :description
+  expose :group_id
+  expose :project_id
+  expose :template
+  expose :created_at
+  expose :updated_at
+end

app/serializers/merge_request_entity.rb

+class MergeRequestEntity < IssuableEntity
+  expose :approvals_before_merge
+  expose :in_progress_merge_commit_sha
+  expose :locked_at
+  expose :merge_commit_sha
+  expose :merge_error
+  expose :merge_params
+  expose :merge_status
+  expose :merge_user_id
+  expose :merge_when_build_succeeds
+  expose :rebase_commit_sha
+  expose :rebase_in_progress?
+  expose :source_branch
+  expose :source_project_id
+  expose :target_branch
+  expose :target_project_id
+end

app/serializers/merge_request_serializer.rb

+ class MergeRequestSerializer < BaseSerializer
+   entity MergeRequestEntity
+ end

app/services/issuable_base_service.rb

@@ -239,9 +239,9 @@ class IssuableBaseService < BaseService
   end
 
   def change_time_spent(issuable)
-    if params[:spend_time]
-      issuable.spend_time(params.delete(:spend_time), current_user)
-    end
+    time_spent = params.delete(:spend_time)
+
+    issuable.spend_time(time_spent, current_user) if time_spent
   end
 
   def has_changes?(issuable, old_labels: [])

app/services/slash_commands/interpret_service.rb

@@ -269,9 +269,10 @@ module SlashCommands
     command :spend do |raw_duration|
       reduce_time = raw_duration.sub!(/\A-/, '')
       time_spent = ChronicDuration.parse(raw_duration, default_unit: 'hours') rescue nil
-      time_spent *= -1 if time_spent && reduce_time
 
       if time_spent
+        time_spent *= -1 if reduce_time
+
         @updates[:spend_time] = time_spent
       end
     end

app/services/system_note_service.rb

@@ -126,10 +126,10 @@ module SystemNoteService
 
   def change_time_estimate(noteable, project, author)
     parsed_time = ChronicDuration.output(noteable.time_estimate, format: :short)
-    body = if parsed_time
-             "Changed time estimate of this #{noteable.human_class_name} to #{parsed_time}"
-           else
+    body = if noteable.time_estimate == 0
              "Removed time estimate on this #{noteable.human_class_name}"
+           else
+             "Changed time estimate of this #{noteable.human_class_name} to #{parsed_time}"
            end
 
     create_note(noteable: noteable, project: project, author: author, note: body)
@@ -155,7 +155,7 @@ module SystemNoteService
       body = "Removed time spent on this #{noteable.human_class_name}"
     else
       parsed_time = ChronicDuration.output(time_spent.abs, format: :short)
-      action = time_spent > 0 ? 'Added' : 'Substracted'
+      action = time_spent > 0 ? 'Added' : 'Subtracted'
       body = "#{action} #{parsed_time} of time spent on this #{noteable.human_class_name}"
     end
 

spec/factories/timelogs.rb

@@ -3,6 +3,7 @@
 FactoryGirl.define do
   factory :timelog do
     time_spent 3600
+    user
     association :trackable, factory: :issue
   end
 end

spec/models/concerns/issuable_spec.rb

@@ -389,9 +389,14 @@ describe Issue, "Issuable" do
     let(:user) { create(:user) }
     let(:issue) { create(:issue) }
 
+    def spend_time(seconds)
+      issue.spend_time(seconds, user)
+      issue.save!
+    end
+
     context 'adding time' do
       it 'should update the total time spent' do
-        issue.spend_time!(1800, user)
+        spend_time(1800)
 
         expect(issue.total_time_spent).to eq(1800)
       end
@@ -399,18 +404,18 @@ describe Issue, "Issuable" do
 
     context 'substracting time' do
       before do
-        issue.spend_time!(1800, user)
+        spend_time(1800)
       end
 
       it 'should update the total time spent' do
-        issue.spend_time!(-900, user)
+        spend_time(-900)
 
         expect(issue.total_time_spent).to eq(900)
       end
 
       context 'when time to substract exceeds the total time spent' do
         it 'should not alter the total time spent' do
-          issue.spend_time!(-3600, user)
+          spend_time(-3600)
 
           expect(issue.total_time_spent).to eq(1800)
         end

spec/models/timelog_spec.rb

@@ -6,4 +6,5 @@ RSpec.describe Timelog, type: :model do
   it { is_expected.to be_valid }
 
   it { is_expected.to validate_presence_of(:time_spent) }
+  it { is_expected.to validate_presence_of(:user) }
 end

spec/services/slash_commands/interpret_service_spec.rb

@@ -211,7 +211,7 @@ describe SlashCommands::InterpretService, services: true do
     end
 
     shared_examples 'estimate command' do
-      it 'populates time_estimate: "3600" if content contains /estimate 1h' do
+      it 'populates time_estimate: 3600 if content contains /estimate 1h' do
         _, updates = service.execute(content, issuable)
 
         expect(updates).to eq(time_estimate: 3600)
@@ -219,7 +219,7 @@ describe SlashCommands::InterpretService, services: true do
     end
 
     shared_examples 'spend command' do
-      it 'populates spend_time: { seconds: 3600, user: user } if content contains /spend 1h' do
+      it 'populates spend_time: 3600 if content contains /spend 1h' do
         _, updates = service.execute(content, issuable)
 
         expect(updates).to eq(spend_time: 3600)
@@ -227,7 +227,7 @@ describe SlashCommands::InterpretService, services: true do
     end
 
     shared_examples 'spend command with negative time' do
-      it 'populates spend_time: { seconds: -1800, user: user } if content contains /spend -30m' do
+      it 'populates spend_time: -1800 if content contains /spend -30m' do
         _, updates = service.execute(content, issuable)
 
         expect(updates).to eq(spend_time: -1800)
@@ -235,7 +235,7 @@ describe SlashCommands::InterpretService, services: true do
     end
 
     shared_examples 'remove_estimate command' do
-      it 'populates time_estimate: "0" if content contains /remove_estimate' do
+      it 'populates time_estimate: 0 if content contains /remove_estimate' do
         _, updates = service.execute(content, issuable)
 
         expect(updates).to eq(time_estimate: 0)
@@ -243,7 +243,7 @@ describe SlashCommands::InterpretService, services: true do
     end
 
     shared_examples 'remove_time_spent command' do
-      it 'populates spend_time: ":reset" if content contains /remove_time_spent' do
+      it 'populates spend_time: :reset if content contains /remove_time_spent' do
         _, updates = service.execute(content, issuable)
 
         expect(updates).to eq(spend_time: :reset)