[tor-bugs] #11309 [Tor Support]: Improve how support statistics are reported
Tor Bug Tracker & Wiki
blackhole at torproject.org
Wed Sep 3 22:11:55 UTC 2014
#11309: Improve how support statistics are reported
-----------------------------+----------------------------
Reporter: Sherief | Owner: Sherief
Type: task | Status: needs_revision
Priority: normal | Milestone:
Component: Tor Support | Version:
Resolution: | Keywords: pups
Actual Points: | Parent ID:
Points: |
-----------------------------+----------------------------
Comment (by lunar):
I'm reviewing the whole `squash` branch here, even if it actually
contains unreleated changes.
{{{
commit 7bf14e728aec2dd0e536c2e0459a0e497276d315
[…]
diff --git a/webchat/models.py b/webchat/models.py
index b475a6d..c4c0fc9 100644
--- a/webchat/models.py
+++ b/webchat/models.py
[…]
@@ -73,3 +74,12 @@ class Token(models.Model):
'''
return Token.objects.filter(owner=assistant).order_by('-t_id')\
.filter(expires_at__gt=timezone.now())
+
+ def visited(self, id):
}}}
Could the method name be made a verb? The passive form does not make it
clear about what this does. Oh, but I see you've try to fix that in
`98ab42`. That would be a good one to squash here.
{{{
[…]
diff --git a/webchat/views.py b/webchat/views.py
index aec8135..a8d92ed 100644
--- a/webchat/views.py
+++ b/webchat/views.py
@@ -86,7 +86,7 @@ def chat(request, token):
and did not expire.
'''
- t = Token()
+ # t = Token()
t_obj = get_object_or_404(Token, token=token)
# Make sure token didn't expire
}}}
Why is this commented? Should it disappear forever? Is it for debugging?
Is there any reason to leave a comment in the code?
{{{
commit ce6cb6db626b42460a5f5f2f253fb78f06615cf5
Author: Sherief Alaa <sheriefalaa.w at gmail.com>
Date: Tue Jun 17 13:41:30 2014 +0300
using constants instead of variables and removed token and comment
from t9n
diff --git a/static/js/prodromus.js b/static/js/prodromus.js
index a16c065..b647864 100644
--- a/static/js/prodromus.js
+++ b/static/js/prodromus.js
@@ -42,9 +42,9 @@
* @license Affero General Public License
*/
-_assistantNotAvailable = 0
-_assistantAvailable = 1
-_serverError = 2
+const _assistantNotAvailable = 0
+const _assistantAvailable = 1
+const _serverError = 2
var assisantStatus;
var status_msg;
}}}
It is customary to use all caps for constants. Like
`ASSISTANT_NOT_AVAILABLE`. Maybe not a huge deal, but Prodromus itself
uses that convention.
This commit mixes two different things. I believe `90dde834` should be
rewritten to never add things to the translation map in the first place.
{{{
commit d84b78b207f4ff4d77b08bd4b5d9d9743b05cca6
Author: Sherief Alaa <sheriefalaa.w at gmail.com>
Date: Tue Jun 17 14:45:08 2014 +0300
using django's slice instead of truncatechars
}}}
The commit message does not answer the question “why?”. Maybe I should not
care, but I don't why one is better than the other here.
{{{
commit eed88415ee1195ac9d5fbeac9831d87614408f9a
Author: Sherief Alaa <sheriefalaa.w at gmail.com>
Date: Sat Jun 21 22:14:31 2014 +0300
removed a repeated variable
}}}
Why? Is it just refactoring or does it has other consequences? Isn't
`Token()` a stateful object? Can it become a stateful object one day? This
change makes me nervous but I don't know much Django. At least a comment
would help.
Okay, now I see this is fixed in `8228aa02`. Probably it should also be
squashed earlier on the patch set.
{{{
commit 89a73d813317a182ae151a15b05aaacc271a851c
Author: Sherief Alaa <sheriefalaa.w at gmail.com>
Date: Sun May 18 01:33:42 2014 +0300
Implements Stats
Stats is a client-side tool that helps support teams report what
kind questions they answer and their frequency.
[…]
diff --git a/pups/templates/management.html
b/pups/templates/management.html
new file mode 100644
index 0000000..0e7065a
--- /dev/null
+++ b/pups/templates/management.html
[…]
+ <form role="form" action="/backupstats" method="GET">
+ {% csrf_token %}
+ <tr>
+ <td>Backup & reset issues counters</td>
}}}
That's a destructive action, right? A destructive action should never be
behind a `GET` action.
I'm not sure what the fields will do in this form.
Also, isn't it better to have this only called through the command-line?
With the current design, it *has* to be called every 1st of the month to
be meaningful. It would probably be better to make it easily fit in a
*crontab*.
{{{
[…]
+ }else{
+ $('#Alert').modal('show');
+ $(".alert-body").html("This issue is currently being edited
by " + issue['locked_by']);
+ }
}}}
Is there a timeout procedure? What happens if I get disconnected while
editing an issue? Can somebody else ever get back the log?
All handlers in Stats.ActionHandler do the exact same thing for `error`
and `headers`. Worth factoring out maybe.
{{{
[…]
a/webchat/models.py b/webchat/models.py
index 8d25021..6722aa1 100644
--- a/webchat/models.py
+++ b/webchat/models.py
@@ -54,6 +54,13 @@ class Token(models.Model):
return q.t_id is not None
@staticmethod
+ def get_token(self, token):
+ try:
+ return Token.objects.get(token=token)
+ except ObjectDoesNotExist:
+ return []
+
}}}
Now, I'm confused. Why is this method coming back? And why in a commit for
stats?
{{{
commit 5afc6340aa2b7b7c6e3d3e024efd7f9214ed747d
Author: Sherief Alaa <sheriefalaa.w at gmail.com>
Date: Wed Jun 4 22:12:15 2014 +0300
pups/home now reports personal user stats
[…]
+ for token in tokens:
+ if token.expires_at > token.created_at:
+ data['live_tokens'] += 1
+ elif token.expires_at < token.created_at:
+ data['expired_tokens'] += 1
+ if token.created_at == token.expires_at:
+ data['revoked_tokens'] += 1
}}}
Still putting data logic in a controller here.
{{{
commit e259032f2bb90b7ab4975bf889ab639444995812
Author: Sherief Alaa <sheriefalaa.w at gmail.com>
Date: Mon Jul 14 17:09:01 2014 +0200
Enable monthly on-disk backup stats reports
[…]
+def backup_stats_report(month, year, forceOverwrite = False):
+ '''
+ Dumps a monthly report in a text file on disk and resets
+ the frequency counters
+ '''
}}}
This comment is misleading. According to my understanding of the code,
what it does is save the current status of the database in the given
monthly report. But it will not particularily select issues for a given
month+year. That ought to be made very clear, because given the name of
the method and the signature, it can be very confusing.
{{{
commit 0fc7b1381d32f35f077e41a097b245c2fb13d2fe
Author: Sherief Alaa <sheriefalaa.w at gmail.com>
Date: Tue Jul 29 09:03:37 2014 +0200
Updates README.md
[…]
+Adding DB columns:
+==================
+Sometimes after creating your schema you'd want to add a column
+or two but django 1.4.5 doesn't support that. Luckily there is
+a solution for this (python-django-south) and this is how to use it..
}}}
That's for a `HACKING` file, not for a `README`. Admins which will set up
Pups don't care on how to change the schema, they just want to set up the
tool.
{{{
commit 96fd52bd2956b5eb41e69b53f882771bb65031b5
[…]
@@ -59,6 +51,31 @@ def get_home_stats(uid,
month=datetime.datetime.now().month):
return data
+def get_live_tokens(owner, month):
+ return Token.objects.filter(owner=owner)\
+ .filter(expires_at__month=month)\
+ .filter(expires_at__gt=F('created_at')).count()
+
[…]
}}}
Shouldn't this be with other Token related methods?
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/11309#comment:14>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
More information about the tor-bugs
mailing list