[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: 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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs