[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_review
Priority: blocker | Milestone:
Component: Tor Support | Version:
Resolution: | Keywords: SponsorO
Actual Points: | Parent ID: #10755
Points: |
-----------------------------+--------------------------
Comment (by Sherief):
Replying to [comment:26 lunar]:
> Is it really needed to have a `pups_project` sub-directory? Probably
related question: shouldn't be the `stats` and `webchat` modules be sub-
modules of the `pups` module?
No. I can just name the repo pups_project and remove the extra folder.
> Is `settings.py` meant to be configured by the local administrator? Then
it should probably not be commited as is. Something like
`settings_sample.py' should be in the repository instead and we should
expect administrators to copy and adjust before running the application.
It's worth being documented in the README.
Added a `settings_sample.py` and will document it later.
> On the topic, could it be documented how to configure Apache to run both
a staging and a production environment from the same codebase? This looks
unfortunately more complicated than it should:
> https://docs.djangoproject.com/en/dev/howto/deployment/wsgi
/#configuring-the-settings-module
> https://stackoverflow.com/a/11515629
> Looks like WSGI daemon mode and multiple âwsgi.pyâ files are required.
This is on my TODO list but I will have to prioritize functionality for
the time being.
> `wsgi.py` contains some boilerplate at the end that should probably be
removed.
Done.
> `ChangePassForm.change_password()` control flow should be inverted (so
that the âhappy pathâ can be easily seen):
> {{{
> if not new_pass_is_right():
> return False
> do_something_when_pass_is_right()
> return True
> }}}
Done.
> I don't understand the meaning of the `success` variable
`Token.revoke_token()`.
While working I was thinking about a way to report a delete failure and
forgot to continue writing the code in the function.
> The semantics of `Token.revoke_token()` are still weird. I can pass it a
list of token great. If it returns True, then I know that all the list was
revoked, great. But when it returns False? I know that there's one in the
list that was not good. But I don't know which one and with the current
code, the ones after that will not have been modifiedâ Not good. I'm not
sure if Django handles database COMMIT/ROLLBACK properly, but if that's
the case, the best course of action is to have the function raise an
Exception so the previous changes are rolled back.
I understand a database transaction is the best way to go here, thanks for
reminding me. I will put this one on my TODO list and comment later when I
am done.
> Sorry we misunderstood each others for `Token.get_token()`. It should
> read:
> {{{
> def get_token(self, token):
> try:
> return Token.objects.get(token=token)
> except ObjectDoesNotExist:
> return []
> }}}
Nah, I was just stupid :D
> `webchat/urls.py` contains mapping for `/chpass` and `/logged` URLs
which do not belong to the webchat part of Pups. Same with
`webchat/views.py`, so things might require to be moved around some more.
Or everything merged together, that's probably fine given the current
size.
Django offers you to create a project that contains many apps inside, each
app is supposed to do one thing and do it good. For example: a blog can go
up to 12 apps (Comment, Post, View, Admin, Captcha, etc..)
Pups_Project is the main project that contains pups (handles login,
logout, changePassword and user creation and deletion), webchat (contains
token_page and prodromus) and finally stats.
I preferred moving things instead of merging to follow the Django's best
practices.
> In `webchat/views.py`:
>
> `change_password()`, `create_token()`, `revoke_token()`, `chat()` all
have `if â: return â; else` constructs.
Fixed that (I hope so) and I am loving the "happy path" :)
> For `change_password()` is there an error message when the form is not
valid?
It's not obvious but validation is handled well.
> This should really be turned into its own method for readability:
> {{{
> token.get_assistant_tokens(User.objects.get(id =
request.user.id)).filter(expires_at__gt=F('created_at')),
> }}}
I just added the `.filter(expires_at__gt=F('created_at'))` part to
`models.Token.get_assistant_tokens(assistant)`
> In `tokens_page()`, `params` is not used before the `render()` call at
the end. It should be moved closer to that.
Done.
> Why use `HttpResponse` in `chat()` and not a proper template? Also can
it be made that the HTTP error codes are 404 for invalid tokens and 410
for expired ones?
I was just lazy but now I created a proper template for each.
> Unless I'm mistaken `webchat/templates/tokens.html` directly contain the
value of `token.comment`. It should be escaped to be displayed in an HTML
context, otherwise that's a security issue.
I tried to add html tags, sql code but non worked since Django's ORM
checks things before adding data into the db automatically and
render(request, template, context)'s context handles what you mean.
Anyway, there is a models cleaning function I can use models.full_clean()
`https://docs.djangoproject.com/en/1.4/ref/models/instances/#django.db.models.Model.full_clean`
> Please make the code PEP8 compliant unless there's a good reason for it:
> {{{
> $ pep8 pups_project | wc -l
> 91
> }}}
On my TODO list.
Again, Thank you for the review. :)
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/10754#comment:27>
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