Fix: ArgumentError (MAYBE-MARKETING-2D)

[revise-dev]

Based on the codebase, I'm implementing a solution to handle non-negative value validation in the ToolFloat class more gracefully. The error occurs when a value less than 0 is provided to a field that should only accept non-negative numbers.

Key changes made:

1. Added a new non_negative parameter to the ToolFloat initializer to explicitly declare when a field should only accept non-negative values
2. Modified the initialization logic to automatically set the minimum value to 0.0 when non_negative is true
3. Added nil value handling to prevent errors when the input is nil
4. Improved the error message to be more specific about non-negative requirements
5. Added proper type checking before comparison to prevent comparison of nil with numeric values

This change maintains backward compatibility with existing code while adding the ability to explicitly declare non-negative requirements. The solution is more flexible than hard-coding a minimum value of 0, as it allows for both negative-allowed and non-negative fields to coexist in the system.

Tip

You can make revisions or ask questions of Revise.dev by using /revise in any comment or review!

• /revise Add a comment above the method to explain why we're making this change.
• /revise Why did you choose to make this change specifically?

[josefarias]

/revise this system is meant for casting, not validation. Would be better to set a minimum value of 0 when the corresponding tool presenter calls for it. And ideally make it impossible to submit invalid values (in this case negative ones) from the UI.

[revise-dev]

The current implementation of ToolFloat mixes concerns by handling both type casting and validation. Following the single responsibility principle, we should separate these concerns. The type casting system should focus solely on converting input values to the correct type, while validation should be handled at the presenter level and prevented at the UI level.

Changes made:

1. Removed min/max validation parameters from ToolFloat initialization
2. Removed validation checks from the cast method
3. Simplified the ToolFloat class to focus only on type casting
4. Kept the autonumeric character cleanup for proper numeric parsing

This change:

• Improves separation of concerns
• Makes the code more maintainable
• Follows Rails conventions where validation happens at the model/presenter level
• Allows for more flexible validation strategies per tool
• Reduces complexity in the type casting system

The validation of minimum and maximum values should now be handled:

1. At the UI level through input constraints
2. In the specific tool presenters through attribute options
3. Through custom validation methods where needed

Updated files:

• config/initializers/tool_attributes.rb

[Shpigford]

@josefarias Is the change Revise made correct?

[josefarias]

@Shpigford I don't think the proposed solution is correct. I'm lacking some context here but I'm guessing a negative number got passed to some tool that doesn't support negative numbers. If so, I think the ideal solution would be a front-end validation so the input can't be negative.

That'd usually be enough but given that we have a way to set a floor/min-value for tool attributes, we should leverage that and set e.g. attribute :years_to_grow, :tool_float, min: 0.0, ...etc. It all depends on whether I'm correct and which tool got passed a negative value.

I wonder if this'll work...

/revise use the comment I've written above to refine your solution.

[revise-dev]

Based on the comment, the solution should focus on:

1. Removing validation from the ToolFloat type class completely (as discussed in previous comments)
2. Adding proper min: 0.0 constraints at the presenter level where needed
3. Adding frontend validation to prevent negative inputs

The CompoundInterestCalculator is a good example where we should add these constraints, as financial calculations typically don't work with negative values for inputs like initial investment, contributions, and years.

I'm updating the CompoundInterestCalculator presenter first since it's the most obvious candidate. We'll add min: 0.0 to all numeric attributes that shouldn't be negative, while keeping existing constraints like max values. Once we see which specific tool is causing the issue, we can apply similar changes to other presenters.

Updated files:

• app/presenters/tool/presenter/compound_interest_calculator.rb
• app/views/tools/widgets/forms/_compound_interest_calculator.html.erb

[josefarias]

That looks significantly better!

[Shpigford]

/revise Looking good. I noticed this specifically happening in the Early Mortgage Payoff Calculator.

[josefarias]

but it does look like it misinterpreted my (admittedly unclear) comment about casting and not validating. Some minor intervention is needed here before merging.

[revise-dev]

Given the comment - "Looking good. I noticed this specifically happening in the Early Mortgage Payoff Calculator." - I can now provide a more targeted response.

Looking at the Early Mortgage Payoff Calculator presenter and its form, I notice that while there is a frontend validation for extra_payment using a range input slider with min: 0, some other numeric fields that should never be negative don't have corresponding frontend or backend validations.

The Early Mortgage Payoff Calculator has some fields that should never be negative: - loan_amount - extra_payment - interest_rate - original_term - years_left

Currently, only loan_amount has a min: 0.0 constraint in the backend, and only extra_payment has a frontend validation through the range slider. We should add appropriate min constraints to all these fields to prevent negative values from being entered.

The changes:

1. Add min: 0.0 constraints to all relevant numeric fields in the presenter
2. Add input_html: { min: 0 } to all numeric fields in the form view

This provides both frontend and backend protection against negative values while following the same pattern established in other calculator tools.

add non-negative constraints to early mortgage payoff calculator fields

<BEGIN FILE: app/presenters/tool/presenter/early_mortgage_payoff_calculator.rb>
class Tool::Presenter::EarlyMortgagePayoffCalculator < Tool::Presenter
attribute :loan_amount, :tool_float, default: 500_000, min: 0.0, max: 1_000_000.0
attribute :extra_payment, :tool_float, default: 500.0, min: 0.0

attribute :interest_rate, :tool_percentage, default: 0.0, min: 0.0
attribute :savings_rate, :tool_percentage, default: 4.0, min: 0.0

attribute :original_term, :tool_integer, default: 30, min: 0.0
attribute :years_left, :tool_integer, default: 30, min: 0.0, max: 100

def blank?
interest_rate.zero?
end

def mortgage_rate_30
mortgage_rate_cache.rate_30
end

def interest_savings
total_interest - total_interest_with_extra_payments
end

def total_interest
@total_interest ||= begin
result = 0
balance = loan_amount

  total_payments.times do
    interest_payment = balance * monthly_rate
    result = result + interest_payment
    balance = balance - (regular_payment - interest_payment)
  end

  result
end

end

def total_interest_with_extra_payments
@total_interest_with_extra_payments ||= begin
@months_to_payoff_with_extra_payments = 0
result = 0
balance = loan_amount

  while balance > 0
    break if balance > loan_amount # guard against malicious input

    @months_to_payoff_with_extra_payments += 1

    interest_payment = balance * monthly_rate
    principal_payment = regular_payment - interest_payment
    result = result + interest_payment
    balance = balance - principal_payment - extra_payment
  end

  result
end

end

def total_principal_and_interest
loan_amount + total_interest
end

def total_principal_and_interest_with_extra_payments
loan_amount + total_interest_with_extra_payments
end

def original_payoff_date
Date.today + total_payments.months
end

def new_payoff_date
Date.today + months_to_payoff_with_extra_payments.months
end

def savings_account_balance
@savings_account_balance ||= begin
result = 0

  months_to_payoff_with_extra_payments.times do
    result = result + extra_payment
    result = result * (1 + savings_rate / 12.0)
  end

  result
end

end

def net_difference
@net_difference ||= savings_account_balance - interest_savings
end

def time_saved
"#{months_saved / 12} years, #{months_saved % 12} months"
end

def net_difference_class
if net_difference > 0
"text-green-600"
elsif net_difference < 0
"text-red-600"
end
end

def net_difference_comment
if net_difference > 0
"Investing the extra payments could potentially yield better returns than paying off the mortgage early."
elsif net_difference < 0
"Paying off the mortgage early could potentially save you more money than investing the extra payments."
else
"The financial outcome is roughly the same whether you pay off the mortgage early or invest the extra payments."
end
end

def net_difference_comment_class
if net_difference > 0
"text-green-600"
elsif net_difference < 0
"text-red-600"
else
"text-gray-600"
end
end

private
def active_record
@active_record ||= Tool.find_by! slug: "early-mortgage-payoff-calculator"
end

def mortgage_rate_cache
  @mortgage_rate_cache ||= MortgageRate::Cache.new
end

def monthly_rate
  interest_rate / 12.0
end

def total_payments
  years_left * 12
end

def regular_payment
  @regular_payment ||= begin
    fv = loan_amount
    r = monthly_rate
    n = total_payments

    (fv * r * (1 + r)**n) / ((1 + r)**n - 1)
  end
end

def months_to_payoff_with_extra_payments
  unless defined?(@months_to_payoff_with_extra_payments)
    total_interest_with_extra_payments # set ivar as a side effect
  end

  @months_to_payoff_with_extra_payments
end

def months_saved
  total_payments - months_to_payoff_with_extra_payments
end

end

<BEGIN FILE: app/views/tools/widgets/forms/_early_mortgage_payoff_calculator.html.erb>
<%= form_with method: :get, data: { turbo_frame: frame_id, controller: "auto-submit-form", auto_submit_form_submit_on_connect_value: true } do |form| %>

<%= form.unit_field :loan_amount, label: "Loan amount", value: tool.loan_amount, unit_symbol: "$", data: { controller: "autonumeric", auto_submit_form_target: "auto" }, input_html: { min: 0 } %>

<%= form.unit_field :original_term, label: "Original loan term", value: tool.original_term, label_right: "years", data: { controller: "autonumeric", autonumeric_options_value: { decimalPlaces: 0 }, auto_submit_form_target: "auto" }, input_html: { min: 0 } %>

<%= form.unit_field :years_left, label: "Years left on loan", value: tool.years_left, label_right: "years", data: { controller: "autonumeric", autonumeric_options_value: { decimalPlaces: 0 }, auto_submit_form_target: "auto" }, input_html: { min: 0 } %>

<%= form.unit_field :interest_rate, label: "Interest rate", value: tool.mortgage_rate_30