Opened 6 years ago

Closed 16 months ago

#11309 closed task (fixed)

Improve how support statistics are reported

Reported by: Sherief Owned by: phoul
Priority: Medium Milestone:
Component: Community/Tor Support Version:
Severity: Normal Keywords: pups
Cc: lunar, phoul Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Interest has been expressed in making support more transparent. Publicizing how many of each type of support request the support team gets every month would help improve this. Currently lunar includes RT ticket frequency but then everyone agreed that using this is not ideal since it is hard to use an article with each ticket.

I created the frontend for a tool (see attached) that all support assistants can use at the same time, it will make use of the django app database and user accounts that webchat already uses.

Child Tickets

Attachments (1)

stats.zip (139.7 KB) - added by Sherief 6 years ago.

Download all attachments as: .zip

Change History (26)

Changed 6 years ago by Sherief

Attachment: stats.zip added

comment:1 Changed 6 years ago by mttp

Summary: Public monthly report of RT ticketsImprove how support statistics are reported

comment:2 Changed 6 years ago by Sherief

Status:

I merged stats into Pups (a Django project that will contain all support assistants tools).

What's next?

Make it possible for everyone to add/edit/view/delete the same set of questions and take care of race conditions.

comment:3 Changed 5 years ago by Sherief

Status: newneeds_review

Everything is ready except extractMonthlyReport() and wipeDB().

Please review the rest of the code.

Thanks.

comment:4 Changed 5 years ago by Sherief

Status: needs_reviewaccepted

comment:5 Changed 5 years ago by Sherief

Status: acceptedneeds_review

comment:6 Changed 5 years ago by lunar

Status: needs_reviewneeds_revision

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

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.

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.

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

+        if (data['lock_status'] == 'lock_success'){

Missing a space here.

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

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

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

-  $("[name=delete_issue]").click(function() {
+  $(document).on('click', '[name=delete_issue]', function() {

Why? Should be explained in the commit message.

+            $("#issue-" + value['pk']).append(
+                '<div id="short_issue_text-'+ value['pk']+'" name="issue_text" class="col-md-9">'
+                 +'<span id="short_sub_text-'+ value['pk']+'">'+shortText+'</span>'
+                 +more
+              +' </div>'
+              +'<input id="full_issue_text-'+ value['pk']+'" type="hidden" value="'+ value['fields'].text+'">'
+              +'<input id="user" type="hidden" value="">'
+               +'<div class="options">'
+                +'<input id="plus_one-'+ value['pk']+'" name="plus_one" class="form-control" type="text" size="4" value="'+value['fields'].f
+                +' <button id="plus-'+ value['pk']+'" name="plus_one_button" class="btn btn-default">+1</button>'
+                +' <button id="edit-'+ value['pk']+'" name="edit_issue" class="btn btn-default" data-toggle="modal">Edit</button>'
+                +' <button id="del-'+ value['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.

+    console.log("id: " + id);

Leftover debug message. Bad.

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

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

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['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

This does not feel right. You are duplicating logic here. What if the rules for expiry change one day? (e.g. it also expires if there has been more than 15 visits)

+    data['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?

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

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

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?

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.

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

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.

Last edited 5 years ago by lunar (previous) (diff)

comment:7 in reply to:  6 ; Changed 5 years ago by Sherief

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

comment:8 Changed 5 years ago by Sherief

Cc: mttp phoul removed
Status: needs_revisionneeds_review

It would be great if you try the application live on Phoul's server. I can make you an account while reviewing.

comment:9 in reply to:  7 ; Changed 5 years ago by lunar

Status: needs_reviewneeds_revision

Replying to 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?

comment:10 in reply to:  9 ; Changed 5 years ago by Sherief

Replying to lunar:

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

Done.

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

Done, once the ajax request fails pups will complain and offer you to retry reaching the server.

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

As we agreed, I've made stats a 100% webapp.

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.

Lets say if I remove token.expired_at entirely and calculated the expiry date on the fly using timezone() and settings.CONFIGexpiry_days?, would that be ok? (project wide)

I am also puzzled by "You are duplicating logic in two different places", where exactly am I duplicating logic?

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?

Done.

comment:11 in reply to:  10 ; Changed 5 years ago by lunar

Replying to Sherief:

Replying to 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.CONFIGexpiry_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.

comment:12 Changed 5 years ago by Sherief

Keywords: pups added

comment:13 in reply to:  11 Changed 5 years ago by Sherief

Replying to lunar:

Replying to Sherief:

Replying to 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.CONFIGexpiry_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

comment:14 Changed 5 years ago 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@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@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@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@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@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@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@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?

comment:15 in reply to:  14 Changed 5 years ago by Sherief

Replying to 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@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@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@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@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.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@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@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@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

comment:16 Changed 5 years ago by Sherief

Status: needs_revisionneeds_review

Please review my 'squashrebase' branch whenever you can..
https://github.com/SheriefAlaa/projectpups/commits/squashrebase

comment:17 in reply to:  16 ; Changed 5 years ago by lunar

Status: needs_reviewneeds_revision

Replying to Sherief:

Please review my 'squashrebase' branch whenever you can..
https://github.com/SheriefAlaa/projectpups/commits/squashrebase

This branch has no common ancestor with origin/master. It's not mergeable, so a proper rebase is needed before review.

Also:

$ git checkout sherief/squashrebase
$ git ls-files '*.pyc'
stats/management/__init__.pyc
stats/management/commands/__init__.pyc

Intermediate products should stay out of the Git repository.

comment:18 in reply to:  17 Changed 5 years ago by Sherief

Status: needs_revisionneeds_review

Replying to lunar:

Replying to Sherief:

Please review my 'squashrebase' branch whenever you can..
https://github.com/SheriefAlaa/projectpups/commits/squashrebase

This branch has no common ancestor with origin/master. It's not mergeable, so a proper rebase is needed before review.

Fixed that (hopefully).

https://github.com/SheriefAlaa/pups now contains 4 branches based on tpo.pups.git/master
1) The original master branch (not modified).
2) webchat_patches: contains only commits related to patching webchat.
3) stats: contains stats' implementation commits + enhancements
4) pups_patches: a merge between master + webchat_patches + stats + pups enhancements commits.

Also:

$ git checkout sherief/squashrebase
$ git ls-files '*.pyc'
stats/management/__init__.pyc
stats/management/commands/__init__.pyc

Intermediate products should stay out of the Git repository.

Fixed.

comment:19 Changed 5 years ago by lunar

Status: needs_reviewneeds_revision

Reviewing the stats branch, as it is what this ticket was about, initially.


In stats/models.py, I don't understand why the only method that is checking if an Issue is locked, is the lock method. Shouldn't delete_issue, save_edit, and plus_one also prevent a locked issue from being modified?


In the JavaScript, there is:

+        if (issue['lock_status'] == 'lock_success' || issue['locked_by'] === $.trim($("#user").text()) ){

On the Python side:

+        # Checking if issue is locked for editing by another user
+        if issue_obj.is_locked:
+            return {'locked_by': issue_obj.locked_by}
+
+        # If issue isn't used by anyone lock it
+        issue_db_row.update(is_locked=True)
+        issue_db_row.update(locked_by=user)
+        return True

Is there any client of the lock method which would be interested in making a difference between “I want to lock this issue” and “I have already locked the issue”? I would tend to think that it's not the case, and so have the Python side reply True when the issue is already locked by the current user.


In the edit_issue_ajax method:

+    if lock_status is True:
+        response_data['lock_status'] = 'lock_success'
+    elif lock_status is False:
+        response_data['lock_status'] = 'does_not_exist'
+    elif type(lock_status) is dict:
+        response_data['locked_by'] = lock_status['locked_by']

Reading this felt like the semantics of the lock method were ill-defined. A non-empty dictionary in Python is considered a “true” value. Having a dictionary and True behaving differently is prone to errors.

I would be tempted to make lock() not return anything, throw an exception if the issue does not exist, and another exception when the lock is held by another user. The latter exception could carry the username as an attribute. (But other methods in Issue will return False on non-existing issues, so that's not coherent).


statsbackupmonthly.py is meant to be a command called by a cronjob. It should be silent if everything is fine. Feel free to use a -v argument or an environment variable to make it more verbose, but the default should be no output except for errors.

+    file_name = month + '_' + str(date.today().year)

Bad idea: if I'm 2015-01-01 and want to save the report for 2014-12, what happens? :D

save_report_on_disk should let IOError get back to the UI side of the code. Printing “Disk error or lack of write permissions” is the kind of error messages that makes sysadmin sads. If you don't feel like handling the error with more details, just let the Python interpreter crash. The built-in exception handler will give more details than that.

Also using a with construct for the file handle would be nicer.

+    # Making sure the report file path exists
+    if not os.path.exists(settings.CONFIG['stats_reports_path']):
+        os.mkdir(settings.CONFIG['stats_reports_path'])

I don't feel this shoud be the responsibility of this script to create the destination directory.

+    reports = os.listdir(settings.CONFIG['stats_reports_path'])
[…]
+    # Report already backed up
+    if file_name in reports:
+        print "Report already backed up"

Wrong way of doing it: it's subject to TOCTTOU.

I'm not sure I like the interface of the script very much. Couldn't we always assume it's going to be saving the last month? Right now it feels like it's telling me it'll do something that it can't do.

One other thing. What happens if the report is saved, but then resetting the counters fail? The next time I'll try to run the script, it's going to tell me that the report has already been created. Not ideal.

+    # Reset frequency in db
+    Issue.objects.update(frequency=0)

I think this should be a dedicated method. Then the comment will not be needed anymore.


The commit message of 5086494 is either misleading or raises questions. It says “his page will refresh and unlock the issue for others to edit”. Is it really the page refresh that will unlock the issue? In that case, it's really fragile. What happens if the user gets disconnected or close their browser?

(By the way, it's customary to use the “her” pronoun when referring to an inderterminate user. You can also use the gender neutral “their” if
you prefer.)

What is the rationale for refreshing the user's page?

In the lock method:

+            if timezone.now() < locked_until:
+                return {'locked_by': issue_obj.locked_by,
+                        'expires_in': (locked_until - timezone.now()).seconds / 60}
+            else:
+                Issue.unlock(id)
+
+        # If issue isn't used by anyone, lock it
         issue_db_row.update(is_locked=True)

Maybe it's not a problem because there's proper transactional semantics, but the way it's currently done feels a little bit wrong. The previous lock is expired. So what's going to happen now is that we are going to steal the previous lock and reassign it to the current user. But here, you first unlock the issue. That leaves a window where someone else could come and claim the lock.

Last edited 5 years ago by lunar (previous) (diff)

comment:20 Changed 5 years ago by Sherief

Cc: phoul added

Phoul agreed to take over reviewing Stats.

comment:21 Changed 5 years ago by Sherief

Here's what I did:

  • updateGrid() encapsulation
  • ajax error refactoring
  • cleanNotFoundIssues()
  • used app wide constants
  • delete_issue, plus_one and save_edit: a) Checks if someone is editing the issue before performing an action. b) reports back to the user that the issue is locked and by whom.
  • If X is editing an Issue he should always be able to override the lock but not restart it (think he lost connection or OS crashed).

Here's the old commit:
https://github.com/SheriefAlaa/pups/commit/84d72adf3a323ac06d7f028aa901a8b8ec523a41

And here are the new changes:
https://github.com/SheriefAlaa/pups/commit/1ae9b672c03101233e313ed21b152f310c4c47e3

comment:22 Changed 3 years ago by isabela

Component: User Experience/Tor SupportCommunity/Tor Support

comment:23 Changed 3 years ago by phoul

Owner: changed from Sherief to phoul
Status: needs_revisionassigned

comment:24 Changed 23 months ago by teor

Severity: Normal

Set all open tickets without a severity to "Normal"

comment:25 Changed 16 months ago by phoul

Resolution: fixed
Status: assignedclosed

Closing this ticket as its specifically related to the PUPS system no longer in use.

Note: See TracTickets for help on using tickets.