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

Re: [tor-bugs] #10754 [Tor Support]: Implement an invitation based token system into webchat

#10754: Implement an invitation based token system into webchat
-----------------------------+----------------------------
     Reporter:  Sherief      |      Owner:  Sherief
         Type:  task         |     Status:  needs_revision
     Priority:  blocker      |  Milestone:
    Component:  Tor Support  |    Version:
   Resolution:               |   Keywords:  SponsorO
Actual Points:               |  Parent ID:  #10755
       Points:               |
-----------------------------+----------------------------
Changes (by lunar):

 * status:  needs_review => needs_revision


Comment:

 Comments after a first look at the code:

 `webchat/webchatapp/django.wsgi`:

 There is a hardcoded path and is probably missing documentation.

 `webchat/webchatapp/models.py`:

 Sorry to be ignorant of Django idioms. In Rails world, we would call the
 columns `created_at` and `expire_at`.  Why not make `comment` a TEXT
 field?

 I believe indentation is not PEP8 friendly.

 `if <something>: return True; else: return False` constructions are
 usually redundant. `return q.t_id is not None` is nicer to read.

 `delete_token()` has weird semantics. There is no way a caller can
 properly handle failures.

 I had the impression that tokens would only expire and not be deleted so
 that users would be able to know that a token has been revoked

 The `q` variable in `get_token()` should be removed.

 `webchat/webchatapp/settings.py`:

 Path to `webchatDB/webchatDB.db` should be configurable. Probably through
 an environment variable. At least we need an easy way to configure a
 staging and a production environment.

 Some of the boilerplate should probably be removed.

 `webchat/webchatapp/urls.py`:

 I guess the boilerplate could also be removed.

 `webchat/webchatapp/utils.py`:

 This one does not look to have much purpose.

 `webchat/webchatapp/views.py`:

 The name of this file is weird. In the MVC model, it contains controllers.

 `if something: return; else: â` is bad style. The `else` can be avoided
 entirely as the control flow will return anyway.

 The control flow of `change_password` is hard to follow.  See remark just
 above.

 One should always redirect when an action has been successful. Otherwise,
 clicking ârefreshâ will trigger the action again.

 `token_page()` needs to be splitted up. One function by action.

 `chat()` should make the distinction between an expired token and a non-
 existing token.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/10754#comment:15>
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