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

[Tor Bug Tracker & Wiki]

#11309: Improve how support statistics are reported
-----------------------------+----------------------------
     Reporter:  Sherief      |      Owner:  Sherief
         Type:  task         |     Status:  needs_revision
     Priority:  normal       |  Milestone:
    Component:  Tor Support  |    Version:
   Resolution:               |   Keywords:  pups
Actual Points:               |  Parent ID:
       Points:               |
-----------------------------+----------------------------
Changes (by lunar):

 * status:  needs_review => needs_revision


Comment:

 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
 [https://en.wikipedia.org/wiki/Time_of_check_to_time_of_use 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.

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