[tor-bugs] #11309 [Tor Support]: Improve how support statistics are reported

Sat Oct 4 02:56:12 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 Sherief):

 Replying to [comment:14 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.

 Done.

 > {{{
 > […]
 > 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?

 Rebased and fixed.

 > {{{
 > 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.

 Fixed.

 > This commit mixes two different things. I believe `90dde834` should be
 rewritten to never add things to the translation map in the first place.

 Fixed.

 > {{{
 > 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.

 Because truncatechars returns a string with three trailing dots, see:
 https://docs.djangoproject.com/en/1.7/ref/templates/builtins/#truncatechars

 Anyway, this commit was squashed.

 > {{{
 > 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.

 Fixed.

 > {{{
 > 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*.

 Done as agreed on irc. See:
 https://github.com/SheriefAlaa/projectpups/commit/b21c86eeb2ddb7bed678b408030325adb9b0de2e

 > {{{
 > […]
 > +          }else{
 > +            $('#Alert').modal('show');
 > +            $(".alert-body").html("This issue is currently being edited
 by " + issue[wiki:'locked_by' 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?

 See my solution for this here:
 https://github.com/SheriefAlaa/projectpups/commit/9a0769e5aa45040a5b2335433489d21a3024718c

 > All handlers in Stats.[wiki:ActionHandler] do the exact same thing for
 `error` and `headers`. Worth factoring out maybe.

 Completely forgot to add this to my TODO list but consider it done before
 deployment.

 > {{{
 > […]
 > 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?

 Probably a missquash (is that even a word?) :)

 Rebased and fixed.

 > {{{
 > 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[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
 > }}}
 >
 > Still putting data logic in a controller here.

 Fixed here:
 https://github.com/SheriefAlaa/projectpups/commit/58c2e0043448e5dbee3e7ccee75f7439099f1015

 > {{{
 > 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.

 This commit was removed completely since we've agreed to move to CLI
 instead of the web interface for management.

 > {{{
 > 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.

 On my TODO list.

 > {{{
 > 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?'''__'''

 Also fixed here:
 https://github.com/SheriefAlaa/projectpups/commit/58c2e0043448e5dbee3e7ccee75f7439099f1015

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/11309#comment:15>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online