Admin::ReportsController reusable search params #2012
Conversation
oeN
force-pushed
the
reports-reusable-params
branch
2 times, most recently
from
4c2d31c
to
eb63e38
Compare
| Time.current.beginning_of_month | ||
| end | ||
|
|
||
| def adjust_end_date string_date |
There was a problem hiding this comment.
Use def with parentheses when there are parameters.
| end | ||
| end | ||
|
|
||
| def adjust_start_date string_date = nil |
There was a problem hiding this comment.
Use def with parentheses when there are parameters.
oeN
force-pushed
the
reports-reusable-params
branch
2 times, most recently
from
126746e
to
c8eb27c
Compare
| def adjust_start_date(string_date = nil) | ||
| return Time.current.beginning_of_month unless string_date | ||
| Time.zone.parse(string_date).beginning_of_day | ||
| rescue |
There was a problem hiding this comment.
Can this rescue something more specific? It's easy for bugs to sneak in with the catchall rescue.
|
|
||
| def adjust_end_date(string_date) | ||
| Time.zone.parse(string_date).end_of_day | ||
| rescue |
| end | ||
|
|
||
| def adjust_start_date(string_date = nil) | ||
| return Time.current.beginning_of_month unless string_date |
There was a problem hiding this comment.
The previous code checked the equivalent of string_date.blank?, should we still be doing that (I suspect yes since we may get an empty string from the user)?
oeN
force-pushed
the
reports-reusable-params
branch
from
c8eb27c
to
dfcc720
Compare
There was a problem hiding this comment.
Thanks! Everything looks nicer now!
I just have one comment: when the date param is passed in an "unparsable" format Time.zone.parse will return nil raising a NoMethodError, eg:
> Time.zone.parse('asd').beginning_of_day
NoMethodError: undefined method `beginning_of_day' for nil:NilClassI'm not sure it's the scope of this PR since this issue was already there. Also since that input is controlled by some JS code (datepicker) maybe we can avoid this check? What do you think @oeN ?
|
@oeN also, I think that if you rebase against master the test suite failure will disappear, thanks! |
oeN
force-pushed
the
reports-reusable-params
branch
from
dfcc720
to
2b7cd2a
Compare
|
@kennyadsl Thanks for the comments! About the "unparsable" format, I think that we can avoid this check, for now, for two reasons:
|
I'm adding a new report to my project, and I want to reuse the same filter as the
sales_total.I think that would be great if we can have the search params logic in an another method like
search_params