Address review: range validation, strict ISO offsets, gating and guard fixes

- Abort issue save with an error when the computed due time would be
  before the start time (previously the sidecar save failed silently)
- persist with save! so unexpected sidecar failures surface loudly
- Require an explicit Z/offset in API timestamps
- Skip sync work entirely for date-only edits on issues without times
- Gate the issue-view hook by tracker enablement like the form hook
- Explicit returns after render_403 in controller filters

Author: dkastl

app/controllers/issue_datetimes_controller.rb

@@ -76,13 +76,13 @@ def index
 
   def find_issue
     @issue = Issue.find(params[:issue_id])
-    render_403 unless @issue.visible?
+    return render_403 unless @issue.visible?
   rescue ActiveRecord::RecordNotFound
     render_404
   end
 
   def require_edit_permission
-    render_403 unless @issue.editable?
+    return render_403 unless @issue.editable?
   end
 
   def find_project
@@ -91,8 +91,13 @@ def find_project
     render_404
   end
 
+  # Requires an explicit offset (Z or +hh:mm) so API writes are
+  # unambiguous regardless of the server's local time zone.
   def parse_timestamp(value)
-    Time.iso8601(value.to_s)
+    text = value.to_s
+    return nil unless /(Z|[+-]\d{2}:?\d{2})\z/i.match?(text)
+
+    Time.iso8601(text)
   rescue ArgumentError
     nil
   end

app/views/hooks/_issue_datetime_show.html.erb

@@ -1,4 +1,4 @@
-<% record = issue && issue.issue_datetime %>
+<% record = issue && RedmineIssueDatetime.enabled_for?(issue.tracker_id) ? issue.issue_datetime : nil %>
 <% if record && !record.blank_times? %>
 <div class="issue-datetime">
   <span class="issue-datetime-start">

lib/redmine_issue_datetime/issue_extension.rb

@@ -41,15 +41,22 @@ def prepare_issue_datetime_sync
       return true unless RedmineIssueDatetime.enabled_for?(tracker_id)
 
       record = issue_datetime
+      return true if record.nil? && @start_time_input.blank? && @due_time_input.blank?
+
       touched = !@start_time_input.nil? || !@due_time_input.nil? ||
                 will_save_change_to_start_date? || will_save_change_to_due_date?
-      return true unless touched || record
+      return true unless touched
 
       old_starts = record&.starts_at
       old_ends = record&.ends_at
       new_starts = RedmineIssueDatetime.combine(start_date, @start_time_input, old_starts)
       new_ends = RedmineIssueDatetime.combine(due_date, @due_time_input, old_ends)
 
+      if new_starts && new_ends && new_ends < new_starts
+        errors.add(:due_date, :greater_than_start_date)
+        throw :abort
+      end
+
       journalize_issue_datetime('start_time', old_starts, new_starts)
       journalize_issue_datetime('due_time', old_ends, new_ends)
 
@@ -70,7 +77,7 @@ def persist_issue_datetime
         record.destroy unless record.new_record?
         association(:issue_datetime).reset
       elsif record.changed?
-        record.save
+        record.save!
       end
     end
 

test/integration/issue_datetimes_api_test.rb

@@ -49,6 +49,14 @@ def setup
     assert_equal Time.utc(2026, 8, 3, 17, 0), @issue.issue_datetime.ends_at
   end
 
+  test 'PUT rejects timestamps without an explicit offset' do
+    put "/issues/#{@issue.id}/datetime.json",
+        params: {starts_at: '2026-08-03T09:15:00'}.to_json,
+        headers: {'Content-Type' => 'application/json'}.merge(credentials('jsmith'))
+
+    assert_response :unprocessable_entity
+  end
+
   test 'PUT rejects malformed timestamps' do
     put "/issues/#{@issue.id}/datetime.json",
         params: {starts_at: 'tomorrow-ish'}.to_json,

test/unit/issue_datetime_sync_test.rb

@@ -95,6 +95,17 @@ def setup
     assert_equal Time.utc(2026, 8, 3, 9, 15), @issue.reload.issue_datetime.starts_at
   end
 
+  test 'due time before start time on the same date blocks the save' do
+    @issue.start_date = Date.new(2026, 8, 3)
+    @issue.due_date = Date.new(2026, 8, 3)
+    @issue.start_time = '10:00'
+    @issue.due_time = '09:00'
+
+    assert_not @issue.save
+    assert @issue.errors[:due_date].present?
+    assert_nil @issue.reload.issue_datetime
+  end
+
   test 'destroying the issue destroys the sidecar row' do
     @issue.update!(start_date: Date.new(2026, 8, 3), start_time: '09:15')
     record_id = @issue.reload.issue_datetime.id