fix: query_simplified_user_actions and add test cases#15
fix: query_simplified_user_actions and add test cases#15dtgoitia wants to merge 1 commit intotxomon:masterfrom
Conversation
|
Some comments to this PR:
|
dtgoitia
force-pushed
the
test
branch
2 times, most recently
from
c9aafc8 to
19d843d
Compare
| ([ | ||
| {'user': 1, 'action': 'upvote'}, | ||
| ], { | ||
| 0: Action.upvote.name, |
There was a problem hiding this comment.
Don't use a dict, and don't use .name, it should be an object!
| {'dtid': '1234', 'username': 'username'}, | ||
| {'id': 1, 'dtid': '1234', 'username': 'username', 'country': None}, | ||
| ), | ||
| ( |
There was a problem hiding this comment.
Formatting shouldn't be changed here
|
|
||
| for action in user_action: | ||
| if action['user'] is None: | ||
| user = await user_generator() |
There was a problem hiding this comment.
Just create this statically, like 3 users and that's it
There was a problem hiding this comment.
If we create the users statically, we still need to somehow map the action['user'] value to the corresponding user (user1, user2...). Is it not too verbose?
user1 = await user_generator()
user2 = await user_generator()
user3 = await user_generator()
user4 = await user_generator()
if action['user'] == 1:
user = user1
elif action['user'] == 2:
user = user2
elif action['user'] == 3:
user = uset3
elif action['user'] == 4:
user = user4
There was a problem hiding this comment.
Check that user_action_generator just needs a dict with an 'id' key, therefore you just need to call it like
await user_action_generator(user={'id': action['user_id']}, ...)
There was a problem hiding this comment.
I was actually looking for that, but I got confused with the function signature (def user_action_generator(db_conn):). I missed that all the arguments extra arguments are passed down to the generate_user_action function via *... #$@&%*!
dtgoitia
force-pushed
the
test
branch
3 times, most recently
from
69f6a14 to
89114e4
Compare
| async for user_action in await conn.execute(query): | ||
| result.append(dict(user_action)) | ||
| return result | ||
| return result No newline at end of file |
There was a problem hiding this comment.
No newline at the end of the file...
dtgoitia
force-pushed
the
test
branch
2 times, most recently
from
155946f to
14364e5
Compare
txomon
left a comment
txomon
left a comment
There was a problem hiding this comment.
I have some doubts, but overall looks good
| await user_action_generator(user={'id': action['user_id']}, playback=playback, action=action['action']) | ||
|
|
||
| simplified_user_action = await query_simplified_user_actions(playback_id=playback['id'], conn=db_conn) | ||
| actual_result = {i: user_action['action'] for i, user_action in enumerate(simplified_user_action)} |
There was a problem hiding this comment.
I don't understand why compare a dict instead of the list of the result direclty. Why are you transforming the result to check it?
| left join user_action ua on p.id = ua.playback_id | ||
| left join track t on p.track_id = t.id | ||
| left join "user" u on ua.user_id = u.id | ||
| where p.id = {playback_id} |
There was a problem hiding this comment.
Have you checked a safer way to execute this query? you are literally rendering the parameter directly, instead of escaping it or using it as a parameter. I don't know if aiopg the library we are using can do it but please have a look
https://chartio.com/resources/tutorials/how-to-execute-raw-sql-in-sqlalchemy/
txomon
force-pushed
the
test
branch
2 times, most recently
from
f79f1b9 to
17545ee
Compare
|
So going back through this, I have found that the performance is a bit terrible, using explain analyze: |
3 participants
Main change
With this fix, the function
query_simplified_user_actionswill now return all the simplified actions related to a specific playback (given the playback ID). In this case, simplified means that only the lastupvoteordownvoteaction of each user will be returned, together with theskip, if any at all. If there are no actions, an empty list will returned.Other changes
Close #2