Implement an invitation based token system into webchat

added SponsorO component::user experience/tor support owner::Sherief parent::10755 priority::immediate resolution::fixed status::closed type::task labels

Trac:
Type: task to defect

Trac:
Parent: N/A to #10755 (moved)

Trac:
Description: As part of the Buoyant Otter project, one of the requirements is to provide webchat support to users.

The current plan is to use "Prodromus", which is a webchat system that uses an anonymous XMPP server as its back-end. This has all been implemented on Moschatum.

The current state of webchat is not ready for use as the interface doesn't show if a support assistant is present to answer the user.

to

As part of the Boisterous Otter project, one of the requirements is to provide webchat support to users.

The current plan is to use "Prodromus", which is a webchat system that uses an anonymous XMPP server as its back-end. This has all been implemented on Moschatum.

The current state of webchat is not ready for use as the interface doesn't show if a support assistant is present to answer the user.

Users seeking support will see a green button if a support assistant is available to answer them and red for not available. I will need some data about how many support assistants are currently connected to the server.

Trac:
Keywords: N/A deleted, SponsorO added

Trac:
Status: new to accepted
Priority: normal to blocker
Type: defect to task
Description: As part of the Boisterous Otter project, one of the requirements is to provide webchat support to users.

The current plan is to use "Prodromus", which is a webchat system that uses an anonymous XMPP server as its back-end. This has all been implemented on Moschatum.

The current state of webchat is not ready for use as the interface doesn't show if a support assistant is present to answer the user.

to

One of SponsorO's deliverables is to provide webchat support to users.

The current plan is to use "Prodromus", which is a webchat system that uses an anonymous XMPP server as its back-end. This has all been implemented on Moschatum.

It was decided during the Winter developers meeting 2014 that the best way to do this is though an invitation based token system.

Progress will be posted as comments and the code will live on github.
Summary: Implement an availability system into webchat to Implement an invitation based token system into webchat
Cc: phoul to phoul, lunar

Tweaked the description to be less general. Hope you don't mind.

Trac:
Description: One of SponsorO's deliverables is to provide webchat support to users.

The current plan is to use "Prodromus", which is a webchat system that uses an anonymous XMPP server as its back-end. This has all been implemented on Moschatum.

It was decided during the Winter developers meeting 2014 that the best way to do this is though an invitation based token system.

Progress will be posted as comments and the code will live on github.

to

In order to provide chat-based support, we need a web front-end.

It will have two sides:

Allow support assistants to create and delete invitation tokens.
Allow users to get a chat interface that will connect them to the support assistant who issued the token.

The latter part will use Prodromus (maybe with some modifications) as the JavaScript XMPP client.

Progress will be posted as comments and the code will live on github.

What's done so far:

Created a couple of tables in the database for support assistants accounts.
Authentication, Login, Logout and change password code is out of the way.
Most of the app's html is now written in django's template engine.

I plan to push the code once I clean it out a bit (maybe by the end of the week).

Most recent code can be found here. You can look at but I advise not to run it.

Also sorry if it isn't cleaned out yet.

Comments on the latest available iteration.

Why request a RT ticket number? As I am visible in the France for working on Tor, I sometimes have people reaching me through private email. Having me open a ticket or asking them to mail help-fr@ would feel like a burden. As tickets expire, I don't really see the need to tie them to a specific RT request. Is the idea to be able to sort out which token has been given to whom? Then a free-form “Comment” field that I can organize however I want is good enough.

In any cases, I don't think the edit button is useful. In case of mistake, deleting and recreating is fine. And that's less clutter in the interface.

What will “delete” do for the users? Tell that the token never existed or that it is expired? The latter might be better.

The list of tickets should be ordered in the other direction (newly created tickets on top).

When I create a new ticket, I want the new token link in big, bold letters standing out that I just can copy/paste quickly. I shouldn't need to look at the list.

Using in the list really adds a lot of clutter and does improve for copy-pasting over a simple text.

Could we have the date either in ISO form (2014-03-15) or in relative format (“in 23 hours”)? That would be easier for my brain to parse.

Also, is it by date (days) or by time (seconds)?

Could you have the

start with “Token Generator” so I can see “Tok…” instead of “Web…” when my taskbar gets really cramped? ;)

Replying to lunar:

Comments on the latest available iteration.

Why request a RT ticket number?

Because it is easier to recognize an RT ticket number faster than a 32 digits/letters token number, that way if some user is spamming my xmpp client I can just disable his token by deleting it.

As I am visible in the France for working on Tor, I sometimes have people reaching me through private email. Having me open a ticket or asking them to mail help-fr@ would feel like a burden.

I never knew about that, I though we kept all email support though RT.

As tickets expire, I don't really see the need to tie them to a specific RT request. Is the idea to be able to sort out which token has been given to whom? Then a free-form "Comment" field that I can organize however I want is good enough.

I can certainly add a comment field.

In any cases, I don't think the edit button is useful. In case of mistake, deleting and recreating is fine. And that's less clutter in the interface.

Well, I just wanted to make life easier but I can remove it.

What will "delete" do for the users? Tell that the token never existed or that it is expired? The latter might be better.

Yes, that's what I though about from the beginning.

The list of tickets should be ordered in the other direction (newly created tickets on top).

Noted.

When I create a new ticket, I want the new token link in big, bold letters standing out that I just can copy/paste quickly. I shouldn't need to look at the list.

Noted.

Using in the list really adds a lot of clutter and does improve for copy-pasting over a simple text.

Then I will just make it normal text. The reason I made it a text area is that if you triple click the link it will selected for you.

Could we have the date either in ISO form (2014-03-15) or in relative format ("in 23 hours")? That would be easier for my brain to parse.

I will add hours (23 format) and modify the date to be ISO compliant.

Also, is it by date (days) or by time (seconds)?

The end result would look like this: Created on: 13:55 2014-03-15 Expires on: 13:55 2014-03-18

Could you have the
start with "Token Generator" so I can see "Tok..." instead of "Web..." when my taskbar gets really cramped? ;)

lol, ok. :)

If I don't get any more comments I will make the changes in 2 days:

Remove RT ticket
Change to normal text
Change the date format and add hours (24h format)
Remove the edit button since there is no need for it unless we want to modify the comment field.
Order tokens from newer to older.
The last created token will be in bold.

If I don't get any more comments I will make the changes in 2 days:

Remove RT ticket

Change to normal text

Change the date format and add hours (24h format)

Remove the edit button since there is no need for it unless we want to modify the comment field.

Order tokens from newer to older.

The last created token will be in bold.

I made the above changes and created two more custom django-admin commands which are createuser and deleteuser that can be used to create and delete support assistant accounts.

Trac:
Status: accepted to needs_review

Today, lunar, phoul and I tested webchat using tokens and it was a success.

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.

Trac:
Status: needs_review to needs_revision

Any progress?

Replying to lunar:

Any progress?

Yes and I did a lot more than what you suggested but I wanted to comment when I am fully done. I'll probably post my comment later today or tomorrow morning.

Replying to lunar:

Comments after a first look at the code:

webchat/webchatapp/django.wsgi:

There is a hardcoded path and is probably missing documentation.

Useless file, deleted.

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?

Fixed the *_at part but I think allowing more than 128 chars will clutter the page.

I believe indentation is not PEP8 friendly.

Probably fixed.

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

Fixed.

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

I tried to make it better, please check it again.

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

I will add a revoked flag in the Token() model to differentiate between expiration and revoke. But I will also create a custom CLI script that the sys admin can use if one dat the db explodes or becomes too slow. Also, "Delete Selected" will be renamed as "Revoke Selected".

The q variable in get_token() should be removed.

I just didn't want to query the DB twice for performance but I removed it anyway.

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.

You can add more than one db because DATABASES is a dict of dicts and the current db is the default so we can do the following:

  {{{#!python
DATABASES = {
    'default': {
        'ENGINE': 'django.db.backends.sqlite3',
        'NAME': os.path.abspath(os.path.join(os.path.dirname( __file__ ),  'databases/pups.db')), 
        'PASSWORD': '',
        'HOST': '',
        'PORT': '',
    } ,

    'testing':
    {
      'ENGINE': ...
    }
}

}}}

Some of the boilerplate should probably be removed.

webchat/webchatapp/urls.py:

I guess the boilerplate could also be removed.

Done.

webchat/webchatapp/utils.py:

This one does not look to have much purpose.

Yes, I had the intention to use it and forgot about it.

webchat/webchatapp/views.py:

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

That's how django names controllers. It can be renamed of course but that would look odd.

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

I mitigated this.

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.

I rewrote all of change_password() and added validation.

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

Done.

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

Consider it done.

Thanks a bunch for this review, I appreciate it.

My current TODO list for webchat:

revoke flag.
chat should check if revoked and if expired.
token_page() should hide expired and revoked.
manage.py cleanexpired.
xmpp presence().
CSS/UI.

Replying to Sherief:

I will add a revoked flag in the Token() model to differentiate between expiration and revoke.

I'm not sure if that's really information of interest. As long as the token does not work anymore, I think it's fine. This could simply be done by changing the expiration date.

Replying to lunar:

Replying to Sherief:

I will add a revoked flag in the Token() model to differentiate between expiration and revoke.

I'm not sure if that's really information of interest. As long as the token does not work anymore, I think it's fine. This could simply be done by changing the expiration date.

Based on what you said I'll just change delete_token() to revoke_token() which will just modify the expiration date (eg: now - 1 day).

chat() will display either display "token doesn't exist" for invalid tokens (wrong input) or "token expired" for revoked/expired tokens.

I believe this is good enough for our needs. :)

One trick, even: the expiration date could be set equal to the creation date upon revocation. That way we can still easily do stats on the amount of token we revoke.

Replying to lunar:

One trick, even: the expiration date could be set equal to the creation date upon revocation. That way we can still easily do stats on the amount of token we revoke.

Neat! I will do that.

Okay, nothing is left except for the support assistant presence and the CSS/UI stuff. Maybe another review?

Trac:
Status: needs_revision to needs_review

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?

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.

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.

wsgi.py contains some boilerplate at the end that should probably be removed.

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

I don't understand the meaning of the success variable Token.revoke_token().

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.

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 []

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.

In webchat/views.py:

change_password(), create_token(), revoke_token(), chat() all have if …: return …; else constructs.

For change_password() is there an error message when the form is not valid?

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')),

In tokens_page(), params is not used before the render() call at the end. It should be moved closer to that.

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?

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.

Please make the code PEP8 compliant unless there's a good reason for it:

$ pep8 pups_project | wc -l
91

Replying to 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. :)

Replying to Sherief:

Replying to 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.

Why not simply pups?

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)

Then I believe the function name should be changed too.

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.

What if an attacker manage to add data to the DB without going through Django's validation process?

Replying to lunar:

Replying to Sherief:

Replying to 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.

Why not simply pups?

That's doable but I will also change "pups" the app that handles accounts to "accounts".

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)

Then I believe the function name should be changed too.

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.

What if an attacker manage to add data to the DB without going through Django's validation process?

That's not even possible because:

token_page() is decorated with @login_required.
you cannot access create_token() because it's not mentioned in urls.py like token_page() and login().

Replying to Sherief:

What if an attacker manage to add data to the DB without going through Django's validation process?

That's not even possible because:

token_page() is decorated with @login_required.

you cannot access create_token() because it's not mentioned in urls.py like token_page() and login().

An attacker could gain direct access to the SQL database.

Replying to lunar:

Replying to Sherief:

What if an attacker manage to add data to the DB without going through Django's validation process?

That's not even possible because:

token_page() is decorated with @login_required.

you cannot access create_token() because it's not mentioned in urls.py like token_page() and login().

An attacker could gain direct access to the SQL database.

I am using sqlite, I am not sure how can an attacker get access to that unless he has access to the VM. And what does that have to do with cleaning the comment input before submitting to the database?

Anyway, I will use django.db.models.Model.full_clean to clean the data before submission.

Status:

Presence is done and supports custom messages eg: "I'll be available 16:00 UTC".

Trac:
Status: needs_review to needs_revision

A note about the new Token revocation method ( Model, Controller ):

Instead of doing commit/rollback I just updated tickets regardless because we are not deleting data to begin with so commit/rollback didn't make sense.

Status:

Progress bar, check.
Branding, check (big thanks to lunar).
CSS/HTML alert boxes instead of alert();, check.

What's left:

Javascript cleanup.
Comment tooltip.
A staging environment.
A nice frontpage.

Lunar, please include prodromus.js in your review, thanks.

Trac:
Description: In order to provide chat-based support, we need a web front-end.

It will have two sides:

Allow support assistants to create and delete invitation tokens.
Allow users to get a chat interface that will connect them to the support assistant who issued the token.

The latter part will use Prodromus (maybe with some modifications) as the JavaScript XMPP client.

Progress will be posted as comments and the code will live on github.

to

In order to provide chat-based support, we need a web front-end.

It will have two sides:

Allow support assistants to create and revoke invitation tokens.
Allow users to get a chat interface that will connect them to the support assistant who issued the token.

The latter part will use Prodromus (maybe with some modifications) as the JavaScript XMPP client.

Progress will be posted as comments and the code will live on github.

We've run into a couple of issues creating a staging and production environments on the same vm due to mod_wsgi's WSGIPythonPath limitations. It can take more than one path but it always prefers the first, to resolve this I had to run mod_wsgi's WSGIDaemonProcess which requires a python path for each project and to do that I had to use virtualenv (found on debian stable).

The problem arises when I get into that virtual python environment which needs me to install django though pip (which comes pre-installed with virtualenv) for pups to work inside it.

The apache config would look like this:

<VirtualHost *:80>
    ServerName webchat.torproject.org

    WSGIDaemonProcess pups processes=5 python-path=/home/sherief/prod_env/pups:/home/sherief/prod_env/lib/python2.7/site-packages threads=1
    WSGIProcessGroup pups
    WSGIScriptAlias / /home/sherief/prod_env/pups/pups/wsgi.py

    Alias /static/ /home/sherief/prod_env/pups/static/ 

    <Directory /home/sherief/prod_env/pups/static> 
      Order deny,allow
      Allow from all
    </Directory>

</VirtualHost>

<VirtualHost *:80>
    ServerName webchat-staging.torproject.org

    WSGIDaemonProcess pups-staging processes=5 python-path=/home/sherief/stag_env/pups:/home/sherief/stag_env/lib/python2.7/site-packages threads=1
    WSGIProcessGroup pups-staging
    WSGIScriptAlias / /home/sherief/stag_env/pups/pups/wsgi.py

    Alias /static/ /home/sherief/stag_env/pups/static/ 

    <Directory /home/sherief/stag_env/pups/static> 
      Order deny,allow
      Allow from all
    </Directory>
</VirtualHost>

After finding out that pip is insecure from phoul I had to ditch this setup and ask him for an alternative.

To have multiple instances of django (for a staging environment) we will need to run multiple instances of Apache2(3). We will need one instance for the production deployment of pups, one for the staging deployment, and one instance for a proxy to route traffic to the desired pups deployment.

The reason for doing the multiple instances of Apache2 is that WSGIPythonPath cannot be defined per-vhost, unless you start using pip & virtualenv as Sherief noted above. This method does not require either, and should hopefully be acceptable for deployment on Moschatum.

See below for example ServerConfigFiles.

pups.conf - https://gist.github.com/Phoul/fec7d65492db6bc7a35b pups-staging.conf - https://gist.github.com/Phoul/624575fc89a33043b312 pups-proxy.conf - https://gist.github.com/Phoul/23abe67f91a92f173cc3

Another review:

Please use a commonly accepted license. That will prevent legal headaches. If you want to give freedom to developers, BSD-3-clause like Tor is just fine. If you want to give freedom to users, AGPLv3 is what we need. Actually prodomus.js is licensed under AGPLv3 already.

The createuser command should exit with a non-zero status code when unable to create an user. Same with deleteuser.

The pups/settings.py should not be commited to the repository and should probably be in .gitignore.

Page title in HTML templates in pups/templates might need to be updated to reflect the new application name.

pups/templates/nav_bar.html contains a commented tag without explanations in the header.

pups/templates/pups.html says “Nothing interesting here yet but I'll think of something.”

License of Bootstrap files should be mentioned in the README. Same with the webfont. Same with jQuery. Same with Strophe. Same with Prodomus.

prodromus.js contains non internationalizable strings (e.g. VARIABLE + "something"). Sprintf-like functions should be used instead.

In promodus.js, isAvailable is ill-named. It should not be named isSomething if it does not hold a boolean value. Right now, this variable can have four different values "null", "0", "1", and "2". Their actual meaning is unclear by reading the source code. Constants should be used instead of magic numbers. Usage of "null" on top of the actual values does not look like a good idea.

extra <div> in pups_project/webchat/templates/prodromus.html

The LoginForm defined in webchat/forms.py should probably be in the pups module instead.

Token.revoke_token should be named revoke_tokens as it takes a list.

The code is still not following PEP8 in many places:

$ pep8 . | wc -l
109

I could also work on the code if needed.

Replying to lunar:

Another review:

Please use a commonly accepted license. That will prevent legal headaches. If you want to give freedom to developers, BSD-3-clause like Tor is just fine. If you want to give freedom to users, AGPLv3 is what we need. Actually prodomus.js is licensed under AGPLv3 already.

The createuser command should exit with a non-zero status code when unable to create an user. Same with deleteuser.

The pups/settings.py should not be commited to the repository and should probably be in .gitignore.

Page title in HTML templates in pups/templates might need to be updated to reflect the new application name.

pups/templates/nav_bar.html contains a commented tag without explanations in the header.

pups/templates/pups.html says “Nothing interesting here yet but I'll think of something.”

License of Bootstrap files should be mentioned in the README. Same with the webfont. Same with jQuery. Same with Strophe. Same with Prodomus.

prodromus.js contains non internationalizable strings (e.g. VARIABLE + "something"). Sprintf-like functions should be used instead.

In promodus.js, isAvailable is ill-named. It should not be named isSomething if it does not hold a boolean value. Right now, this variable can have four different values "null", "0", "1", and "2". Their actual meaning is unclear by reading the source code. Constants should be used instead of magic numbers. Usage of "null" on top of the actual values does not look like a good idea.

extra <div> in pups_project/webchat/templates/prodromus.html

The LoginForm defined in webchat/forms.py should probably be in the pups module instead.

Token.revoke_token should be named revoke_tokens as it takes a list.

The code is still not following PEP8 in many places: {{{ $ pep8 . | wc -l 109 }}}

I could also work on the code if needed.

Nah, it's not a problem. I can finish this in less than an hour or so.

Thanks for the review.

The issue with requiring multiple instances of Apache2 can be resolved with Passenger(thanks Lunar). So it is possible to have the production environment & the staging environment running off the same instance of Apache2 (provided we can get libapache2-mod-passenger installed, which is available in Debian Wheezy).

Replying to phoul:

The issue with requiring multiple instances of Apache2 can be resolved with Passenger(thanks Lunar). So it is possible to have the production environment & the staging environment running off the same instance of Apache2 (provided we can get libapache2-mod-passenger installed, which is available in Debian Wheezy).

I don't think this is right or I just don't understand how it works.

I don't see any databases other than pups/databases/pups.db, how can the staging environment have its own separate db and code in this setup?

Copying my response on IRC below.

Not sure I fully understand your question, but check out the .htaccess files under /var/www & /var/www2, as well as the passenger_wsgi.py files in pups & pups-staging. if those dont answer your question, please let me know :) Each instance does appear to be using its own DB.

Pups is ready and uploaded to tpo's git.

Trac:
Resolution: N/A to fixed
Status: needs_revision to closed

closed

mentioned in issue #11881 (moved)

Implement an invitation based token system into webchat

Child items ...

Activity