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
make timestamp optional for playing now listens in RedisListenStore #475
Conversation
b0d622c
to
de9e9be
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @vansika!
Thanks for the contribution! It looks good to me overall, just a few high level changes for clarity. Also, a test would be really awesome for this.
@@ -30,7 +30,7 @@ def get_playing_now(self, user_id): | |||
if not data: | |||
return None | |||
data = ujson.loads(data) | |||
data.update({'listened_at': MIN_ID+1}) | |||
data.update({'listened_at': None}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of directly putting None here, I would prefer it if we added a playing_now
key to the data and then set the timestamp attribute in the from_json function there accordingly.
@@ -30,7 +30,7 @@ def get_playing_now(self, user_id): | |||
if not data: | |||
return None | |||
data = ujson.loads(data) | |||
data.update({'listened_at': MIN_ID+1}) | |||
data.update({'listened_at': None}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we should look into removing MIN_ID altogether from the codebase. Is it used anywhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MIN_ID has been used as 'listened_at' value for playing now listens. If we put appropriate checks on timestamp value according to listen type , MIN_ID can be removed from the codebase.
listenbrainz/listen.py
Outdated
@@ -99,10 +99,12 @@ def __init__(self, user_id=None, user_name=None, timestamp=None, artist_msid=Non | |||
@classmethod | |||
def from_json(cls, j): | |||
"""Factory to make Listen() objects from a dict""" | |||
if j['listened_at']: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like a test for from_json
where we create pass a json file with playing_now
set to true and check that the timestamp is None and then pass a json file for a normal listen (non playing_now
) and check that the timestamp is valid in the Listen object.
You can add this test to test_listen.py
.
b551c4f
to
e2921ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The builds were failing because of a recent bug in MessyBrainz. I've fixed it and rebased this PR to fix it. Sorry about that! :(
This PR now looks pretty great, except for a few nitpicks and style changes. If you fix those, we should be good to go. :)
Also, the commit messages for two of the commits are the same. In general, each commit should have a message telling us what it specifically does. So the second commit should have been something like Add playing_now key in redislistenstore and use it in from_json
. You can fix this by doing an interactive rebase. (git rebase -i HEAD~3
)
listenbrainz/listen.py
Outdated
@@ -6,6 +6,7 @@ | |||
|
|||
from datetime import datetime | |||
from listenbrainz.utils import escape, convert_to_unix_timestamp | |||
from flask import current_app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems extraneous.
@@ -1,6 +1,5 @@ | |||
import logging | |||
|
|||
MIN_ID = 1033430400 # approx when audioscrobbler was created |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woohoo! 🎉 🎉
@@ -15,12 +15,17 @@ def generate_data(test_user_id, user_name, from_ts, num_records): | |||
test_data = [] | |||
artist_msid = str(uuid.uuid4()) | |||
|
|||
for i in range(num_records): | |||
if (from_ts == None): #check for playing now listens |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We generally don't do parantheses in if conditions in Python. :)
this could be better written as if from_ts is None
item = Listen( | ||
user_name=user_name, | ||
user_id=test_user_id, | ||
timestamp=datetime.utcfromtimestamp(from_ts), | ||
timestamp= timestamp, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not to nitpick, but there is an extra space here. PEP-8, the style guide we try to loosely follow, suggests spaces between operands but not between arguments in functions.
so timestamp=timestamp
instead of the current line.
|
||
|
||
def test_from_json(self): | ||
json_row = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we possibly use testdata/valid_single.json
here? (https://github.com/metabrainz/listenbrainz-server/blob/master/listenbrainz/testdata/valid_single.json). It would reduce the complexity and size of the test a lot.
json_row.update({'playing_now': True}) | ||
listen = Listen.from_json(json_row) | ||
|
||
self.assertEqual(listen.timestamp, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be None and not 0? we have actual listens in the db with timestamp 0.
e2921ce
to
38ff8aa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks for the contribution. 🥇
Happy to work☺ |
Problem
A random value is used as timestamp for 'playing now' listens.
Solution
Timestamp value in Listen object is assigned after checking the 'Listened_at' value which would be None in case of 'playing_now' listens.