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

make timestamp optional for playing now listens in RedisListenStore #475

Merged
merged 5 commits into from Jan 18, 2019

Conversation

vansika
Copy link
Member

@vansika vansika commented Jan 7, 2019

  • This is a…
    • Bug fix
    • Feature addition
    • Refactoring
    • Minor / simple change (like a typo)
    • Other

Problem

A random value is used as timestamp for 'playing now' listens.

  • JIRA ticket (optional): LB-404

Solution

Timestamp value in Listen object is assigned after checking the 'Listened_at' value which would be None in case of 'playing_now' listens.

Copy link
Collaborator

@paramsingh paramsingh left a 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})
Copy link
Collaborator

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})
Copy link
Collaborator

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?

Copy link
Member Author

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.

@@ -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']:
Copy link
Collaborator

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.

Copy link
Collaborator

@paramsingh paramsingh left a 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)

@@ -6,6 +6,7 @@

from datetime import datetime
from listenbrainz.utils import escape, convert_to_unix_timestamp
from flask import current_app
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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,
Copy link
Collaborator

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 = {
Copy link
Collaborator

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)
Copy link
Collaborator

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.

Copy link
Collaborator

@paramsingh paramsingh left a 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. 🥇

@vansika
Copy link
Member Author

vansika commented Jan 14, 2019

Happy to work☺

@paramsingh paramsingh merged commit d21dcb6 into metabrainz:master Jan 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants