[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