Change NotificationSettings duration units to days

Days are easier to work with than hours when building SQL requests.

Also change "everyday" value for overdue from 0 to 1, for consistency
with the other values.

app/contracts/user_preferences/params_contract.rb

@@ -28,8 +28,8 @@
 
 module UserPreferences
   class ParamsContract < ::ParamsContract
-    DATE_ALERT_DURATIONS = [nil, 0, 24, 72, 168].freeze
-    DATE_ALERT_OVERDUE_DURATIONS = [nil, 0, 72, 168].freeze
+    DATE_ALERT_DURATIONS = [nil, 0, 1, 3, 7].freeze
+    DATE_ALERT_OVERDUE_DURATIONS = [nil, 1, 3, 7].freeze
 
     validate :only_one_global_setting,
              if: -> { notifications.present? }

db/migrate/20221028070534_change_notification_settings_start_date_due_date_and_overdue_duration_unit_to_days.rb

@@ -0,0 +1,40 @@
+class ChangeNotificationSettingsStartDateDueDateAndOverdueDurationUnitToDays < ActiveRecord::Migration[7.0]
+  def change
+    change_table :notification_settings, bulk: true do |t|
+      t.change_default :start_date, from: 24, to: 1
+      t.change_default :due_date, from: 24, to: 1
+    end
+
+    reversible do |dir|
+      dir.up do
+        update_durations_from_hours_to_days
+      end
+
+      dir.down do
+        update_durations_from_days_to_hours
+      end
+    end
+  end
+
+  def update_durations_from_hours_to_days
+    execute <<~SQL.squish
+      UPDATE
+        notification_settings
+      SET
+        start_date = CASE WHEN start_date IS NOT NULL THEN start_date / 24 END,
+        due_date = CASE WHEN due_date IS NOT NULL THEN due_date / 24 END,
+        overdue = CASE WHEN overdue IS NOT NULL THEN overdue / 24 END
+    SQL
+  end
+
+  def update_durations_from_days_to_hours
+    execute <<~SQL.squish
+      UPDATE
+        notification_settings
+      SET
+        start_date = CASE WHEN start_date IS NOT NULL THEN start_date * 24 END,
+        due_date = CASE WHEN due_date IS NOT NULL THEN due_date * 24 END,
+        overdue = CASE WHEN overdue IS NOT NULL THEN overdue * 24 END
+    SQL
+  end
+end

lib/api/decorators/date_property.rb

@@ -107,18 +107,18 @@ module API
 
         def default_duration_getter(name)
           ->(represented:, decorator:, **) {
-            decorator.datetime_formatter.format_duration_from_hours(represented.send(name), allow_nil: true)
+            decorator.datetime_formatter.format_duration_from_days(represented.send(name), allow_nil: true)
           }
         end
 
         def default_duration_setter(name)
           ->(fragment:, decorator:, **) {
-            hours = decorator
-                    .datetime_formatter
-                    .parse_duration_to_hours(fragment,
-                                             name.to_s.camelize(:lower),
-                                             allow_nil: true)
-            send(:"#{name}=", hours)
+            days = decorator
+                   .datetime_formatter
+                   .parse_duration_to_days(fragment,
+                                           name.to_s.camelize(:lower),
+                                           allow_nil: true)
+            send(:"#{name}=", days)
           }
         end
       end

lib/api/v3/utilities/date_time_formatter.rb

@@ -101,6 +101,12 @@ module API
           end
         end
 
+        def format_duration_from_days(days, allow_nil: false)
+          return nil if days.nil? && allow_nil
+
+          Duration.new(seconds: days * 3600 * 24).iso8601
+        end
+
         def parse_duration_to_days(duration, property_name, allow_nil: false)
           return nil if duration.nil? && allow_nil
 

spec/contracts/user_preferences/params_contract_spec.rb

@@ -89,7 +89,7 @@ describe UserPreferences::ParamsContract do
     context 'when project setting with start_date, due_date and overdue set' do
       let(:notification_settings) do
         [
-          { project_id: 1234, start_date: 24, due_date: 24, overdue: 0 }
+          { project_id: 1234, start_date: 1, due_date: 1, overdue: 1 }
         ]
       end
 
@@ -99,27 +99,27 @@ describe UserPreferences::ParamsContract do
     context 'when global setting with start_date, due_date and overdue set' do
       let(:notification_settings) do
         [
-          { start_date: 24, due_date: 24, overdue: 0 }
+          { start_date: 1, due_date: 1, overdue: 1 }
         ]
       end
 
       it_behaves_like 'contract is valid'
     end
 
-    context 'when project setting with due_date and start_date with overdue are invalid' do
+    context 'when project setting with valid start_date, valid due_date and invalid overdue' do
       let(:notification_settings) do
         [
-          { project_id: 1234, start_date: 22, due_date: 24, overdue: 24 }
+          { project_id: 1234, start_date: 1, due_date: 1, overdue: 0 }
         ]
       end
 
       it_behaves_like 'contract is invalid', notification_settings: :wrong_date
     end
 
-    context 'when global setting with due_date and start_date with overdue are invalid' do
+    context 'when global setting with invalid start_date, valid due_date and valid overdue' do
       let(:notification_settings) do
         [
-          { start_date: 22, due_date: 24, overdue: 24 }
+          { start_date: -1, due_date: 1, overdue: 1 }
         ]
       end
 
@@ -129,7 +129,7 @@ describe UserPreferences::ParamsContract do
     context 'when project setting with start_date, due_date and overdue missing' do
       let(:notification_settings) do
         [
-          { project_id: 1234, start_date: 24, due_date: 24 }
+          { project_id: 1234, start_date: 1, due_date: 1 }
         ]
       end
 
@@ -139,27 +139,27 @@ describe UserPreferences::ParamsContract do
     context 'when global setting with start_date, due_date and overdue missing' do
       let(:notification_settings) do
         [
-          { start_date: 24, due_date: 24 }
+          { start_date: 1, due_date: 1 }
         ]
       end
 
       it_behaves_like 'contract is valid'
     end
 
-    context 'when project setting with start_date, due_date invalid and overdue missing' do
+    context 'when project setting with valid start_date, invalid due_date and overdue missing' do
       let(:notification_settings) do
         [
-          { project_id: 1234, start_date: 24, due_date: 22 }
+          { project_id: 1234, start_date: 1, due_date: 24 }
         ]
       end
 
       it_behaves_like 'contract is invalid', notification_settings: :wrong_date
     end
 
-    context 'when global setting with start_date, due_date invalid and overdue missing' do
+    context 'when global setting with invalid start_date, valid due_date and overdue missing' do
       let(:notification_settings) do
         [
-          { start_date: 24, due_date: 22 }
+          { start_date: 24, due_date: 1 }
         ]
       end
 

spec/lib/api/v3/support/date_time_examples.rb

@@ -61,20 +61,3 @@ shared_examples_for 'has UTC ISO 8601 date and time' do
       .at_least(:once)
   end
 end
-
-shared_examples_for 'has ISO 8601 duration' do
-  it 'exists' do
-    expect(subject).to have_json_path(json_path)
-  end
-
-  it 'indicates duration as ISO 8601' do
-    allow(::API::V3::Utilities::DateTimeFormatter).to receive(:format_duration_from_hours)
-
-    subject
-
-    expect(::API::V3::Utilities::DateTimeFormatter)
-      .to have_received(:format_duration_from_hours)
-      .with(duration, anything)
-      .at_least(:once)
-  end
-end

spec/lib/api/v3/support/property_examples.rb

@@ -55,10 +55,3 @@ shared_examples_for 'datetime property' do |name|
     let(:date) { value }
   end
 end
-
-shared_examples_for 'duration property' do |name|
-  it_behaves_like 'has ISO 8601 duration' do
-    let(:json_path) { name.to_s }
-    let(:duration) { value }
-  end
-end

spec/lib/api/v3/user_preferences/notification_setting_representer_rendering_spec.rb

@@ -34,7 +34,7 @@ describe ::API::V3::UserPreferences::NotificationSettingRepresenter, 'rendering'
   subject(:generated) { representer.to_json }
 
   let(:project) { build_stubbed :project }
-  let(:notification_setting) { build_stubbed(:notification_setting, overdue: 0, project:) }
+  let(:notification_setting) { build_stubbed(:notification_setting, overdue: 3, project:) }
 
   let(:representer) do
     described_class.create notification_setting,
@@ -69,17 +69,23 @@ describe ::API::V3::UserPreferences::NotificationSettingRepresenter, 'rendering'
         .not_to have_json_path('_type')
     end
 
-    NotificationSetting.all_settings.each do |property|
-      if property.in?(NotificationSetting.duration_settings)
-        it_behaves_like 'duration property', property.to_s.camelize(:lower) do
-          let(:value) { notification_setting.send property }
-        end
-      else
-        it_behaves_like 'property', property.to_s.camelize(:lower) do
-          let(:value) { notification_setting.send property }
+    (NotificationSetting.all_settings - NotificationSetting.duration_settings).each do |property|
+      it_behaves_like 'property', property.to_s.camelize(:lower) do
+        let(:value) do
+          notification_setting.send property
         end
       end
     end
+
+    it_behaves_like 'property', :startDate do
+      let(:value) { 'P1D' }
+    end
+    it_behaves_like 'property', :dueDate do
+      let(:value) { 'P1D' }
+    end
+    it_behaves_like 'property', :overdue do
+      let(:value) { 'P3D' }
+    end
   end
 
   describe '_embedded' do
@@ -90,4 +96,16 @@ describe ::API::V3::UserPreferences::NotificationSettingRepresenter, 'rendering'
       end
     end
   end
+
+  context 'when duration settings are all nil' do
+    let(:notification_setting) do
+      build_stubbed(:notification_setting, project:, start_date: nil, due_date: nil, overdue: nil)
+    end
+
+    it 'does not represent them in the resulting json' do
+      expect(subject).not_to have_json_path("startDate")
+      expect(subject).not_to have_json_path("dueDate")
+      expect(subject).not_to have_json_path("overdue")
+    end
+  end
 end

spec/lib/api/v3/user_preferences/user_preference_representer_rendering_spec.rb

@@ -41,7 +41,7 @@ describe ::API::V3::UserPreferences::UserPreferenceRepresenter,
                     }
                   })
   end
-  let(:notification_setting) { build(:notification_setting, start_date: 72, due_date: 72, overdue: 72) }
+  let(:notification_setting) { build(:notification_setting, start_date: 3, due_date: 3, overdue: 3) }
   let(:user) { build_stubbed(:user, preference:) }
   let(:representer) { described_class.new(preference, current_user: user) }
 

spec/lib/api/v3/utilities/date_time_formatter_spec.rb

@@ -215,6 +215,29 @@ describe ::API::V3::Utilities::DateTimeFormatter do
     end
   end
 
+  describe 'format_duration_from_days' do
+    it 'formats floats' do
+      expect(subject.format_duration_from_days(5.0)).to eq('P5D')
+    end
+
+    it 'formats fractional floats' do
+      expect(subject.format_duration_from_days(5.5)).to eq('P5DT12H')
+    end
+
+    it 'includes minutes and seconds' do
+      expect(subject.format_duration_from_days(5.501)).to eq('P5DT12H1M26S')
+    end
+
+    it 'formats ints' do
+      expect(subject.format_duration_from_days(5)).to eq('P5D')
+    end
+
+    it_behaves_like 'can format nil' do
+      let(:method) { :format_duration_from_days }
+      let(:input) { 5 }
+    end
+  end
+
   describe 'parse_duration_to_days' do
     it 'parses ISO 8601 durations of full days' do
       expect(subject.parse_duration_to_days('P5D', 'prop')).to eq(5)

spec/services/user_preferences/update_service_integration_spec.rb

@@ -120,9 +120,9 @@ describe UserPreferences::UpdateService, 'integration', type: :model do
         expect(subject.count).to eq 1
         expect(subject.first.project_id).to eq project.id
 
-        default_settings = {
-          NotificationSetting::START_DATE => 24,
-          NotificationSetting::DUE_DATE => 24,
+        expected_default_settings = {
+          NotificationSetting::START_DATE => 1,
+          NotificationSetting::DUE_DATE => 1,
           NotificationSetting::OVERDUE => nil,
           NotificationSetting::ASSIGNEE => true,
           NotificationSetting::RESPONSIBLE => true,
@@ -132,11 +132,11 @@ describe UserPreferences::UpdateService, 'integration', type: :model do
         NotificationSetting.all_settings.each do |key|
           val = subject.first.send key
 
-          if key.in?(default_settings)
-            expect(val).to eq(default_settings[key])
+          if key.in?(expected_default_settings)
+            expect(key => val).to eq(key => expected_default_settings[key])
           else
             # Settings with default value false
-            expect(val).to be(false)
+            expect(key => val).to eq(key => false)
           end
         end