[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

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



#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:               |
-----------------------------+----------------------------

Comment (by Sherief):

 Replying to [comment:6 lunar]:


 > 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/>`?
 >
 Done.

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

 I replaced the modal dialog with more or less <a> links.

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

 I capitalized it.

 > > {{{
 > > +      success: function(data) {
 > > +      // if unlocked
 > > +        console.log(data);
 > > +        console.log(data[wiki:'locked_by' 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.


 I am not sure what do you want me to do right here, hide a member string
 function?

 For example:

 get_id(delimiter, pos){
     // logic here
     return id
 }

 > > {{{
 > > +        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
 >
 > > {{{
 > > +          $('#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.

 Switched to use constants.

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

 It will keep sending requests indefinitely until the server replies.

 > > {{{
 > > +  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?

 Why it changed is because we have more space in the div but that doesn't
 matter anymore since I switched to a more/less links instead of the modal
 dialog. Also, switched to const.

 > > {{{
 > > -  $("[name=delete_issue]").click(function() {
 > > +  $(document).on('click', '[name=delete_issue]', function() {
 > > }}}
 > >
 >
 > Why? Should be explained in the commit message.

 Yes, I agree. .click() doesn't work on dynamically created elements so I
 had to switch to .on()

 > > {{{
 > > +            $("#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

 > > {{{
 > > +    console.log("id: " + id);
 > > }}}
 > >
 >
 > Leftover debug message. Bad.
 >
 :( removed.

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

 > > {{{
 > > +    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?
 >

 Changed to use static methods (we spoke about this one already sometime
 back).

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

 Should we really be changing expiry days? that just creates more work (we
 will need to change every single expiry date in the db. I don't like it at
 all.

 An easier solution is to remove the db column `expired_at` and just do a +
 expiry_days on
 created_at to calculate the expiry_at date

 > (e.g. it also expires if there has been more than 15 visits)

 wait.. do you want a token to expire after X visits?

 > > {{{
 > > +    data[wiki:'frequent_issues' 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?

 I am currently working on the unifying the css of pups instead of putting
 random style="" all over the place.

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

 > > {{{
 > > +    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?

 Fixed.

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

 In progress.

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

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

 Fixed.

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

 That changed entirely and have been moved to pups/management.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/11309#comment:7>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs