[tor-bugs] #11309 [Tor Support]: Improve how support statistics are reported
Tor Bug Tracker & Wiki
blackhole at torproject.org
Wed Jul 16 08:03:19 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:
Replying to [comment:7 Sherief]:
> > > {{{
> > > 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.
>
>
> I am not sure what do you want me to do right here, hide a member string
function?
You are doing the exact same thing 3 times (and maybe in other places).
This should be factored out in a function.
> For example:
>
> get_id(delimiter, pos){
> // logic here
> return id
> }
Wrong variables. You are finding a value in the 'id' argument of an
object. What is varying here is the object.
> > > {{{
> > > + if (data[wiki:'lock_status' lock_status] ==
'lock_success'){
> > > }}}
> > >
> >
> > Missing a space here.
>
> I leave such style errors when I do a periodic $ pep8 on the project dir
Don't. Every commit should stand on its own.
> > > {{{
> > > + 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?
>
> It will keep sending requests indefinitely until the server replies.
So that's not ok. Please implement a way of exponantial backoff. I don't
want the browser to eat up CPU and battery when the network goes down.
> > > {{{
> > > + $("#issue-" + value[wiki:'pk' pk]).append(
> > > + '<div id="short_issue_text-'+ value[wiki:'pk'
pk]+'" name="issue_text" class="col-md-9">'
> > > + +'<span id="short_sub_text-'+ value[wiki:'pk'
pk]+'">'+shortText+'</span>'
> > > + +more
> > > + +' </div>'
> > > + +'<input id="full_issue_text-'+ value[wiki:'pk'
pk]+'" type="hidden" value="'+ value[wiki:'fields' fields].text+'">'
> > > + +'<input id="user" type="hidden" value="">'
> > > + +'<div class="options">'
> > > + +'<input id="plus_one-'+ value[wiki:'pk' pk]+'"
name="plus_one" class="form-control" type="text" size="4"
value="'+value[wiki:'fields' fields].f
> > > + +' <button id="plus-'+ value[wiki:'pk' pk]+'"
name="plus_one_button" class="btn btn-default">+1</button>'
> > > + +' <button id="edit-'+ value[wiki:'pk' pk]+'"
name="edit_issue" class="btn btn-default" data-
toggle="modal">Edit</button>'
> > > + +' <button id="del-'+ value[wiki:'pk' 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.
>
> Yes, I realized that but I am pretty sure I did it correctly. Please
check it out:
>
https://github.com/SheriefAlaa/projectpups/blob/devel/static/js/stats.js#L207
The issue is that you are duplicating code. Same code in the JavaScript
part and same code in Python part. Bad idea. This can be easily solved by
getting the web server to send the fully formated HTML.
> > > {{{
> > > 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.)
> >
>
> I agree, I will do a commit to update the read me once I finish the
project.
Don't. Commits should stand on their own. Also, there is no “finishing”
the project. This is a production application that will stay running as
long as we fnid it useful. So there will be bugs, changes, adaptations…
The documentation should always be in sync.
> > > {{{
> > > 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[wiki:'live_tokens' live_tokens] += 1
> > > + elif token.expires_at < token.created_at:
> > > + data[wiki:'expired_tokens' expired_tokens] += 1
> > > + if token.created_at == token.expires_at:
> > > + data[wiki:'revoked_tokens' revoked_tokens] += 1
> > > }}}
> > >
> >
> > This does not feel right. You are duplicating logic here. What if the
rules for expiry change one day?
>
> This code is only intended for the assistant's personal statistics.
Doesn't matter. You are duplicating logic in two different places. That's
bad. It should be factored out and the common function/method used where
needed.
> > > {{{
> > > 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.
> >
>
> Because non of them are actually inputs except for SENDERNAME.
Err… Isn't “comment” given by support assistants?
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/11309#comment:9>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
More information about the tor-bugs
mailing list