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

[Tor Bug Tracker & Wiki]

#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:11 lunar]:
 > Replying to [comment:10 Sherief]:
 > > Replying to [comment:9 lunar]:
 > > > > > > {{{
 > > > > > > 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.
 > >
 > >
 > > Lets say if I remove token.expired_at entirely and calculated the
 expiry date on the fly using timezone() and
 settings.CONFIG['expiry_days'], would that be ok? (project wide)
 >
 > No. Exchanging code for a configuration file is not interesting here. At
 least until there are more users to the code. And then, it would probably
 still be better to use plain Python.
 >
 > > I am also puzzled by "You are duplicating logic in two different
 places", where exactly am I duplicating logic?
 >
 > This is just a method to report numbers. But this method contain logic
 which
 > decide “Is this token live?”, “Was this token revoked?”. That logic does
 not
 > belong in a reporting function.
 >
 > Also, now that I read it more careful, I don't see why you are testing 3
 cases when the last one is implied by the 2 previous ones.

 I've wrote an improved version [0] that uses the database instead. After
 reviewing I can squash it with 5afc6340aa2b7b7c6e3d3e024efd7f9214ed747d
 [1].

 All in all the whole thing is ready for review, please do when you have
 the time.

 [0]
 https://github.com/SheriefAlaa/projectpups/commit/96fd52bd2956b5eb41e69b53f882771bb65031b5
 [1]
 https://github.com/SheriefAlaa/projectpups/commit/5afc6340aa2b7b7c6e3d3e024efd7f9214ed747d

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