-
Notifications
You must be signed in to change notification settings - Fork 75
Fix formatting of the #company_descriptive_date filed
#67
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
Conversation
#strftime will return a string representation where #strptime is a `DateTime`.
| f.strftime('%y%m%d') | ||
| rescue | ||
| f |
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.
Thanks for reporting the issue. The only problem is that this f value may not be a Date, it really can be any 6 characters according to the ACH specs.
What I believe we should do is make sure it's always 6 characters (which is what I thought strptime would do, my bad..) something like:
| f.strftime('%y%m%d') | |
| rescue | |
| f | |
| f = Date.parse(f) unless f.is_a? Date | |
| f.strftime('%y%m%d') | |
| rescue | |
| f.rjust(6, " ") |
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.
Updated the comment because we might want to do parse only if it's not a date.
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.
I understand your goal but there are a few problems
fcan beTimeorDateTime- in case if
fmatch to/^SD\d+$/(e.g.SD12345)
Also, from the definition of the .field method, I see that it supports validation, maybe this is a more appropriate place for it?
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.
The case for SDhhmm strings is handled on line 5
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.
The company_descriptive_date field of the batch header is optional and alphanumeric, it doesn't have to be parse-able, so I don't see how we could validate it as it really can be any 6 characters
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.
I believe this covers all the cases:
| f.strftime('%y%m%d') | |
| rescue | |
| f | |
| f = Date.parse(f) unless f.respond_to?(:strftime) | |
| f.strftime('%y%m%d') | |
| rescue | |
| f.to_s.rjust(6, ' ') | |
| end |
It will:
- Try to parse the date only if
fdoes not already respond tostrftime - If it fails, pad the string with spaces up to 6 characters
What do you think @chubchenko?
Overview
In the previous release, the
#stringify_with_same_daymethod was changed. After that,company_descriptive_datestarted having different formatting when creating the ACH file.To prove that I have added a test and attached a screenshot below.