-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FYST-1704] Income/ donation forms: Inaccurate date selection #5570
base: main
Are you sure you want to change the base?
Changes from all commits
eb4961e
792af63
cb411e3
8651a55
8fc6433
fe12345
0f84752
a40dae7
d94b9da
b05c77c
fa878c0
f854dd2
cc00a54
fd87c4b
618546d
6226267
e164c23
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,6 @@ module DateAccessible | |
TAX_YEAR = Date.new(Rails.configuration.statefile_current_tax_year) | ||
|
||
included do | ||
private | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what's the significance of making these not private methods? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The methods from the module are class methods, so they can't be private, and I believe this line has no effect |
||
|
||
# Calls `date_reader` and `date_writer` on specified date properties to set | ||
# getters and setters on the specified date properties. For use with | ||
|
@@ -29,15 +28,13 @@ def self.date_reader(*properties) | |
|
||
properties.each do |property| | ||
self.define_method("#{property}_month") do | ||
send(property)&.month | ||
self.instance_variable_get("@#{property}_month") || send(property)&.month | ||
end | ||
|
||
self.define_method("#{property}_year") do | ||
send(property)&.year | ||
self.instance_variable_get("@#{property}_year") || send(property)&.year | ||
end | ||
|
||
self.define_method("#{property}_day") do | ||
send(property)&.day | ||
self.instance_variable_get("@#{property}_day") || send(property)&.day | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will sidestep any other setters, as opposed to calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes that's the intent - for this setter to default to the instance variable, with a backup of parsing the persisted value |
||
end | ||
end | ||
end | ||
|
@@ -52,42 +49,31 @@ def self.date_writer(*properties) | |
properties = [properties] unless properties.is_a?(Enumerable) | ||
|
||
properties.each do |property| | ||
self.define_method("#{property}_month=") do |month| | ||
change_date_property(property, month: month) unless month.blank? | ||
end | ||
attr_writer :"#{property}_month", :"#{property}_year", :"#{property}_day" | ||
|
||
self.define_method("#{property}_year=") do |year| | ||
change_date_property(property, year: year) unless year.blank? | ||
end | ||
before_validation do | ||
month_to_set = self.instance_variable_get("@#{property}_month") | ||
day_to_set = self.instance_variable_get("@#{property}_day") | ||
year_to_set = self.instance_variable_get("@#{property}_year") | ||
|
||
self.define_method("#{property}_day=") do |day| | ||
change_date_property(property, day: day) unless day.blank? | ||
if year_to_set.present? && month_to_set.present? && day_to_set.present? | ||
send("#{property}=", Date.new(year_to_set.to_i, month_to_set.to_i, day_to_set.to_i)) | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what happens if day/month/year attribute is missing? (fails the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if one of the values is missing, then the date won't get set / updated, and it'll fail the inclusion check in it's model |
||
rescue Date::Error | ||
send("#{property}=", nil) | ||
end | ||
end | ||
end | ||
|
||
# Takes in valid arguments to Date#change. Will create a new date if | ||
# `date_of_contribution` is nil, otherwise will merely modify the correct | ||
# date part. Values can be strings as long as #to_i renders an appropriate | ||
# integer. Note that Date#change only accepts :year, :month, and :day as | ||
# keys, all other keys will be treated as nothing was passed at all. | ||
# | ||
# Note that until all three fragments are passed; month, day, and year | ||
# For year, a range must be indicated on the validator on the property itself | ||
# | ||
# @see Date#change | ||
# | ||
# @param date_property [Symbol] The property to manipulate | ||
# @param args [Hash<Symbol, String | Integer>] Arguments conforming to Date#change | ||
def change_date_property(date_property, args) | ||
existing_date = send(date_property) || Date.new | ||
self.class_eval do | ||
validate :"#{property}_date_valid" | ||
|
||
self.send( | ||
"#{date_property}=", | ||
existing_date.change(**args.transform_values(&:to_i)) | ||
) | ||
rescue Date::Error | ||
nil | ||
define_method("#{property}_date_valid") do | ||
date = send(property) | ||
if date.present? && !Date.valid_date?(date.year, date.month, date.day) | ||
errors.add(:date_of_contribution, :invalid_date, message: I18n.t("activerecord.errors.models.az321_contribution.attributes.date_of_contribution.inclusion")) | ||
end | ||
end | ||
end | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,3 +24,4 @@ | |
state_file_az_intake | ||
end | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is the presence check removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for this the inclusion validation serves as a presence check as well, as
nil
values fall outside of the range. It made it so I didn't have to duplicate the error message