Refactor TimeSeries artifacts (#651)

* Reindent TimeSeries classes

* Fix spacing in time series tests

* Remove trend tests where current is nil

I think if we've gotten this far with a nil value for current, there's a data integrity problem.

If we allow this, we'll have to be very defensive in our code. Best to raise and fix early.

* Reindent Money class

* Refactor TimeSeries artifacts

* Use as_json in TimeSeries

* Bring back tests for trends where current is nil

* Bring back trend test

* Correctly enumerate trend test

* Use favorable_direction for trend_styles helper

* Make trend public in TimeSeries::Value

* Allow nil current values in trends

I think I might've gotten it wrong before, nils might appear in trends if values are unavailable for snapshots

* Clean up TimeSeries::Trend

* Skip trend values same class validations if any values are nil

* Refactor Money

* Remove object parsing in TimeSeries::Value

We're only every passing hashes

app/helpers/application_helper.rb

@@ -53,13 +53,13 @@ module ApplicationHelper
 
   def trend_styles(trend)
     fallback = { bg_class: "bg-gray-500/5", text_class: "text-gray-500", symbol: "", icon: "minus" }
-    return fallback if trend.nil? || trend.direction == "flat"
+    return fallback if trend.nil? || trend.direction.flat?
 
     bg_class, text_class, symbol, icon = case trend.direction
     when "up"
-      trend.type == "liability" ? [ "bg-red-500/5", "text-red-500", "+", "arrow-up" ] : [ "bg-green-500/5", "text-green-500", "+", "arrow-up" ]
+      trend.favorable_direction.down? ? [ "bg-red-500/5", "text-red-500", "+", "arrow-up" ] : [ "bg-green-500/5", "text-green-500", "+", "arrow-up" ]
     when "down"
-      trend.type == "liability" ? [ "bg-green-500/5", "text-green-500", "-", "arrow-down" ] : [ "bg-red-500/5", "text-red-500", "-", "arrow-down" ]
+      trend.favorable_direction.down? ? [ "bg-green-500/5", "text-green-500", "-", "arrow-down" ] : [ "bg-red-500/5", "text-red-500", "-", "arrow-down" ]
     when "flat"
       [ "bg-gray-500/5", "text-gray-500", "", "minus" ]
     else

app/models/time_series.rb

@@ -1,21 +1,21 @@
-
 class TimeSeries
-    attr_reader :type
+  DIRECTIONS = %w[ up down ].freeze
 
-    def self.from_collection(collection, value_method, options = {})
+  attr_reader :values, :favorable_direction
-        data = collection.map do |obj|
+
-            { date: obj.date, value: obj.public_send(value_method), original: obj }
+  def self.from_collection(collection, value_method)
-        end
+    collection.map do |obj|
-        new(data, options)
+      {
+        date: obj.date,
+        value: obj.public_send(value_method),
+        original: obj
+      }
+    end.then { |data| new(data) }
   end
 
-    def initialize(data, options = {})
+  def initialize(data, favorable_direction: "up")
-        @type = options[:type] || :normal
+    @favorable_direction = (favorable_direction.presence_in(DIRECTIONS) || "up").inquiry
-        initialize_series_data(data)
+    @values = initialize_values data
-    end
-
-    def values
-        @values ||= add_trends_to_series
   end
 
   def first
@@ -31,53 +31,27 @@ class TimeSeries
   end
 
   def trend
-        TimeSeries::Trend.new(
+    TimeSeries::Trend.new \
       current: last&.value,
       previous: first&.value,
-            type: @type
+      series: self
-        )
   end
 
-    # Data shape that frontend expects for D3 charts
+  # `as_json` returns the data shape used by D3 charts
-    def to_json(*_args)
+  def as_json
     {
-            values: values.map do |v|
+      values: values.map(&:as_json),
-                {
+      trend: trend.as_json,
-                    date: v.date,
+      favorable_direction: favorable_direction
-                    value: JSON.parse(v.value.to_json),
+    }.as_json
-                    trend: {
-                        type: v.trend.type,
-                        direction: v.trend.direction,
-                        value: JSON.parse(v.trend.value.to_json),
-                        percent: v.trend.percent
-                    }
-                }
-            end,
-            trend: {
-                type: @type,
-                direction: trend.direction,
-                value: JSON.parse(trend.value.to_json),
-                percent: trend.percent
-            },
-            type: @type
-        }.to_json
   end
 
   private
-        def initialize_series_data(data)
+    def initialize_values(data)
-            @series_data = data.nil? || data.empty? ? [] : data.map { |d| TimeSeries::Value.new(d) }.sort_by(&:date)
+      [ nil, *data ].each_cons(2).map do |previous, current|
-        end
+        TimeSeries::Value.new **current,
-
+          previous_value: previous.try(:[], :value),
-        def add_trends_to_series
+          series: self
-            [ nil, *@series_data ].each_cons(2).map do |previous, current|
-                unless current.trend
-                    current.trend = TimeSeries::Trend.new(
-                        current: current.value,
-                        previous: previous&.value,
-                        type: @type
-                    )
-                end
-                current
       end
     end
 end

app/models/time_series/trend.rb

@@ -1,48 +1,93 @@
 class TimeSeries::Trend
-  attr_reader :type
+  include ActiveModel::Validations
 
-  # Tells us whether an increasing/decreasing trend is good or bad (i.e. a liability decreasing is good)
+  attr_reader :current, :previous
-  TYPES = %i[normal inverse].freeze
 
-  def initialize(current: nil, previous: nil, type: :normal)
+  delegate :favorable_direction, to: :series
-    validate_data_types(current, previous)
+
-    validate_type(type)
+  validate :values_must_be_of_same_type, :values_must_be_of_known_type
+
+  def initialize(current:, previous:, series: nil)
     @current = current
     @previous = previous
-    @type = type
+    @series = series
+
+    validate!
   end
 
   def direction
-    return "flat" if @previous.nil? || @current == @previous
+    if previous.nil? || current == previous
-    return "up" if @current && @current > @previous
+      "flat"
+    elsif current && current > previous
+      "up"
+    else
       "down"
+    end.inquiry
   end
 
   def value
-    return Money.new(0) if @previous.nil? && @current.is_a?(Money)
+    if previous.nil?
-    return 0 if @previous.nil?
+      current.is_a?(Money) ? Money.new(0) : 0
-    @current - @previous
+    else
+      current - previous
+    end
   end
 
   def percent
-    return 0.0 if @previous.nil?
+    if previous.nil?
-    return Float::INFINITY if @previous == 0
+      0.0
-    ((extract_numeric(@current) - extract_numeric(@previous)).abs / extract_numeric(@previous).abs.to_f * 100).round(1).to_f
+    elsif previous.zero?
+      Float::INFINITY
+    else
+      change = (current_amount - previous_amount).abs
+      base = previous_amount.abs.to_f
+
+      (change / base * 100).round(1).to_f
+    end
+  end
+
+  def as_json
+    {
+      favorable_direction: favorable_direction,
+      direction: direction,
+      value: value,
+      percent: percent
+    }.as_json
   end
 
   private
-    def validate_type(type)
+    attr_reader :series
-      raise ArgumentError, "Invalid type" unless TYPES.include?(type)
+
+    def values_must_be_of_same_type
+      unless current.class == previous.class || [ previous, current ].any?(&:nil?)
+        errors.add :current, "must be of the same type as previous"
+        errors.add :previous, "must be of the same type as current"
+      end
     end
 
-    def validate_data_types(current, previous)
+    def values_must_be_of_known_type
-      return if previous.nil? || current.nil?
+      unless current.is_a?(Money) || current.is_a?(Numeric) || current.nil?
-      raise ArgumentError, "Current and previous values must be of the same type" unless current.class == previous.class
+        errors.add :current, "must be of type Money, Numeric, or nil"
-      raise ArgumentError, "Current and previous values must be of type Money or Numeric" unless current.is_a?(Money) || current.is_a?(Numeric)
+      end
+
+      unless previous.is_a?(Money) || previous.is_a?(Numeric) || previous.nil?
+        errors.add :previous, "must be of type Money, Numeric, or nil"
+      end
+    end
+
+    def current_amount
+      extract_numeric current
+    end
+
+    def previous_amount
+      extract_numeric previous
     end
 
     def extract_numeric(obj)
-      return obj.amount if obj.is_a? Money
+      if obj.is_a? Money
+        obj.amount
+      else
         obj
       end
     end
+end

app/models/time_series/value.rb

@@ -1,21 +1,17 @@
 class TimeSeries::Value
   include Comparable
+  include ActiveModel::Validations
 
-    attr_accessor :trend
+  attr_reader :value, :date, :original, :trend
-    attr_reader :value, :date, :original
 
-    def initialize(obj)
+  validates :date, presence: true
-        @original = obj.fetch(:original, obj)
+  validate :value_must_be_of_known_type
 
-        if obj.is_a?(Hash)
+  def initialize(date:, value:, original: nil, series: nil, previous_value: nil)
-            @date = obj[:date]
+    @date, @value, @original, @series = date, value, original, series
-            @value = obj[:value]
+    @trend = create_trend previous_value
-        else
-            @date = obj.date
-            @value = obj.value
-        end
 
-        validate_input
+    validate!
   end
 
   def <=>(other)
@@ -24,9 +20,27 @@ class TimeSeries::Value
     result
   end
 
+  def as_json
+    {
+      date: date,
+      value: value.as_json,
+      trend: trend.as_json
+    }
+  end
+
   private
-        def validate_input
+    attr_reader :series
-            raise ArgumentError, "Date is required" unless @date.is_a?(Date)
+
-            raise ArgumentError, "Money or Numeric value is required" unless @value.is_a?(Money) || @value.is_a?(Numeric)
+    def create_trend(previous_value)
+      TimeSeries::Trend.new \
+        current: value,
+        previous: previous_value,
+        series: series
+    end
+
+    def value_must_be_of_known_type
+      unless value.is_a?(Money) || value.is_a?(Numeric)
+        errors.add :value, "must be a Money or Numeric"
+      end
     end
 end

app/views/shared/_trend_change.html.erb

@@ -1,7 +1,7 @@
 <%# locals: { trend: } %>
 <% styles = trend_styles(trend) %>
 <p class="text-sm <%= styles[:text_class] %>">
-  <% if trend.direction == "flat" %>
+  <% if trend.direction.flat? %>
     <span>No change</span>
   <% else %>
     <span><%= styles[:symbol] %><%= trend.value.is_a?(Money) ? format_money(trend.value.abs) : trend.value.abs %></span>

app/views/shared/_value_heading.html.erb

@@ -16,7 +16,7 @@
   </p>
   <% if trend.nil? %>
     <p class="text-sm text-gray-500">Data not available for the selected period</p>
-  <% elsif trend.direction == "flat" %>
+  <% elsif trend.direction.flat? %>
     <p class="text-sm text-gray-500">No change vs. prior period</p>
   <% else %>
     <div class="flex items-center gap-2">

lib/money.rb

@@ -1,9 +1,11 @@
 class Money
-    include Comparable
+  include Comparable, Arithmetic
-    include Arithmetic
+  include ActiveModel::Validations
 
   attr_reader :amount, :currency
 
+  validate :source_must_be_of_known_type
+
   class << self
     def default_currency
       @default ||= Money::Currency.new(:usd)
@@ -15,59 +17,78 @@ class Money
   end
 
   def initialize(obj, currency = Money.default_currency)
-        unless obj.is_a?(Money) || obj.is_a?(Numeric) || obj.is_a?(BigDecimal)
+    @source = obj
-            raise ArgumentError, "obj must be an instance of Money, Numeric, or BigDecimal"
-        end
-
     @amount = obj.is_a?(Money) ? obj.amount : BigDecimal(obj.to_s)
     @currency = obj.is_a?(Money) ? obj.currency : Money::Currency.new(currency)
+
+    validate!
   end
 
   # TODO: Replace with injected rate store
   def exchange_to(other_currency, date = Date.current)
-        return self if @currency == Money::Currency.new(other_currency)
+    if currency == Money::Currency.new(other_currency)
-        rate = ExchangeRate.find_rate(from: @currency, to: other_currency, date: date)
+      self
-        return nil if rate.nil?
+    elsif rate = ExchangeRate.find_rate(from: currency, to: other_currency, date: date)
-        Money.new(@amount * rate.rate, other_currency)
+      Money.new(amount * rate.rate, other_currency)
+    end
   end
 
-    def cents_str(precision = @currency.default_precision)
+  def cents_str(precision = currency.default_precision)
     format_str = "%.#{precision}f"
-        amount_str = format_str % @amount
+    amount_str = format_str % amount
-        parts = amount_str.split(@currency.separator)
+    parts = amount_str.split(currency.separator)
-
-        return "" if parts.length < 2
 
+    if parts.length < 2
+      ""
+    else
       parts.last.ljust(precision, "0")
     end
-
-    # Basic formatting only.  Use the Rails number_to_currency helper for more advanced formatting.
-    alias to_s format
-    def format
-        whole_part, fractional_part = sprintf("%.#{@currency.default_precision}f", @amount).split(".")
-        whole_with_delimiters = whole_part.chars.to_a.reverse.each_slice(3).map(&:join).join(@currency.delimiter).reverse
-        formatted_amount = "#{whole_with_delimiters}#{@currency.separator}#{fractional_part}"
-        @currency.default_format.gsub("%n", formatted_amount).gsub("%u", @currency.symbol)
   end
 
-    def to_json(*_args)
+  # Use `format` for basic formatting only.
-        { amount: @amount, currency: @currency.iso_code }.to_json
+  # Use the Rails number_to_currency helper for more advanced formatting.
+  def format
+    whole_part, fractional_part = sprintf("%.#{currency.default_precision}f", amount).split(".")
+    whole_with_delimiters = whole_part.chars.to_a.reverse.each_slice(3).map(&:join).join(currency.delimiter).reverse
+    formatted_amount = "#{whole_with_delimiters}#{currency.separator}#{fractional_part}"
+
+    currency.default_format.gsub("%n", formatted_amount).gsub("%u", currency.symbol)
+  end
+  alias_method :to_s, :format
+
+  def as_json
+    { amount: amount, currency: currency.iso_code }.as_json
   end
 
   def <=>(other)
     raise TypeError, "Money can only be compared with other Money objects except for 0" unless other.is_a?(Money) || other.eql?(0)
-        return @amount <=> other if other.is_a?(Numeric)
+
-        amount_comparison = @amount <=> other.amount
+    if other.is_a?(Numeric)
-        return amount_comparison unless amount_comparison == 0
+      amount <=> other
-        @currency <=> other.currency
+    else
+      amount_comparison = amount <=> other.amount
+
+      if amount_comparison == 0
+        currency <=> other.currency
+      else
+        amount_comparison
+      end
+    end
   end
 
   def default_format_options
     {
-            unit: @currency.symbol,
+      unit: currency.symbol,
-            precision: @currency.default_precision,
+      precision: currency.default_precision,
-            delimiter: @currency.delimiter,
+      delimiter: currency.delimiter,
-            separator: @currency.separator
+      separator: currency.separator
     }
   end
+
+  private
+    def source_must_be_of_known_type
+      unless @source.is_a?(Money) || @source.is_a?(Numeric) || @source.is_a?(BigDecimal)
+        errors.add :source, "must be a Money, Numeric, or BigDecimal"
+      end
+    end
 end

test/models/time_series/trend_test.rb

@@ -7,6 +7,7 @@ class TimeSeries::TrendTest < ActiveSupport::TestCase
     assert_equal Money.new(50), trend.value
     assert_equal 100.0, trend.percent
   end
+
   test "up" do
     trend = TimeSeries::Trend.new(current: 100, previous: 50)
     assert_equal "up", trend.direction
@@ -19,8 +20,8 @@ class TimeSeries::TrendTest < ActiveSupport::TestCase
 
   test "flat" do
     trend1 = TimeSeries::Trend.new(current: 100, previous: 100)
-    trend3 = TimeSeries::Trend.new(current: 100, previous: nil)
+    trend2 = TimeSeries::Trend.new(current: 100, previous: nil)
-    trend2 = TimeSeries::Trend.new(current: nil, previous: nil)
+    trend3 = TimeSeries::Trend.new(current: nil, previous: nil)
     assert_equal "flat", trend1.direction
     assert_equal "flat", trend2.direction
     assert_equal "flat", trend3.direction
@@ -39,7 +40,7 @@ class TimeSeries::TrendTest < ActiveSupport::TestCase
   end
 
   test "empty" do
-    trend =TimeSeries::Trend.new
+    trend = TimeSeries::Trend.new(current: nil, previous: nil)
     assert_equal "flat", trend.direction
   end
 end

test/models/time_series_test.rb

@@ -6,7 +6,7 @@ class TimeSeriesTest < ActiveSupport::TestCase
 
     assert_equal Money.new(100), series.first.value
     assert_equal Money.new(200), series.last.value
-    assert_equal :normal, series.type
+    assert_equal "up", series.favorable_direction
     assert_equal "up", series.trend.direction
     assert_equal Money.new(100), series.trend.value
     assert_equal 100.0, series.trend.percent
@@ -18,21 +18,18 @@ class TimeSeriesTest < ActiveSupport::TestCase
     assert_equal 100, series.first.value
     assert_equal 200, series.last.value
     assert_equal 100, series.on(1.day.ago.to_date).value
-    assert_equal :normal, series.type
+    assert_equal "up", series.favorable_direction
     assert_equal "up", series.trend.direction
     assert_equal 100, series.trend.value
     assert_equal 100.0, series.trend.percent
   end
 
-  test "when nil or empty array passed, it returns empty series" do
+  test "when empty array passed, it returns empty series" do
-    series = TimeSeries.new(nil)
-    assert_equal [], series.values
-
     series = TimeSeries.new([])
 
     assert_nil series.first
     assert_nil series.last
-    assert_equal({ values: [], trend: { type: "normal", direction: "flat", value: 0, percent: 0.0 }, type: "normal" }.to_json, series.to_json)
+    assert_equal({ values: [], trend: { favorable_direction: "up", direction: "flat", value: 0, percent: 0.0 }, favorable_direction: "up" }.to_json, series.to_json)
   end
 
   test "money series can be serialized to json" do
@@ -41,16 +38,16 @@ class TimeSeriesTest < ActiveSupport::TestCase
         {
           date: 1.day.ago.to_date,
           value: { amount: "100.0", currency: "USD" },
-          trend: { type: "normal", direction: "flat", value: { amount: "0.0", currency: "USD" }, percent: 0.0 }
+          trend: { favorable_direction: "up", direction: "flat", value: { amount: "0.0", currency: "USD" }, percent: 0.0 }
         },
         {
           date: Date.current,
           value: { amount: "200.0", currency: "USD" },
-          trend: { type: "normal", direction: "up", value: { amount: "100.0", currency: "USD" }, percent: 100.0 }
+          trend: { favorable_direction: "up", direction: "up", value: { amount: "100.0", currency: "USD" }, percent: 100.0 }
         }
       ],
-      trend: { type: "normal", direction: "up", value: { amount: "100.0", currency: "USD" }, percent: 100.0 },
+      trend: { favorable_direction: "up", direction: "up", value: { amount: "100.0", currency: "USD" }, percent: 100.0 },
-      type: "normal"
+      favorable_direction: "up"
     }.to_json
 
     series = TimeSeries.new([ { date: 1.day.ago.to_date, value: Money.new(100) }, { date: Date.current, value: Money.new(200) } ])
@@ -61,11 +58,11 @@ class TimeSeriesTest < ActiveSupport::TestCase
   test "numeric series can be serialized to json" do
     expected_values = {
       values: [
-            { date: 1.day.ago.to_date, value: 100, trend: { type: "normal", direction: "flat", value: 0, percent: 0.0 } },
+        { date: 1.day.ago.to_date, value: 100, trend: { favorable_direction: "up", direction: "flat", value: 0, percent: 0.0 } },
-            { date: Date.current, value: 200, trend: { type: "normal", direction: "up", value: 100, percent: 100.0 } }
+        { date: Date.current, value: 200, trend: { favorable_direction: "up", direction: "up", value: 100, percent: 100.0 } }
       ],
-        trend: { type: "normal", direction: "up", value: 100, percent: 100.0 },
+      trend: { favorable_direction: "up", direction: "up", value: 100, percent: 100.0 },
-        type: "normal"
+      favorable_direction: "up"
     }.to_json
 
     series = TimeSeries.new([ { date: 1.day.ago.to_date, value: 100 }, { date: Date.current, value: 200 } ])