Fix time period key conflicts (#1944)

app/controllers/pages_controller.rb

@@ -2,7 +2,7 @@ class PagesController < ApplicationController
   skip_before_action :authenticate_user!, only: %i[early_access]
 
   def dashboard
-    @period = Period.from_key(params[:period], fallback: true)
+    @period = params[:period] ? Period.from_key(params[:period]) : Period.last_30_days
     @balance_sheet = Current.family.balance_sheet
     @accounts = Current.family.accounts.active.with_attached_logo
 

app/models/budget.rb

@@ -54,7 +54,7 @@ class Budget < ApplicationRecord
   end
 
   def period
-    Period.new(start_date: start_date, end_date: end_date)
+    Period.custom(start_date: start_date, end_date: end_date)
   end
 
   def to_param

app/models/period.rb

@@ -1,9 +1,12 @@
 class Period
   include ActiveModel::Validations, Comparable
 
-  attr_reader :start_date, :end_date
+  class InvalidKeyError < StandardError; end
 
-  validates :start_date, :end_date, presence: true
+  attr_reader :key, :start_date, :end_date
+
+  validates :start_date, :end_date, presence: true, if: -> { PERIODS[key].nil? }
+  validates :key, presence: true, if: -> { start_date.nil? || end_date.nil? }
   validate :must_be_valid_date_range
 
   PERIODS = {
@@ -64,18 +67,18 @@ class Period
   }
 
   class << self
-    def default
+    def from_key(key)
-      from_key("last_30_days")
+      unless PERIODS.key?(key)
+        raise InvalidKeyError, "Invalid period key: #{key}"
+      end
+
+      start_date, end_date = PERIODS[key].fetch(:date_range)
+
+      new(key: key, start_date: start_date, end_date: end_date)
     end
 
-    def from_key(key, fallback: false)
+    def custom(start_date:, end_date:)
-      if PERIODS[key].present?
+      new(start_date: start_date, end_date: end_date)
-        start_date, end_date = PERIODS[key].fetch(:date_range)
-        new(start_date: start_date, end_date: end_date)
-      else
-        return default if fallback
-        raise ArgumentError, "Invalid period key: #{key}"
-      end
     end
 
     def all
@@ -85,12 +88,12 @@ class Period
 
   PERIODS.each do |key, period|
     define_singleton_method(key) do
-      start_date, end_date = period.fetch(:date_range)
+      from_key(key)
-      new(start_date: start_date, end_date: end_date)
     end
   end
 
-  def initialize(start_date:, end_date:, date_format: "%b %d, %Y")
+  def initialize(start_date: nil, end_date: nil, key: nil, date_format: "%b %d, %Y")
+    @key = key
     @start_date = start_date
     @end_date = end_date
     @date_format = date_format
@@ -121,37 +124,33 @@ class Period
     end
   end
 
-  def key
-    PERIODS.find { |_, period| period.fetch(:date_range) == [ start_date, end_date ] }&.first
-  end
-
   def label
-    if known?
+    if key_metadata
-      PERIODS[key].fetch(:label)
+      key_metadata.fetch(:label)
     else
       "Custom Period"
     end
   end
 
   def label_short
-    if known?
+    if key_metadata
-      PERIODS[key].fetch(:label_short)
+      key_metadata.fetch(:label_short)
     else
-      "CP"
+      "Custom"
     end
   end
 
   def comparison_label
-    if known?
+    if key_metadata
-      PERIODS[key].fetch(:comparison_label)
+      key_metadata.fetch(:comparison_label)
     else
       "#{start_date.strftime(@date_format)} to #{end_date.strftime(@date_format)}"
     end
   end
 
   private
-    def known?
+    def key_metadata
-      key.present?
+      @key_metadata ||= PERIODS[key]
     end
 
     def must_be_valid_date_range

app/views/accounts/chart.html.erb

@@ -1,4 +1,4 @@
-<% period = Period.from_key(params[:period], fallback: true) %>
+<% period = params[:period] ? Period.from_key(params[:period]) : Period.last_30_days %>
 <% series = @account.balance_series(period: period) %>
 <% trend = series.trend %>
 

app/views/accounts/show/_chart.html.erb

@@ -1,6 +1,6 @@
 <%# locals: (account:, title: nil, tooltip: nil, **args) %>
 
-<% period = Period.from_key(params[:period], fallback: true) %>
+<% period = params[:period] ? Period.from_key(params[:period]) : Period.last_30_days %>
 <% default_value_title = account.asset? ? t(".balance") : t(".owed") %>
 
 <div id="<%= dom_id(account, :chart) %>" class="bg-white shadow-xs rounded-xl border border-alpha-black-25 rounded-lg space-y-2">

test/models/period_test.rb

@@ -18,20 +18,19 @@ class PeriodTest < ActiveSupport::TestCase
     assert_includes error.message, "Start date must be before end date"
   end
 
+  test "can create custom period" do
+    period = Period.new(start_date: Date.current - 15.days, end_date: Date.current)
+    assert_equal "Custom Period", period.label
+  end
+
   test "from_key returns period for valid key" do
     period = Period.from_key("last_30_days")
     assert_equal 30.days.ago.to_date, period.start_date
     assert_equal Date.current, period.end_date
   end
 
-  test "from_key with invalid key and fallback returns default period" do
-    period = Period.from_key("invalid_key", fallback: true)
-    assert_equal 30.days.ago.to_date, period.start_date
-    assert_equal Date.current, period.end_date
-  end
-
   test "from_key with invalid key and no fallback raises error" do
-    assert_raises ArgumentError do
+    error = assert_raises(Period::InvalidKeyError) do
       Period.from_key("invalid_key")
     end
   end