LB-187: Add NOT NULL constraint to the last_login field #214
Conversation
Also, some refactoring to make the `update_last_login` function use PostgreSQL's NOW() method, considering we never change the `last_login` value to anything else.
| @@ -0,0 +1,12 @@ | |||
| BEGIN; | |||
There was a problem hiding this comment.
in acousticbrainz we put the date in the filenames of update scripts, to make a it a little easier to know in what order to run things. Should we do this here too?
listenbrainz/db/tests/test_user.py
Outdated
| user = db_user.get_by_mb_id(user['musicbrainz_id']) | ||
| self.assertEqual(val, int(user['last_login'].strftime('%s'))) | ||
| # after update_last_login, the val should be equal to the current time | ||
| self.assertEqual(int(time.time()), int(user['last_login'].strftime('%s'))) |
There was a problem hiding this comment.
this is dangerous, because 99% of the time it might work, but then suddenly there may be a case where we do the above line just before the time increases by a second.
I would prefer to create a user with a last login time obviously in the past - you could do this by doing an sql query to get now() - interval 1 day or choose some time in june 2017 - and then do the update and check that this value is greater than the original value that you set it to
There was a problem hiding this comment.
Hmm, I actually thought that this could be a problem when writing the test, should have done this from the beginning, done now!
First run an SQL query to update the last login value to unix epoch. Then call the function and assert that the value now is greater than 0.
paramsingh
force-pushed
the
last-login/null
branch
from
dc1e9a2
to
968fbf6
Compare
| self.assertEqual(val, int(user['last_login'].strftime('%s'))) | ||
|
|
||
| # after update_last_login, the val should be greater than the old value i.e 0 | ||
| self.assertGreater(int(user['last_login'].strftime('%s')), 0) |
There was a problem hiding this comment.
This test is a bit iffy to me. I think you should get the current time, save it, then ensure that the DB got updated to that time.
There was a problem hiding this comment.
That would require changing the function to take a timestamp parameter and update to that timestamp, which is what alastair wanted changed here: #189 (comment)
Initially I had written the test like you are suggesting here but we changed it here with alastair's review: #214 (comment)
Should I go back to taking a timestamp in the function and not using NOW()?
There was a problem hiding this comment.
Naw, lets just improve the test once #189 is implemented. But in general giving a timestamp makes more sense, because it allows the caller to determine what to do.
Fix issues pointed out by @alastair in this review: #189 (review)