Code review comment for lp://staging/~benoit.pierre/sloecode/incubation

Revision 129 & 130 are fine.

Revision 131 looks fine, but I'm a bit concerned about revision 132. I understand that you need to add new users to the sloecode database once they're authenticated, but surely there's a hook in the repoze PAM authentication plugin that can be used for this? In fact, I think it can be easily done by specialising the PAMAuthenticatorPlugin and overriding the authenticate(...) method such that successful authentications are then inserted into the database, if they don't already exist. See:

http://docs.repoze.org/who/1.0/narr.html#writing-an-authenticator-plugin

for more information. Also, while you're doing this, I think it makes sense to set the password in the database correctly - that way one can switch away from PAM authentication in the future and existing accounts will continue to work. Once you're making the change listed above, this should be trivial to implement.

Revisions 133, 134, 135 look fine.

Revision 136: the public_projects() function in sloecode/model/project.py should really use a SQLAlchemy query to get all the public projects, rather than getting all projects and iterating over them. You should be able to do something similar to:

def public_projects():
    return Project.get(public=True)

Unfortunately, whenever we make a database schema change we need to also write a database patch file that is able to alter a live database without deleting the current data. These are simple SQL scripts that make the appropriate changes, and are run as part of the debian package's upgrade scripts. See the existing ones in 'dbpatches/patches/*/' - you need to write one for mysql and one for SQLite. SQLite is a bit more tricky because it doesn't let you alter tables much, but you should be able to adapt an existing patch file for your needs. I'd suggest adding the 'public' column, and setting all projects to private by default.

Also, it would be nice to have some text explaining what a public project is in the template - either as a short string next to the 'Public' checkbox, or as a help page. Something that explains that public projects are readable by anyone with access to the sloecode server, but write access is still restricted to developers and managers. I also wonder whether we should not allow the addition of 'Observers' to a public project - then again, that UI needs to be re-done anyway, so maybe we'll deal with that later.

Finally, this isn't a bug that you've introduced, but in the nav.html file, projects should be listed alphabetically, in a case-insensitive fashion. If you're able to make that change without too much trouble that would be superb!

Rev 137 is great, and has already been merged with trunk.

Rev 138 needs more thought. I'd like users to have some control over whether their branches are public or private.... or maybe this is a config setting?

Revision 139 looks OK, but maybe thumper can comment on this?

Revision 140 - I'd like some more information on the change - it looks potentially good, but I can't see any change in output on my test system... In any case we'd need to get rid of the 'if False' thing ;)

Revision 141 looks good.

Revision 142: Same comment as rev 140 - I'd like some more information on what this code does...

Finally, if you're using PAM authentication, how do you change the admin password from it's initial, insecure setting?

There's a whole lot of good work here - I'd love to merge as much of it as possible, but a few areas need cleaning up or need more work. Thanks for your help!