Address feedback from last code review.

app/controllers/projects/notes_controller.rb

@@ -146,21 +146,27 @@ class Projects::NotesController < Projects::ApplicationController
   end
 
   def note_json(note)
-    if note.is_a?(AwardEmoji)
     attrs = {
+      award: false,
+      id: note.id,
+      commands_changes: note.commands_changes
+    }
+
+    if note.is_a?(AwardEmoji)
+      attrs.merge!(
         valid:  note.valid?,
         award:  true,
         name:   note.name
-      }
+      )
     elsif note.persisted?
       Banzai::NoteRenderer.render([note], @project, current_user)
 
-      attrs = {
+      attrs.merge!(
         valid: true,
         discussion_id: note.discussion_id,
         html: note_html(note),
         note: note.note
-      }
+      )
 
       if note.diff_note?
         discussion = note.to_discussion
@@ -186,16 +192,12 @@ class Projects::NotesController < Projects::ApplicationController
         end
       end
     else
-      attrs = {
+      attrs.merge!(
         valid: false,
         errors: note.errors
-      }
+      )
     end
 
-    attrs[:award] ||= false
-    attrs[:id] = note.id
-    attrs[:commands_changes] = note.commands_changes
-
     attrs
   end
 

app/models/concerns/time_trackable.rb

@@ -13,23 +13,43 @@ module TimeTrackable
 
     alias_method :time_spent?, :time_spent
 
+    default_value_for :time_estimate, value: 0, allows_nil: false
+
     has_many :timelogs, as: :trackable, dependent: :destroy
   end
 
-  def spend_time=(seconds:, user:)
-    # Exit if time to subtract exceeds the total time spent.
-    return if seconds < 0 && (seconds.abs > total_time_spent)
+  def spend_time(seconds, user)
+    return if seconds == 0
 
-    # When seconds = 0 we reset the total time spent by creating a new Timelog
-    # record with a negative value that is equal to the current total time spent.
-    new_time_spent = seconds.zero? ? (total_time_spent * -1) : seconds
+    @time_spent = seconds
+    @time_spent_user = user
 
-    timelogs.new(user: user, time_spent: new_time_spent)
+    if seconds == :reset
+      reset_spent_time
+    else
+      add_or_susbtract_spent_time
+    end
+  end
 
-    @time_spent = seconds
+  def spend_time!(seconds, user)
+    spend_time(seconds, user)
+    save!
   end
 
   def total_time_spent
     timelogs.sum(:time_spent)
   end
+
+  private
+
+  def reset_spent_time
+    timelogs.new(time_spent: total_time_spent * -1, user: @time_spent_user)
+  end
+
+  def add_or_susbtract_spent_time
+    # Exit if time to subtract exceeds the total time spent.
+    return if time_spent < 0 && (time_spent.abs > total_time_spent)
+
+    timelogs.new(time_spent: time_spent, user: @time_spent_user)
+  end
 end

app/services/issuable_base_service.rb

@@ -145,6 +145,7 @@ class IssuableBaseService < BaseService
   def create(issuable)
     merge_slash_commands_into_params!(issuable)
     filter_params
+    change_time_spent(issuable)
 
     params.delete(:state_event)
     params[:author] ||= current_user
@@ -185,6 +186,7 @@ class IssuableBaseService < BaseService
     change_state(issuable)
     change_subscription(issuable)
     change_todo(issuable)
+    change_time_spent(issuable)
     filter_params
     old_labels = issuable.labels.to_a
     old_mentioned_users = issuable.mentioned_users.to_a
@@ -236,6 +238,12 @@ class IssuableBaseService < BaseService
     end
   end
 
+  def change_time_spent(issuable)
+    if params[:spend_time]
+      issuable.spend_time(params.delete(:spend_time), current_user)
+    end
+  end
+
   def has_changes?(issuable, old_labels: [])
     valid_attrs = [:title, :description, :assignee_id, :milestone_id, :target_branch]
 

app/services/slash_commands/interpret_service.rb

@@ -254,10 +254,10 @@ module SlashCommands
       current_user.can?(:"admin_#{issuable.to_ability_name}", project)
     end
     command :estimate do |raw_duration|
-      time_spent = ChronicDuration.parse(raw_duration, default_unit: 'hours') rescue nil
+      time_estimate = ChronicDuration.parse(raw_duration, default_unit: 'hours') rescue nil
 
-      if time_spent
-        @updates[:time_estimate] = time_spent
+      if time_estimate
+        @updates[:time_estimate] = time_estimate
       end
     end
 
@@ -269,31 +269,29 @@ 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
-        @updates[:spend_time] = {
-          seconds: reduce_time ? (time_spent * -1) : time_spent,
-          user: current_user
-        }
+        @updates[:spend_time] = time_spent
       end
     end
 
-    desc 'Remove the estimated time'
+    desc 'Remove time estimate'
     condition do
       issuable.persisted? &&
         current_user.can?(:"admin_#{issuable.to_ability_name}", project)
     end
-    command :remove_estimation do
+    command :remove_estimate do
       @updates[:time_estimate] = 0
     end
 
-    desc 'Remove the time spent'
+    desc 'Remove spent time'
     condition do
       issuable.persisted? &&
         current_user.can?(:"admin_#{issuable.to_ability_name}", project)
     end
     command :remove_time_spent do
-      @updates[:spend_time] = { seconds: 0, user: current_user }
+      @updates[:spend_time] = :reset
     end
 
     def find_label_ids(labels_param)

app/services/system_note_service.rb

@@ -151,7 +151,7 @@ module SystemNoteService
   def change_time_spent(noteable, project, author)
     time_spent = noteable.time_spent
 
-    if time_spent.zero?
+    if time_spent == :reset
       body = "Removed time spent on this #{noteable.human_class_name}"
     else
       parsed_time = ChronicDuration.output(time_spent.abs, format: :short)

db/migrate/20161030005533_add_estimate_to_issuables.rb

@@ -24,8 +24,8 @@ class AddEstimateToIssuables < ActiveRecord::Migration
   # disable_ddl_transaction!
 
   def up
-    add_column_with_default :issues, :time_estimate, :integer, default: 0
-    add_column_with_default :merge_requests, :time_estimate, :integer, default: 0
+    add_column :issues, :time_estimate, :integer
+    add_column :merge_requests, :time_estimate, :integer
   end
 
   def down

db/schema.rb

@@ -560,7 +560,7 @@ ActiveRecord::Schema.define(version: 20161106185620) do
     t.integer "lock_version"
     t.text "title_html"
     t.text "description_html"
-    t.integer "time_estimate", default: 0
+    t.integer "time_estimate"
   end
 
   add_index "issues", ["assignee_id"], name: "index_issues_on_assignee_id", using: :btree
@@ -757,7 +757,7 @@ ActiveRecord::Schema.define(version: 20161106185620) do
     t.integer "lock_version"
     t.text "title_html"
     t.text "description_html"
-    t.integer "time_estimate", default: 0
+    t.integer "time_estimate"
   end
 
   add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree
@@ -822,9 +822,9 @@ ActiveRecord::Schema.define(version: 20161106185620) do
     t.datetime "ldap_sync_last_successful_update_at"
     t.datetime "ldap_sync_last_sync_at"
     t.datetime "deleted_at"
-    t.text "description_html"
     t.boolean "lfs_enabled"
     t.integer "repository_size_limit"
+    t.text "description_html"
   end
 
   add_index "namespaces", ["created_at"], name: "index_namespaces_on_created_at", using: :btree
@@ -1007,6 +1007,7 @@ ActiveRecord::Schema.define(version: 20161106185620) do
     t.datetime "created_at"
     t.datetime "updated_at"
     t.integer "creator_id"
+    t.boolean "wall_enabled", default: true, null: false
     t.integer "namespace_id"
     t.datetime "last_activity_at"
     t.string "import_url"
@@ -1049,8 +1050,8 @@ ActiveRecord::Schema.define(version: 20161106185620) do
     t.boolean "has_external_wiki"
     t.boolean "repository_read_only"
     t.boolean "lfs_enabled"
-    t.text "description_html"
     t.integer "repository_size_limit"
+    t.text "description_html"
     t.boolean "only_allow_merge_if_all_discussions_are_resolved", default: false, null: false
   end
 

doc/user/project/slash_commands.md

@@ -30,6 +30,6 @@ do.
 | `/remove_due_date`         | Remove due date |
 | `/wip`                     | Toggle the Work In Progress status |
 | <code>/estimate &lt;1w 3d 2h 14m&gt;</code> | Set time estimate |
-| `/remove_estimation`       | Remove estimated time |
+| `/remove_estimate`       | Remove estimated time |
 | <code>/spend &lt;1h 30m &#124; -1h 5m&gt;</code> | Add or substract spent time |
 | `/remove_time_spent`       | Remove time spent |

spec/controllers/projects/issues_controller_spec.rb

@@ -272,6 +272,20 @@ describe Projects::IssuesController do
   end
 
   describe 'POST #create' do
+    def post_new_issue(attrs = {})
+      sign_in(user)
+      project = create(:empty_project, :public)
+      project.team << [user, :developer]
+
+      post :create, {
+        namespace_id: project.namespace.to_param,
+        project_id: project.to_param,
+        issue: { title: 'Title', description: 'Description' }.merge(attrs)
+      }
+
+      project.issues.first
+    end
+
     context 'Akismet is enabled' do
       before do
         allow_any_instance_of(SpamService).to receive(:check_for_spam?).and_return(true)
@@ -279,13 +293,7 @@ describe Projects::IssuesController do
       end
 
       def post_spam_issue
-        sign_in(user)
-        spam_project = create(:empty_project, :public)
-        post :create, {
-          namespace_id: spam_project.namespace.to_param,
-          project_id: spam_project.to_param,
-          issue: { title: 'Spam Title', description: 'Spam lives here' }
-        }
+        post_new_issue(title: 'Spam Title', description: 'Spam lives here')
       end
 
       it 'rejects an issue recognized as spam' do
@@ -306,18 +314,26 @@ describe Projects::IssuesController do
         request.env['action_dispatch.remote_ip'] = '127.0.0.1'
       end
 
-      def post_new_issue
+      it 'creates a user agent detail' do
+        expect{ post_new_issue }.to change(UserAgentDetail, :count).by(1)
+      end
+    end
+
+    context 'when description has slash commands' do
+      before do
         sign_in(user)
-        project = create(:empty_project, :public)
-        post :create, {
-          namespace_id: project.namespace.to_param,
-          project_id: project.to_param,
-          issue: { title: 'Title', description: 'Description' }
-        }
       end
 
-      it 'creates a user agent detail' do
-        expect{ post_new_issue }.to change(UserAgentDetail, :count).by(1)
+      it 'can add spent time' do
+        issue = post_new_issue(description: '/spend 1h')
+
+        expect(issue.total_time_spent).to eq(3600)
+      end
+
+      it 'can set the time estimate' do
+        issue = post_new_issue(description: '/estimate 2h')
+
+        expect(issue.time_estimate).to eq(7200)
       end
     end
   end

spec/models/concerns/issuable_spec.rb

@@ -391,7 +391,7 @@ describe Issue, "Issuable" do
 
     context 'adding time' do
       it 'should update the total time spent' do
-        issue.update_attributes!(spend_time: { seconds: 1800, user: user })
+        issue.spend_time!(1800, user)
 
         expect(issue.total_time_spent).to eq(1800)
       end
@@ -399,18 +399,18 @@ describe Issue, "Issuable" do
 
     context 'substracting time' do
       before do
-        issue.update_attributes!(spend_time: { seconds: 1800, user: user })
+        issue.spend_time!(1800, user)
       end
 
       it 'should update the total time spent' do
-        issue.update_attributes!(spend_time: { seconds: -900, user: user })
+        issue.spend_time!(-900, user)
 
         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.update_attributes!(spend_time: { seconds: -3600, user: user })
+          issue.spend_time!(-3600, user)
 
           expect(issue.total_time_spent).to eq(1800)
         end

spec/services/slash_commands/interpret_service_spec.rb

@@ -222,7 +222,7 @@ describe SlashCommands::InterpretService, services: true do
       it 'populates spend_time: { seconds: 3600, user: user } if content contains /spend 1h' do
         _, updates = service.execute(content, issuable)
 
-        expect(updates).to eq(spend_time: { seconds: 3600, user: developer })
+        expect(updates).to eq(spend_time: 3600)
       end
     end
 
@@ -230,12 +230,12 @@ describe SlashCommands::InterpretService, services: true do
       it 'populates spend_time: { seconds: -1800, user: user } if content contains /spend -30m' do
         _, updates = service.execute(content, issuable)
 
-        expect(updates).to eq(spend_time: { seconds: -1800, user: developer })
+        expect(updates).to eq(spend_time: -1800)
       end
     end
 
-    shared_examples 'remove_estimation command' do
-      it 'populates time_estimate: "0" if content contains /remove_estimation' do
+    shared_examples 'remove_estimate command' 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,10 +243,10 @@ describe SlashCommands::InterpretService, services: true do
     end
 
     shared_examples 'remove_time_spent command' do
-      it 'populates spend_time: "0" 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: { seconds: 0, user: developer })
+        expect(updates).to eq(spend_time: :reset)
       end
     end
 
@@ -526,8 +526,8 @@ describe SlashCommands::InterpretService, services: true do
       let(:issuable) { issue }
     end
 
-    it_behaves_like 'remove_estimation command' do
-      let(:content) { '/remove_estimation' }
+    it_behaves_like 'remove_estimate command' do
+      let(:content) { '/remove_estimate' }
       let(:issuable) { issue }
     end