Match timezones

[manpreetnarang]

No description provided.

[manpreetnarang]

@celsodantas Hey there, here's the fix for issue: #33

[jasdeepsingh]

@celsodantas we were finally able to figure out the reason for the duplicate ID's, the fix is included with this PR.

[celsodantas]

Hey guys! Thanks so much for hunting down this bug, but I think the fix needs to be somewhere else.

https://github.com/celsodantas/protokoll/blob/master/lib/protokoll/formater.rb#L64-L69 << all the Time.now should be Time.now.utc.

plus, this:
https://github.com/celsodantas/protokoll/blob/master/lib/protokoll/counter.rb#L33

should be comparing utc with utc, so it should be:

Time.now.utc.strftime(update_event(options)).to_i > record.updated_at.utc.strftime(update_event(options)).to_i

The issue I see using Time.zone.now is that it will be using the timezone of the server. If you have multiple servers running in different timezones (or if you move your server to a different timezone, or just change its clock) it will generate non-sequential numbers. And I think that's a problem.

The issue with my suggestion is that it's a breaking fix (will need to bump the major) and will format all the numbers using UTC.

So maybe we should have an extra option to be passed to the protokoll call to specify which timezone should the counter follow.

What do you guys think?

[jasdeepsingh]

@celsodantas you do raise good points with your suggestions regarding patching the formatter.rb and counter.rb instead.

However, even if servers are distributed across different timezones, in a Rails application Time.zone is driven from a setting somewhere within application.rb or one of the environments file, which looks something like:

config.time_zone = 'Eastern Time (US & Canada)'

And this above setting then becomes Time.zone

Now, if we started generating these sequences based on UTC timezone as you proposed, the users may start seeing records that they don't expect. Ex: a record created on 8th August, 10pm EDT/EST, will trigger the date to be 9th of August in UTC: causing a bit of a confusion for the users (given they know the pattern behind the sequence generation)

Perhaps, the solution can be to make protokoll accept a new Proc value which dictates the Timezone to use to generate the sequence. (Although, I don't know if this could introduce more complexities), so i'm thinking something like:

protokoll :number, pattern: "%y%m%d##", timezone: Proc.new { |model| model.timezone }

What are your thoughts on this? cc @manpreetnarang

[jasdeepsingh]

Oh wait, did I just said the same thing that you were proposing? 😄

[celsodantas]

hehe yeah, you did. 😂

You are right about the timezone used by the Rails server. But config.time_zone is used to define a default timezone, and use cases might change that based on the request: Time.use_zone or something. I'm guessing it's better to keep the timezone used by the gem separated so no more bugs related to timezones happen.

We could do your way, by passing a lambda (lambda is just cleaner =D):

protokoll :number, pattern: "%y%m%d##", timezone: -> (_) { "Eastern Time (US & Canada)" }

# or
protokoll :number, pattern: "%y%m%d##", timezone: -> (_) { Time.zone.name }

# or
protokoll :number, pattern: "%y%m%d##", timezone: -> (model) { model.timezone }

I like it.

IMO, the timezone param should be required, and not optional. That way, we avoid any confusion for new users or ppl upgrading. Being explicit in this case is better.

Do you guys want to implement it? If not, I can do it.
(will need to update the README too)

[jasdeepsingh]

@celsodantas thanks for getting back, I like the

protokoll :number, pattern: "%y%m%d##", timezone: -> (model) { model.timezone } version

Would appreciate if you can implement this, and we can chime in to help with manual verification/testing/QA on this.

[wqsaali]

Suggested change

	Time.zone.now.strftime(update_event(options)).to_i > record.updated_at.strftime(update_event(options)).to_i
	Time.current.strftime(update_event(options)).to_i > record.updated_at.strftime(update_event(options)).to_i

https://apidock.com/rails/Time/current/class