Fixed an issue with links' expiration time. #144
Conversation
There was a problem hiding this comment.
Thanks so much for your contribution, it is very much appreciated. I have some feedback on your pull request.
Please note: Some of it is terse, but that is for the sake of brevity, not because I am a rude/unappreciative type. Apologies for not having time to reword to sound a bit friendlier!
Thanks again!
| else | ||
| Shortener.default_redirect || '/' | ||
| end | ||
| shortened_url.increment_usage_count if track |
There was a problem hiding this comment.
I'd like to keep this PR's changes to just the logic change, and specs.
| @@ -0,0 +1,23 @@ | |||
| require 'spec_helper' | |||
|
|
|||
| describe Shortener::ShortenedUrl do | |||
There was a problem hiding this comment.
Specs for Shortener::ShortenedUrl belong in spec/models/shortened_url_spec.rb
|
|
||
| describe Shortener::ShortenedUrl do | ||
|
|
||
| def generate_shortened_link(link, expiration_time) |
There was a problem hiding this comment.
This method does not add enough value to justify the added indirection.
|
|
||
| it "returns a default redirect" do | ||
| expected = { url: Shortener.default_redirect || '/', shortened_url: nil } | ||
| LINKS.each do |link| |
There was a problem hiding this comment.
Testing these four different URLs do not exercise the code in different ways, so do not add value over a single url.
| shortened_link = generate_shortened_link(link, Time.now) | ||
| token = shortened_link[:unique_key] | ||
| out = Shortener::ShortenedUrl.fetch_with_token(token: token) | ||
| expect(out).to eq expected |
There was a problem hiding this comment.
The expectation is testing the result of fetch_with_token, but the changes were to the unexpired scope. Let's test the scope instead.
| 'https://twitter.com', | ||
| 'https://reddit.com'] | ||
|
|
||
| it "returns a default redirect" do |
There was a problem hiding this comment.
We want to test whether the shortened link appears in/out of the scope based on the set expiry date. We should test:
- when there is no expiry date
- when there is an expiry date that has already past
- when there is an expiry date that is still in the future
We also want to test the scenario that caused you to reveal this edgecase - e.g. that having an alternate timezone causes the logic to fail.
Without doing some poking around, I am not sure the bug will cause problems whenever the system is not UTC, or perhaps it only occurs when the db server timezone is different to the application timezone.
If you could determine the scenario when this breakage occurs, and then craft some tests to simulate that scenario, it will demonstrate that your fix has done the job.
|
@jpmcgrath See: https://thoughtbot.com/blog/its-about-time-zones#don39t-use |
|
@threadedstream Is your DB configured to be UTC and is your app default also? Improperly mixing and matching timezone config can be a common source of errors related to timezones. If your DB is in UTC, Rails will properly convert all Time objects passed through ActiveRecord from the app timezone to UTC for the DB. I think I see an error that I addressed in #146 that would cause this to be incorrect in certain cases, but I think DB timezone config sounds like the first place to check. |
Today morning i encountered that links' ability to expire does not function properly. Turned out that unexpired links were filtered using ::Time::current::to_s(:db) routine, which outputs incorrect time for my country (3 hour difference).
To mitigate the problem, I just replaced it by ::Time.now. I want to apologize beforehand if there are some issues in the way I wrote the spec test. I'm relatively new to Ruby and am not aware of good practices of writing clean Ruby code.