Merge lp:~veebers/sloecode/project-details into lp:sloecode
Proposed by
Christopher Lee
Status: | Merged |
---|---|
Merged at revision: | 39 |
Proposed branch: | lp:~veebers/sloecode/project-details |
Merge into: | lp:sloecode |
Diff against target: |
313 lines (+106/-41) 8 files modified
sloecode/controllers/error.py (+0/-1) sloecode/controllers/project.py (+59/-14) sloecode/lib/helpers.py (+4/-1) sloecode/model/project_role.py (+5/-0) sloecode/public/style.css (+14/-1) sloecode/templates/base.html (+1/-7) sloecode/templates/project-details.html (+6/-9) sloecode/templates/project-manage-users.html (+17/-8) |
To merge this branch: | bzr merge lp:~veebers/sloecode/project-details |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Thomi Richards (community) | Needs Fixing | ||
Review via email:
|
Description of the change
Got the user administration working as well as some small UI changes
To post a comment you must log in.
A few comments:
in sloecode/ controllers/ project. py, lines 30-34, you re-retrieve the project in line 30, in case it has been deleted between the last request and this one (good!), but then on line 34 you don't check whether 'this_project' is None or not, leaving yourself open to a potential crash.
In the same file, line 37, I'm a bit concerned about the number of parameters being passed in. For e.g- instead of passing in 'project_name' and 'already_assigned', why not just pass in the project? That way we can change the template code later (if we need to), and it already has a reasonable access point into the DB model. In fact, I'd be tempted to just pass in the project and have done with it. The USER_ROLES list should probably be added to the helpers module (and used from there).
In the same file, line 48, why not use the convenience method Project.get? I'm trying to avoid having sqlalchemy queries unless we really need them, and prefer instead to use the QueryMixin.get method (it looks a lot cleaner IMO).
Line 58 & 59: can users have more than one role within a project? I don't know. Also, I think the flash message could be clearer. SOmething like 'USERNAME's role updated to ROLE for this project'
Lines 60 & 61: 'list.remove( value)' is the same as 'i = list.index(value); del list[i]' - as long as the value exists once. Check out 'help(list. remove) '.
Line 64: make flash message more descriptive.
Line 72: Turns out you can also do this: 'mem = Membership( person= p, project=proj)'. I.e.- you can assign Person and Project objects, and SQLAlchemy is smart enough to pick out the PKs. Saves a bit of typing and makes it a bit more readable.
In sloecode/ model.project_ role.py, line 105: Can we rename USER_ROLES to PROJECT_ROLES please?
in sloeocode/ public/ style.css lines 153 - 164 - ???? I assume you meant to remove these. Looks like the result of a merge.
In sloecode/ templates/ project- manage- users.html, line 214: Whenever generating URLs for templates, or anywhere else, please use h.url_for. In this case, it would be something like:
h.url_for( controller= '/project' , action= 'process_ manager_ users', project_ name=project_ name).
This allows us to move and re-configure routes mapping without having to change any code. Check out the admin template files in trunk for more examples.