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

Sunday event shows as a Saturday event #3099

Open
NickSchimek opened this issue Mar 24, 2019 · 19 comments

Comments

@NickSchimek
Copy link
Member

commented Mar 24, 2019

General Description

It seems events that are created to re-occur every week are being set for the incorrect day.
for example. a reoccurring Sunday event is scheduled for Saturday, a reoccurring Saturday event is set for Friday. etc...

Expected Behavior

An event created for Sunday should show as reoccurring on Sunday.

Current Behavior

An event created for every Sunday is showing up as a Saturday event.

Here are some screen shots. Here the event was created to re-occur every Sunday.
Screenshot 2019-03-23 18 29 42

And here on the event page is shows as occurring every Saturday.
Screenshot 2019-03-23 18 29 59

Pics taking from the production environment.
Saturday event being shown as Friday.
Screenshot 2019-03-23 21 11 27

Friday event is showing as Thursday.
Screenshot 2019-03-23 21 16 52

Possible Solution

Not sure, could start by checking to see how the masks are implemented. We're using the ice_cube gem for this and it offers great readability such as using the word :sunday for the mask, but the code base seems to be using an integer 64 to mean Sunday. https://github.com/seejohnrun/ice_cube#weekly, although this has been working in the past...
See comment below.

Steps to Reproduce (for bugs)

  1. Create new recurring weekly event for every Sunday at 5PM PST
  2. Visit event page and see that it shows it's a Saturday event
@NickSchimek NickSchimek added the bug label Mar 24, 2019
@NickSchimek

This comment has been minimized.

Copy link
Member Author

commented Mar 24, 2019

Did some more investigating and came to the conclusion that it's due to how we're either storing or retrieving the event dates from the database. The bug only occurs when an event set to reoccur in the evening after 5pm for PST, because currently PST is -7 hours UTC and anything after 5PM is being stored/retrieved incorrectly. The bug does not show for events set to occur before 5PM.

@NickSchimek

This comment has been minimized.

Copy link
Member Author

commented Mar 24, 2019

This is also causing this spec to fail when it is ran after 5PM PST. spec/models/event_spec.rb:73

@tansaku

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

thanks for reporting this @NickSchimek

@mattwr18 mattwr18 added the ready label Mar 26, 2019
@mattwr18

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

@abdellani has expressed interest in working on this, maybe you could pair @NickSchimek

@NickSchimek

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2019

@mattwr18 I'd be happy to pair on it, but to be honest I don't have much time right now. @abdellani is more than welcome to claim the ticket and work on it as I wasn't planning to work on it. If you have any questions or need any help I can definitely provide assistance. Just reach out to me on slack @Poppa Nick

@mattwr18

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

great thanks @NickSchimek... we voted on it this morning 😸 :

@abdellani

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

Hi all :)
I tried to reproduce the bug using my local time zone (+1 UTC). I used the same value from the screenshots, 18:25-18:55 PDT America/Los_Angeles <=> 02:25-02:55 (CEST) Africa/Algiers

1
2

Then, I tried to create a new event using 18:25PM (+1UTC)

3
4

I didn't trigger even, after trying several values on local time 1:00,2:00,3:00,18:00,20:00,22:00. but, when I changed the time zone in the form to America/Los_Angeles :

5
6

The time (hour and time) was converted correctly, only the day is wrong, it supposed to be Monday 2:00 AM.
I didn't inspect the content of the database yet, because I faced a connection problem using pgAdmin.

I'm still working on it.

Regards.

@NickSchimek

This comment has been minimized.

Copy link
Member Author

commented Mar 28, 2019

Yep that's it. when -7 hours for America los angeles timezone then the error can be reproduced if time is set to after 5pm. I wonder if the error will be re-produced for your time zone +1 hour if the time is set for 12:30 am.

I hadn't thought about positive time zones, but may it will be a day ahead????

@abdellani

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

That's right @NickSchimek .

I think that I found the source of the bug, but please check with me again.
When I created a new event at 12:30 AM (+1UTC) for every Sunday. It was saved in the database as follow :

repeats_weekly_each_days_of_the_week_mask start_datetime timestamp without time zone
64 2019-03-27 23:30:00

After submitting the form, The browser was redirected to app/controllers/events_controller.rb#show, where I found that @event.next_occurrences was sending wrong dates

    @event_schedule = @event.next_occurrences

I followed the method calls in app/models/event.rb and finally reached the schedule method

  def schedule
    sched = series_end_time.nil? || !repeat_ends ? IceCube::Schedule.new(start_datetime) : IceCube::Schedule.new(start_datetime, :end_time => series_end_time)
    case repeats
      when 'never'
        sched.add_recurrence_time(start_datetime)
      when 'weekly', 'biweekly'
        days = repeats_weekly_each_days_of_the_week.map { |d| d.to_sym }
        sched.add_recurrence_rule IceCube::Rule.weekly(repeats_every_n_weeks).day(*days)
    end
    self.exclusions ||= []
    self.exclusions.each do |ex|
      sched.add_exception_time(ex)
    end
    sched
  end

This method was looking for the occurrences with days equals to Sunday. which means that it will find the next Sundays 23:30 +0 UTC in the interval [start_datetime, :end_time],
then the results will be converted back to +1UTC which will give Monday 00:30 +0 UTC.

Conclusion :
When a user submit a new event, The website is only converting the hour to UTC, but forgets about the days.
For example: 12:30 AM (+1UTC) every Sunday should be saved 11:30 PM every Saturday in the database.

One possible solution is to update the create_start_date_time inside app/controllers/events_controller.rb

  def create_start_date_time(event_params)
    return unless date_and_time_present?
    tz = TZInfo::Timezone.get(params['start_time_tz'])
    event_params[:start_datetime] = next_date_offset(tz).to_utc(DateTime.parse(params['start_date']+ ' ' + params['start_time']))
  end
@NickSchimek

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2019

@abdellani I'm not sure what the best approach would be. I could look into if you'd like and we can discuss it. I think the solution you suggested looks solid though. You probably know this already, but the first thing I would do is set up a couple tests (one for positive time zones, and one for negative time zones) to protect against regression, and then you can easily play around with different implementations.

@tansaku

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

the solution makes sense to be as well, but @NickSchimek 's suggestion is solid - create a few tests that expose the bug specifically, see them fail, and then make the change to the application code to ensure the proposed fix does what you expect

@NickSchimek

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

@abdellani How's it going with this issue? Anything I can help with?

@abdellani

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

@NickSchimek
Sorry, I was absent this period.
I'll resume working on this bug soon.
Regards.

@NickSchimek

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

No worries @abdellani . I was just curious if you wanted to pair on it sometime as I'm planning to make some free time for pairing on a couple issues.

@abdellani

This comment has been minimized.

Copy link
Contributor

commented May 12, 2019

@NickSchimek

I wrote this test where I tried to create a weekly event (each Sunday) at 00:30 (Africa/Algiers +1UTC). It failed with the following error message :

expected to find text "Saturday" in "Anders Persson 
ABOUT
PROJECTS
MEMBERS
PREMIUM
EVENTS 
GETTING STARTED
Daily Standup
we stand up
Event type: PairProgramming
Event for: All
Event Actions 
Next scheduled event:
Sunday, May 19, 2019 
23:30-00:00 (UTC) Africa/Abidjan
Occurs weekly at the specified times
created by:
Anders Persson
2019-05-19
AttendCannot Attend
No previous instances of this event
Learn more about Scrums
Learn More
About Us
Getting Started
Dashboard
Opportunities
Blog
Press Kit
Social
 Facebook
 Twitter
Our Sponsors
Standup Bot
Craft Academy
Mooqita
Becoming a sponsor
AgileVentures - Crowdsourced Learning
Contact us
Send a traditional email to info@agileventures.org.
We are a Charitable Incorporated Organisation (CIO) in the UK. Ref #1170963" (RSpec::Expectations::ExpectationNotMetError)

I think there is a small problem in the test environment. The show event page rendered after the creation of the event, loads the data in the (Africa/Abidjan) (+0 UTC) time zone (which is the first option in the drop-down menu).

Unless there is a ways to change this behavior of the testing environment, we need converted our time related inputs into africa/adidjan's time zone. As in the test, Sunday 00:30 ( +1 UTC) -> Saturday 23:30 ( +0 UTC)

@NickSchimek

This comment has been minimized.

Copy link
Member Author

commented May 12, 2019

Yes, I think you are right. The test environment doesn't seem to be producing the same results as the development/production environment.
I just created an event in Algiers time zone by configuring my machine to CEST. And a Sunday event gets created as a Monday event

Screen Shot 2019-05-12 at 12 23 36 AM

The time is correct, but the day is wrong.

I created another event after putting my machine back to PST. Created an event by selecting Algiers time zone and put 0:22 am on Sunday. And the show page displays it as PST.

Screen Shot 2019-05-11 at 9 26 45 PM

Unfortunately simply converting we need converted our time related inputs into africa/adidjan's time zone will not work since tests get ran in all sorts of locations around the world we will need to control what location the show page is detecting in order to reproduce the development/production environment.

We'll need to have a cucumber line that states something like:
A user located in Algiers logs in and creates an event .... I'm not sure how to do this off the top of my head. Will have to look into. Maybe @tansaku knows?

@NickSchimek

This comment has been minimized.

Copy link
Member Author

commented May 12, 2019

@abdellani so I did some quick searching and comically found this issue that asks that capybara team how to solve the issue we're seeing. teamcapybara/capybara#1685
It seems our good pal @tansaku is all over the net. I'm not sure if this would work. It turns off the timezone detection and sets it to whatever you pass in as an argument.

Given(/^the browser is in "([^"]*)" and the server is in UTC$/) do |tz|

@abdellani

This comment has been minimized.

Copy link
Contributor

commented May 12, 2019

@NickSchimek , with Given the browser is in "UTC" and the server is in UTC, I got this error :

      Unable to find css "form#event-form" (Capybara::ElementNotFound)
      ./features/step_definitions/basic_steps.rb:162:in `block (3 levels) in <top (required)>'
      ./features/step_definitions/basic_steps.rb:161:in `each'
      ./features/step_definitions/basic_steps.rb:161:in `block (2 levels) in <top (required)>'
      ./features/support/helpers.rb:103:in `with_scope'
      ./features/step_definitions/basic_steps.rb:160:in `/^I fill in event field(?: "([^"]*)")?:$/'
      features/events/create_events.feature:157:in `Given I fill in event field:'
@NickSchimek

This comment has been minimized.

Copy link
Member Author

commented May 12, 2019

Ah, here is the issue. That method has multiple responsibilities. It has code to visit the root path, hence it can't find the relevant form element that would be on the event page. https://github.com/AgileVentures/WebsiteOne/blob/develop/features/step_definitions/event_steps.rb#L175

Here is what I would do. Create this new step below and invoke it before the test runs.

Given(/^the user is located in "([^"]*)"$/) do |timezone|
  ENV['TZ'] = timezone
end

And then create and run this step below after your assertion or after your test runs.

And(/^reset timezone to UTC$/) do
  ENV['TZ'] = 'UTC'
end

There is probably a better way to do this so that we don't need to explicitly run the clean up code when the setup code is ran... That will be something to look into.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.