[tor-bugs] #11309 [Tor Support]: Improve how support statistics are reported
Tor Bug Tracker & Wiki
blackhole at torproject.org
Mon Jun 9 07:23:25 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:
Actual Points: | Parent ID:
Points: |
-----------------------------+----------------------------
Changes (by lunar):
* status: needs_review => needs_revision
Comment:
I don't know if your treat is coffee, tee or Club Mate, but really, you
need to slow down. The code and the commit all feel rushed out to me.
We have little pressure to deliver quickly here. Why not take the time
to write good and maintainable code? Unless you're bored with
developping this, in that case it might be better to take a break or
rely on someone else. But I understood you had fun until now.
What follows is a review from the Git commit log of the devel branch.
I'm sure I've skipped some commits from the master branch. I have not
done a diff of the final result with the master branch or tested the
code in any way.
I wonder if this work should not be rebased into proper feature
branches. It would take some time but now the history have many commits
that result in half-working code.
> commit 2d4aeb140cf5c7a6345a6b736f1b0949e095b116
> Date: Thu May 22 20:12:08 2014 +0300
>
> restricting comments in tokens.html
>
> diff --git a/webchat/templates/tokens.html
b/webchat/templates/tokens.html
> index b8aaf8c..be24b3b 100644
> --- a/webchat/templates/tokens.html
> +++ b/webchat/templates/tokens.html
> @@ -8,6 +8,18 @@
> {% block script %}
> <script type="text/javascript"
src="/static/js/jquery.min.js"></script>
> <script src="/static/js/bootstrap.min.js"></script>
> + <script type="text/javascript">
> + $(document).ready (function (){
> + $(".comment").html(function(){
> + $(this).html($(this).text().substring(0,35)
> + + ' <span data-toggle="modal" data-target="#comment-modal"
style="color:blue; font-size:80%;"> Read more..</span>');
The extra space after the span does not look right. The end should be an
elipsis. Also, it's a link. Shouldn't <a/> be used instead of <span/>?
I don't belive using a “modal” dialog is appropriate. What if I want to
expand several comments at once because I'm not exactly sure what I
wrote a couple days ago?
I suggest you look at how Trac displays the summary in ticket tables
(IIRC, like on SponsorO page). It's probably closer to what we should
have.
> commit 4533191ef0851369d404e85b4d7efd2d8f3fb25c
> Date: Sun May 25 23:06:42 2014 +0300
>
> stats: a bit of code cleaning and text formating/restriction
> […]
> +Stats.actionhandler = {
That's not a good identifier. It should probably be capitalized like the
rest and that's two words here.
> + success: function(data) {
> + // if unlocked
> + console.log(data);
> + console.log(data['locked_by']);
> + // Lock issue in db
> + // Let the user edit the issue
> + $("#edit_text").val($("#issue_text-" + id).text());
> + current_issue_edit_id = id;
> + // else
> + // report that it's already lock and being edited by USER
> + },
Indentation is wrong. I know it's from previously commited code but the
comments don't really make sense. If they are TODO, then they should be
labeled as such. Same for the rest of the code.
> +Stats.UI = {
> + readMore: function(){
> + $("[name=issue_text]").html(function(){
> + if ( $(this).html().length > MAX_ROW_LENGTH){
> + $(this).html($(this).text().substring(0, MAX_ROW_LENGTH)
> + +'<span class="readMore" data-toggle="modal" data-
target="#Alert" style="color:blue; font-size:80%;);"> Read
more..</span>');
> + }
> + });
> + },
That's a brand new function right here. It's not mentioned in the commit
message in any way.
> commit 52ed34bd6c336d201be223c676d12d5fddf12b7a
> Date: Thu May 29 23:35:22 2014 +0300
>
> edit now supports race condition prevention
> […]
> + $("[name=edit_issue]").click(function() {
> + Stats.actionhandler.edit_issue($(this).attr("id").split('-')[1]);
> + });
> +
> + $("[name=delete_issue]").click(function() {
> + Stats.actionhandler.delete_issue($(this).attr("id").split('-')[1]);
> + });
> +
> + $("[name=plus_one_button]").click(function() {
> + Stats.actionhandler.plus_one($(this).attr("id").split('-')[1]);
> + });
> +
Don't you see a pattern here? The `id` splitting should really be
factored in a function of its own. You are using it all over.
> + if (data['lock_status'] == 'lock_success'){
Missing a space here.
> + $('#EditIssue').modal('show');
> […]
> + $('#Alert').modal('show');
Modal is probably not good UI here. What if I want to copy/paste text
from another issue? Why should I get interrupted until I press a button
if the issue is locked by someone else?
I don't see where the lock is actually taken in this commit.
> def get_issues(self):
> - return Issue.objects.all() # Should order by the highest
> frequency
> + return Issue.objects.all().order_by('-frequency')
That's an unrelated change. Should be in its own commit.
> commit 48e9312f284a80a8f6f0e6e46bcb31c6ff3be001
> Date: Sat May 31 03:10:23 2014 +0300
>
> live ajaxy updates and more race condition prevention
> […]
> +var gridUpdateInterval = setInterval("Stats.Util.updateGrid()", 10000);
Magic number. Should be in a constant and it should be said what it
means.
> - Stats.actionhandler.edit_issue($(this).attr("id").split('-')[1]);
> + Stats.Util.updateGrid();
> + Stats.actionhandler.editIssue($(this).attr("id").split('-')[1]);
This is a functional change munged with a style change. That's really
bad for commit readability and understanding.
> + updateGrid: function() {
> + $.ajax({
> + type: "POST",
> + dataType: "json",
> + url: "/stats_data_ajax",
> + success: function(data) {
What happen in case of errors?
What happen if I leave a tab open and I go offline?
> + extractReport: function() {
> +
> + },
> +
> + monthlyWipe: function() {
> +
> }
Undocumented empty functions. :( Also, unrelated to the commit message.
I have a hard time understanding the rest of the commit because it's
mixing several things. No idea why the `issue_id` field is removed and
why there's suddenly a new function below. And why getting access to
stats data don't require a login. (I see this is fixed in a subsequent
commit.)
> commit acf61b0054ddcd684816d48f9c4cb8c39e5c740d
> Date: Sat May 31 20:33:35 2014 +0300
>
> dynamically created elements in the DOM are now functional
> […]
> -var MAX_ROW_LENGTH = 76;
> -var gridUpdateInterval = setInterval("Stats.Util.updateGrid()", 10000);
> +var MAX_ROW_LENGTH = 83;
Why change MAX_ROW_LENGTH? Also why is it not defined as const?
> - $("[name=delete_issue]").click(function() {
> + $(document).on('click', '[name=delete_issue]', function() {
Why? Should be explained in the commit message.
> + $("#issue-" + value['pk']).append(
> + '<div id="short_issue_text-'+ value['pk']+'"
name="issue_text" class="col-md-9">'
> + +'<span id="short_sub_text-'+
value['pk']+'">'+shortText+'</span>'
> + +more
> + +' </div>'
> + +'<input id="full_issue_text-'+ value['pk']+'"
type="hidden" value="'+ value['fields'].text+'">'
> + +'<input id="user" type="hidden" value="">'
> + +'<div class="options">'
> + +'<input id="plus_one-'+ value['pk']+'" name="plus_one"
class="form-control" type="text" size="4" value="'+value['fields'].f
> + +' <button id="plus-'+ value['pk']+'"
name="plus_one_button" class="btn btn-default">+1</button>'
> + +' <button id="edit-'+ value['pk']+'" name="edit_issue"
class="btn btn-default" data-toggle="modal">Edit</button>'
> + +' <button id="del-'+ value['pk']+'"
name="delete_issue" class="btn btn-danger">Delete</button>'
> + +'</div>'
That doesn't feel right. You should not have to duplicate how issues
should be displayed. Having a code path to present issues when they are
first displayed or added later has a high risk of inconsistency. Leads
to bad things.
> + console.log("id: " + id);
Leftover debug message. Bad.
> commit c487a2814605ca2c461804f0276472c20279ca6c
> Date: Mon Jun 2 06:40:54 2014 +0300
>
> added a new db column (visits) which acts as a counter for visits
per token by users
>
> diff --git a/pups/settings.py.sample b/pups/settings.py.sample
> index 97d4ab6..85607cf 100644
> --- a/pups/settings.py.sample
> +++ b/pups/settings.py.sample
> @@ -98,6 +98,7 @@ INSTALLED_APPS = (
> 'pups',
> 'webchat',
> 'stats',
> + 'south'
> )
You are adding a new dependency here. That should be documented in the
commit message and as I asked earlier in the user documentation.
(And maybe in the developper documentation.)
> + def visited(self, id):
> […]
> + token.update(visits=F('visits')+1)
That's not a good name. A method doing something to the world should
have a verb. When I saw the method name, my initial thinking was “this
will return True or False depending if the token has ever been visited”.
> +
> + t_obj.visited(t_obj.pk) # Count visits for metrics
> +
Interestingly enough, you've spotted the problem yourself later on. This
comments would not need to be there if the method was called
“count_visit”. (Please look up an English dictionnary here, I'm unsure
that it's an accepted usage of the verb count.)
Also, `t_obj` is not really a nice identifier.
> - t = Token()
> + # t = Token()
Why?
> commit e4e8321e12fb1149e2d92ea67ef1a943bf708343
> 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
This does not feel right. You are duplicating logic here. What if the
rules for expiry change one day? (e.g. it also expires if there has been
more than 15 visits)
> + data['frequent_issues'] =
len(Issue.objects.filter(created_by=owner.username))
Not a big deal, but isn't there a method that allows you to do counting
queries without having to load the full objects in memory?
> +<div style="width:600px;">
Why?
> + <td><b>Live Tokens</b></td>
> + <td><b>Token visits</b></td>
> + <td><b>Expired Tokens</b></td>
> + <td><b>Revoked Tokens</b></td>
Capitalization mismatch.
> + data = get_home_stats(request.user.id)
> + return render(request, "pups.html", {'data': data})
`data` is one of the worst possible identifier. Sometimes, you have to
resort to use it, but it should be the last case. Can't you find
something more descriptive here?
> commit f82bf37bb0a40e0de2f3dcb42a9accdea912d515
> Date: Wed Jun 4 22:51:46 2014 +0300
>
> filtering pups/home stats by month instead of all time
Then the month should be shown together with the stats, don't you think?
> commit 5d810d5cce5ca3a3c3e5319f184a3c64d61c82ba
> Date: Thu Jun 5 11:08:47 2014 +0300
>
> sending token.comment when the chat session starts
>
> diff --git a/static/js/prodromus.js b/static/js/prodromus.js
> index 916df2c..8395b18 100644
> --- a/static/js/prodromus.js
> +++ b/static/js/prodromus.js
> @@ -222,7 +222,10 @@ Prodromus.actionhandler = {
> Prodromus.connection.send( $pres() );
>
> Prodromus.buildAndSendMessage(
> - Prodromus.Util.htmlspecialchars(
Prodromus.config.SENDERNAME ) + Prodromus.i18n.t9n( 'msg-hello' ) +
Prodromus.i18n.t9n(
> + Prodromus.Util.htmlspecialchars(
Prodromus.config.SENDERNAME ) +
> + Prodromus.i18n.t9n( 'msg-hello' ) +
> + Prodromus.i18n.t9n( 'token' ) +
> + Prodromus.i18n.t9n( 'comment' )
Why is there only one variabled escaped here? I believe at least
`comment` to be able to contain HTML characters.
> , 'chat'
> );
> break;
> @@ -347,7 +350,8 @@ Prodromus.t9n = {
> 'failed-to-connect': 'Failed to connect to the server!',
> 'msg-hello': ' joins the chat.',
> 'msg-goodbye': ' leaves the chat. ',
> - 'token': " Token: " + Prodromus.config.TOKEN
> + 'token': " Token: " + Prodromus.config.TOKEN,
> + 'comment': "Comment: " + Prodromus.config.COMMENT
> }
>
> }
t9n stands for “translation”. Puting the token and comments there is
just wrong.
> commit 93f4e9f54cf05b723c4fd900aabbc70f63277b95
> Date: Sat Jun 7 17:24:34 2014 +0300
>
> added a management page which points to extractReport and wipeStats
I'm still unsure what wipeStats is for.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/11309#comment:6>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
More information about the tor-bugs
mailing list