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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix method that fails in Christmas #100

Merged
merged 5 commits into from Jan 5, 2018

Conversation

Projects
None yet
2 participants
@Ana06
Copy link
Member

Ana06 commented Dec 29, 2017

The birthdays_of_this_week method worked always except when executing it from 26 to 31 December as we hadn't have into account the new year break. 馃巹

I wrote a blog post about this:
http://anamaria.martinezgomez.name/2018/01/01/christmas-failure.html

Fixes #98.

@Ana06 Ana06 force-pushed the Ana06:birthday branch from 7436acf to 1a7e673 Dec 29, 2017

@Ana06 Ana06 force-pushed the Ana06:birthday branch 6 times, most recently from 41d5e5e to af6a631 Jan 2, 2018

@openSUSE openSUSE deleted a comment from codecov-io Jan 2, 2018

@Ana06 Ana06 force-pushed the Ana06:birthday branch from af6a631 to 4cd26c9 Jan 2, 2018

@openSUSE openSUSE deleted a comment from codecov-io Jan 2, 2018

@Ana06 Ana06 requested a review from DavidKang Jan 2, 2018

end
def self.next_birthdays(number_days = 6)
today = Date.current
today_str = "#{today.year}#{today.strftime('%m%d')}"

This comment has been minimized.

@bgeuken

bgeuken Jan 2, 2018

Member

Date.today.strftime('%Y%m%d')

today = Date.current
today_str = "#{today.year}#{today.strftime('%m%d')}"
limit = today + number_days.days
limit_str = "#{limit.year}#{limit.strftime('%m%d')}"

This comment has been minimized.

@bgeuken

bgeuken Jan 2, 2018

Member

number_days.days.from_now.strftime('%Y%m%d')

User.where(
"(('#{today.year}' || to_char(birthday, 'MMDD')) between '#{today_str}' and '#{limit_str}')" \
'or' \
"(('#{limit.year}' || to_char(birthday, 'MMDD')) between '#{today_str}' and '#{limit_str}')"

This comment has been minimized.

@bgeuken

bgeuken Jan 2, 2018

Member

And because .from_now takes the year into account you should be able to just write:

  User.where("to_char(birthday, 'YYYYMMDD') between '#{today_str}' and '#{limit_str}'")

This comment has been minimized.

@bgeuken

bgeuken Jan 2, 2018

Member

And it probably should be

User.where("to_char(birthday, 'YYYYMMDD') between ? and ?", today_str, today_str)

untested, but might also work:

 User.where(birthday: Date.today..number_days.days.from_now)

edit: Ignore this. It doesn't work.

This comment has been minimized.

@bgeuken

bgeuken Jan 4, 2018

Member

I tested my previous examples and they don't work, because of the year. 馃槅

But this would work. Though the last line might be to fancy. But I wanted to share it^^ Btw I would consider making next_birthdays a scope. Since it's all about querying data.

  scope :birthdays_from, ->(date) {                                                                                                                                                                           
    where("to_char(birthday, 'MMDD') >= ?", date.strftime('%m%d'))                                                                                                                                            
  }                                                                                                                                                                                                           
  scope :birthdays_until, ->(date) {                                                                                                                                                                          
    where("to_char(birthday, 'MMDD') <= ?", date.strftime('%m%d'))                                                                                                                                            
  }                                                                                                                                                                                                                                                                                                                                                                                                                    
                                                                                                                                                                                                              
  def self.next_birthdays(number_days = 6)                                                                                                                                                                    
    today = Date.today                                                                                                                                                                                        
    in_n_days = number_days.days.from_now                                                                                                                                                                     
                                                                                                                                                                                                              
    sql_operator = (today.year == in_n_days.year ? :merge : :or)                                                                                                                                              
                                                                                                                                                                                                              
    birthdays_from(today).send(sql_operator, birthdays_until(in_n_days))                                                                                                                                      
  end

This comment has been minimized.

@Ana06

Ana06 Jan 5, 2018

Member

@bgeuken nice solution! 馃槈

It seems to be a little bit less efficient than mine, but it is a really small difference and that can even depend on the database. Those are the real elapses times:

image

Also, using from_now can give you problems when running the method at midnight, as if the day changes you will be changing number day fro one day more. And take into account that Date.today is of type Date, while number_days.days.from_now is ActiveSupport::TimeWithZone. And you don't gain anything using from_now.

@Ana06 Ana06 force-pushed the Ana06:birthday branch from 4cd26c9 to c5d42e4 Jan 5, 2018

Ana06 added some commits Jan 2, 2018

Fix method that fails in Christmas
The `birthdays_of_this_week` method worked always except when executing
it from 26 to 31 December as we hadn't have into account the new year
break.

Fixes #98.
Add 29/2 birthday in birthdays_of_this_week test
Improve `User#birthdays_of_this_week` test to avoid that it fails again
by adding a case with a user born on 29th February.
Add birthday in new year in birthdays_of_this_week
Improve `User#birthdays_of_this_week` test to avoid that it fails again
by adding a case with a user born at the beginning of the year.
Refactor User#birthdays_of_this_week test
Shorten the test and improve its readability.

Pair-programmed with @bgeuken.

@Ana06 Ana06 force-pushed the Ana06:birthday branch from c5d42e4 to e16920b Jan 5, 2018

Use scope for User#birthdays_of_this_week
and rename it for readability.

@Ana06 Ana06 force-pushed the Ana06:birthday branch from e16920b to d622c61 Jan 5, 2018

@openSUSE openSUSE deleted a comment from codecov-io Jan 5, 2018

@bgeuken

bgeuken approved these changes Jan 5, 2018

Copy link
Member

bgeuken left a comment

馃憤

@bgeuken bgeuken merged commit 19ca092 into openSUSE:master Jan 5, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment