improve metrics #77
improve metrics #77
Conversation
yelp_beans/logic/metrics.py
Outdated
|
|
||
| metrics = defaultdict(set) | ||
| for subscription in subscriptions: | ||
| metrics[subscription.key.urlsafe()] = [] |
There was a problem hiding this comment.
Since metrics is a defaultdict, why do we need this initialization code here?
There was a problem hiding this comment.
There are two types of subscriptions. One with users and one without. The second for-loop doesn't populate subscriptions without users because it iterates through users. The for-loop you commented on specifically ensures that even subscriptions with no users are represented in the data structure.
This isn't too straightforward, maybe it should have a comment?
There was a problem hiding this comment.
Yeah, I'd like to see a brief comment here. I definitely see a future where I come in and try to delete this bit of code 😛
There was a problem hiding this comment.
@ajm188 comments added
| @@ -15,31 +15,40 @@ def test_get_metrics(app, minimal_database): | |||
| response = metrics_api() | |||
| assert response == '[]' | |||
|
|
|||
| MeetingSubscription(title='test1').put() | |||
| new_subscription = MeetingSubscription(title='test1') | |||
| new_subscription.put() | |||
There was a problem hiding this comment.
Not a blocker, but just had this thought: is there a way we can make sure tests clean up after themselves easily?
There was a problem hiding this comment.
@ajm188 Ah good old test pollution, seems like we should maybe create an issue?
Fixes #48. Subscription counts only unique users now. In addition, this gives a count for current week participation.
The logic is cleaned up a bit as well, though this isn't the most efficient way of doing this metric. In the future, we would probably want to add some fields to the database to eliminate having to query the entire DB for some of these metrics.