Skip to content
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

between_dates(nil, nil) or between_times(nil, nil) clears method chaining. #87

Open
sidemt opened this issue May 31, 2020 · 4 comments
Open

Comments

@sidemt
Copy link

sidemt commented May 31, 2020

I found that between_dates(nil, nil) or between_times(nil, nil) clears all method chaining. Is this an expected behabior?

For example, I have User and Post models. I tried the following on rails console.

irb(main):002:0> user = User.first
  User Load (0.6ms)  SELECT `users`.* FROM `users` ORDER BY `users`.`id` ASC LIMIT 1
=> #<User id: 1, email: ... >
irb(main):003:0> user.posts.count
   (0.7ms)  SELECT COUNT(*) FROM `posts` WHERE `posts`.`user_id` = 1
=> 41
irb(main):004:0> user.posts.between_dates(Date.new(2000, 1), nil).count
   (0.9ms)  SELECT COUNT(*) FROM `posts` WHERE `posts`.`user_id` = 1 AND (posts.created_at >= '1999-12-31 15:00:00')
=> 41
irb(main):005:0> user.posts.between_dates(nil, Date.today).count
   (0.7ms)  SELECT COUNT(*) FROM `posts` WHERE `posts`.`user_id` = 1 AND (posts.created_at <= '2020-06-01 14:59:59')
=> 41
irb(main):006:0> user.posts.between_dates(nil, nil).count # See the generated SQL below
   (0.7ms)  SELECT COUNT(*) FROM `posts`
=> 1326

I expected it to work as follows:

irb(main):006:0> user.posts.between_dates(nil, nil).count
   (0.7ms)  SELECT COUNT(*) FROM `posts` WHERE `posts`.`user_id` = 1
=> 41

Checked with:

  • by_star 4.0.0 3b53809
  • rails 6.0.2.2
  • ruby 2.6.5
@radar
Copy link
Owner

radar commented Jun 1, 2020

I think it's a bug. If both values are nil, why would you want to use this method?

@sidemt
Copy link
Author

sidemt commented Jun 1, 2020

I was creating a selectbox form where a user can select dates to view the records of a specific period, or of all time by leaving the dates blank.

At first I did something like this.

# start_date and end_date are given by user input
def expenses_for_normal_user(start_date, end_date)
  current_user.expenses.where(active: true).between_dates(start_date, end_date) # this clears method chaining and returns all records
end

As of now, I'm checking if parameters are nil and changing whether to call between_dates to get the desired behavior.

# start_date and end_date are given by user input
def expenses_for_normal_user(start_date, end_date)
  if start_date || end_date
    current_user.expenses.where(active: true).between_dates(start_date, end_date)
  else
    # if both start_date and end_date are nil, don't use between_dates
    current_user.expenses.where(active: true)
  end
end

@radar
Copy link
Owner

radar commented Jun 1, 2020

I think that's your best bet.

You could also do that a bit shorter:

def expenses_for_normal_user(start_date, end_date)
  expenses = current_user.expenses.where(active: true)
  return expenses if start_date.blank? && end_date.blank?

  expenses.between_dates(start_date, end_date)
end

@sidemt
Copy link
Author

sidemt commented Jun 4, 2020

Thank you for the suggestion! That looks better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants