[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