Handle blank issue inputs (#210)

* Refactor TimeRebalancer and TimerSessionsController to handle blank issue inputs and improve error handling

* remove redundant methods and LINT

* Replace assigns

* Refactor issue selection validation and update handling in timer sessions

* LINT & replace `it` due to linter complaining

* pass blank as an arg

Author: snitzelbread

app/controllers/timer_sessions_controller.rb

@@ -3,8 +3,7 @@
 class TimerSessionsController < TrackyController
   def index
     @timer_sessions_in_range = TimerSession.includes(:time_entries, :timer_session_time_entries, issues: :project)
-                                           .finished
-                                           .created_by(User.current)
+                                           .finished.created_by(User.current)
     @non_matching_timer_session_ids = TimeDiscrepancyLoader.uneven_timer_session_ids(@timer_sessions_in_range)
     set_timer_sessions
     @timer_offset = offset_for_time_zone
@@ -25,10 +24,7 @@ def time_entries_in_range(timer_sessions)
 
   def rebalance
     @timer_session = user_scoped_timer_session(params[:id])
-    TimeRebalancer.new(
-      @timer_session.relevant_issues.map(&:id),
-      @timer_session
-    ).force_rebalance
+    TimeRebalancer.new(@timer_session.relevant_issues.map(&:id), @timer_session).force_rebalance
     flash[:notice] = l(:notice_successful_update)
     redirect_to timer_sessions_path
   end
@@ -55,20 +51,20 @@ def continue
     timer_session_template = user_scoped_timer_session(params[:id])
     linked_issues = timer_session_template.relevant_issues
     new_timer_session = timer_session_template.dup
-    new_timer_session.update(timer_end: nil,
-                             finished: false,
+    new_timer_session.update(timer_end: nil, finished: false,
                              timer_start: (User.current.time_zone || Time.zone).now.asctime)
     IssueConnector.new(linked_issues.map(&:id) || [], new_timer_session).run
     redirect_to timer_sessions_path
   end
 
   def update
     @timer_session = user_scoped_timer_session(params[:id])
-    if @timer_session.update(timer_session_params)
-      TimeRebalancer.new(timer_session_params[:issue_ids],
-                         @timer_session).rebalance_entries
+
+    if @timer_session.update(timer_session_update_params)
+      TimeRebalancer.new(selected_issue_ids || @timer_session.relevant_issues.map(&:id), @timer_session)
+                    .rebalance_entries
       flash[:notice] = l(:notice_successful_update)
-      render_js(@timer_session.valid? ? :update_redirect : :update)
+      render_js(:update_redirect)
     else
       render_js :update
     end
@@ -95,11 +91,18 @@ def user_scoped_timer_session(id)
     TimerSession.where(user: User.current).find(id)
   end
 
+  def selected_issue_ids
+    return timer_session_params[:issue_ids].reject(&:blank?) if timer_session_params.key?(:issue_ids)
+
+    Array.wrap(timer_session_params[:issue_id]).reject(&:blank?) if timer_session_params.key?(:issue_id)
+  end
+
   def timer_session_params
-    params.require(:timer_session).permit(:comments,
-                                          :timer_start,
-                                          :timer_end,
-                                          issue_ids: [])
+    params.require(:timer_session).permit(:comments, :timer_start, :timer_end, :issue_id, issue_ids: [])
+  end
+
+  def timer_session_update_params
+    timer_session_params.except(:issue_id, :issue_ids)
   end
 
   def report_query_params

app/models/finished_timer_session_validator.rb

@@ -30,7 +30,8 @@ def validate_comment_present
   end
 
   def validate_issues_selected
-    @record.errors.add(:issue_id, :no_selection) if @record.issues.none?
+    @record.errors.add(:issue_id, :no_selection) if @record.issues.blank?
+    @record.errors.add(:issue_id, :no_selection) if @record.issues.any?(&:blank?)
   end
 
   def validate_minimal_duration

app/services/time_rebalancer.rb

@@ -2,13 +2,14 @@
 
 class TimeRebalancer
   def initialize(issues, timer_session)
-    @issues = (issues || []).map(&:to_i)
+    @issues = Array.wrap(issues).reject(&:blank?).map(&:to_i).uniq
     @issue_ids = timer_session.relevant_issues.map(&:id).map(&:to_i)
     @timer_session = timer_session
     validate
   end
 
   def rebalance_entries
+    return if @issues.blank?
     return unless @timer_session.valid?
 
     if issues_changed?
@@ -28,6 +29,7 @@ def force_rebalance
 
   def validate
     @timer_session.errors.add(:issue_id, :no_selection) if @issues.blank?
+    @timer_session.errors.add(:issue_id, :no_selection) if @issues.any?(&:blank?)
   end
 
   def handle_issues_changed

app/views/timer_sessions/update.js.erb

@@ -1,4 +1,4 @@
-<% @timer_session.validate %>
+<% @timer_session.validate if @timer_session.errors.empty? %>
 $('#ajax-modal').html(
   ("<%= j render 'edit_modal', timer_session: @timer_session %>")
 );

test/functional/timer_sessions_controller_test.rb

@@ -19,10 +19,15 @@ class TimerSessionsControllerTest < ActionController::TestCase
 
   def setup
     @issue = Issue.find(1)
+    @other_issue = Issue.find(2)
     @time_entry = TimeEntry.find(1)
     @user = User.find(1)
     @timer_session = FactoryBot.create(:timer_session,
                                        user: @user)
+    TimerSessionIssue.create!(
+      issue: @issue,
+      timer_session_id: @timer_session.id
+    )
     TimerSessionTimeEntry.create!(
       time_entry: @time_entry,
       timer_session_id: @timer_session.id
@@ -71,6 +76,58 @@ def setup
     assert @timer_session.comments, 'NEW IPA TOPIC'
   end
 
+  test 'update accepts singular issue_id param' do
+    put(:update, params: {
+          id: @timer_session.id,
+          timer_session: { comments: 'NEW IPA TOPIC', issue_id: @other_issue.id }
+        }, xhr: true)
+
+    timer_session = @controller.instance_variable_get(:@timer_session)
+
+    assert_response 200
+    assert_empty timer_session.errors[:issue_id]
+    assert_equal [@other_issue.id], @timer_session.reload.issue_ids
+  end
+
+  test 'update with empty string in issue array selection does not continue' do
+    original_issue_ids = @timer_session.issue_ids
+
+    put(:update, params: {
+          id: @timer_session.id,
+          timer_session: { comments: 'NEW IPA TOPIC', issue_ids: [''] }
+        }, xhr: true)
+
+    timer_session = @controller.instance_variable_get(:@timer_session)
+
+    assert_response 200
+    assert_includes timer_session.errors[:issue_id],
+                    I18n.t('activerecord.errors.models.timer_session.attributes.issue_id.no_selection', locale: :en)
+    assert_equal original_issue_ids, @timer_session.reload.issue_ids
+  end
+
+  test 'update with an empty array as issue selection does not continue' do
+    original_issue_ids = @timer_session.issue_ids
+
+    put :update,
+        params: { id: @timer_session.id },
+        body: {
+          timer_session: {
+            comments: 'NEW IPA TOPIC',
+            issue_ids: []
+          }
+        }.to_json,
+        as: :json,
+        format: :js,
+        xhr: true
+
+    timer_session = @controller.instance_variable_get(:@timer_session)
+
+    assert_response 200
+    assert_includes timer_session.errors[:issue_id],
+                    I18n.t('activerecord.errors.models.timer_session.attributes.issue_id.no_selection', locale: :en)
+    assert_equal original_issue_ids, @timer_session.reload.issue_ids
+  end
+
   test 'continue' do
     start_timer_sessions_count = TimerSession.all.count
     post(:continue, params: { id: @timer_session.id })

test/unit/time_rebalancer_test.rb

@@ -87,4 +87,14 @@ class TimeRebalancerTest < ActiveSupport::TestCase
     assert_equal 1, TimerSessionIssue.count
     assert_equal issue_id, @timer_session.issues.first.id
   end
+
+  test '#rebalance_entries - blank issue input adds no_selection error' do
+    original_issue_ids = @timer_session.issue_ids
+
+    TimeRebalancer.new([''], @timer_session).rebalance_entries
+
+    assert_includes @timer_session.errors[:issue_id],
+                    I18n.t('activerecord.errors.models.timer_session.attributes.issue_id.no_selection', locale: :en)
+    assert_equal original_issue_ids, @timer_session.reload.issue_ids
+  end
 end