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
Reviewer Review Type Date Requested Status
Thomi Richards (community) Needs Fixing
Review via email: mp+46244@code.launchpad.net

Description of the change

Got the user administration working as well as some small UI changes

To post a comment you must log in.
Revision history for this message
Thomi Richards (thomir-deactivatedaccount) wrote :

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.

review: Needs Fixing
Revision history for this message
Thomi Richards (thomir-deactivatedaccount) wrote :

Also, trunk has been updated with a code cleanup. Running pylint or pyflakes over your code is a good way to spot lines that are too long, etc.

lp:~veebers/sloecode/project-details updated
36. By Christopher Lee

Slight changes, need to merge trunk in

37. By Christopher Lee

Merge

38. By Christopher Lee

Made modifications suggested by Thomi

Also cleaned up some of the code and logic

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'sloecode/controllers/error.py'
2--- sloecode/controllers/error.py 2011-01-15 00:07:01 +0000
3+++ sloecode/controllers/error.py 2011-01-22 09:02:13 +0000
4@@ -10,7 +10,6 @@
5
6 from sloecode.lib.base import BaseController
7
8-
9 class ErrorController(BaseController):
10
11 """Generates error documents as and when they are required.
12
13=== modified file 'sloecode/controllers/project.py'
14--- sloecode/controllers/project.py 2011-01-15 00:07:01 +0000
15+++ sloecode/controllers/project.py 2011-01-22 09:02:13 +0000
16@@ -1,7 +1,7 @@
17 import logging
18
19 from pylons import request
20-from pylons.controllers.util import redirect
21+from pylons.controllers.util import redirect, abort
22 from repoze.what.plugins.pylonshq import ControllerProtector
23
24 from sloecode.lib.base import BaseController
25@@ -9,6 +9,7 @@
26 from sloecode.model.person import Person
27 from sloecode.model.membership import Membership
28 from sloecode.model.site_role import PROJECT_ADMIN
29+from sloecode.model.project_role import ProjectRole
30 from sloecode.model.meta import Session
31 from sloecode.lib.predicates import has_site_role
32 import sloecode.lib.helpers as h
33@@ -28,34 +29,78 @@
34 """
35 projects = Project.get(name=project_name)
36 existing_project = projects[0] if projects else None
37- return self.render('/project-details.html',
38- {'project': existing_project,
39- 'project_name': project_name})
40-
41+
42+ if existing_project is None:
43+ abort(404)
44+ else:
45+ return self.render('/project-details.html',
46+ {'project': existing_project})
47+
48 def manage_users(self, project_name):
49 """
50 Add/Remove users from this project.
51 """
52 all_people = Person.get()
53- return self.render('/project-manage-users.html',
54- {'all_people':all_people,
55+ # Need to check here, perhaps the project has been deleted between then
56+ # & now
57+ this_project = Project.get(name = project_name)[0]
58+
59+ if this_project is None:
60+ abort(404)
61+
62+ # Figure out whos already in this project and their role in it
63+ already_assigned = {}
64+ for user in this_project.users:
65+ already_assigned[user.person.login] = user.roles[0].role
66+
67+ for user in all_people:
68+ user.sc_already_assigned = already_assigned.get(user.login, False)
69+
70+ return self.render('/project-manage-users.html',
71+ {'all_people': all_people,
72 'project_name': project_name})
73-
74+
75 def process_manage_users(self, project_name):
76 """
77 Process the change in the users-in-projects
78 """
79+
80+ project_details = Project.get(name = project_name)[0]
81 selected_people = request.params.getall('usernames')
82
83+ # Update or remove existing users from the project
84+ for memship in project_details.users:
85+ users_role = request.params.get('user_roles_' \
86+ + memship.person.login)
87+ if memship.person.login in selected_people:
88+ if memship.roles[0].role != users_role:
89+ memship.roles[0].role = users_role
90+ h.flash("Role for %s Updated to %s Successfully"
91+ % (memship.person.login, users_role))
92+ selected_people.remove(memship.person.login)
93+ elif memship.person.login not in selected_people:
94+ Session.delete(memship)
95+ h.flash("%s Removed from Project Successfully"
96+ % memship.person.login)
97+
98+ Session.commit()
99+
100+ # Add new users to the project
101 for user in selected_people:
102- user_details = Person.get(login = user)
103- new_membership = Membership()
104- new_membership.person_id = user_details[0].id
105- new_membership.project_id = 1
106-
107+ user_details = Person.get(login = user)[0]
108+ new_membership = Membership(person = user_details,
109+ project = project_details)
110+
111 Session.add(new_membership)
112+
113+ # And setup the project roles
114+ user_project_role = ProjectRole(membership = new_membership)
115+ user_project_role.role = request.params.get('user_roles_' + user)
116+
117+ Session.add(user_project_role)
118 Session.commit()
119
120- h.flash("Added %s Successfully" % user_details[0].name)
121+ h.flash("%s Successfully Added to Project"
122+ % user_details.name)
123
124 return redirect('/p/%s' % project_name)
125
126=== modified file 'sloecode/lib/helpers.py'
127--- sloecode/lib/helpers.py 2011-01-15 00:07:01 +0000
128+++ sloecode/lib/helpers.py 2011-01-22 09:02:13 +0000
129@@ -15,11 +15,14 @@
130 # makes it easier to reorganise routes without having to touch template code:
131 from routes import url_for
132
133+# Roles information
134+from sloecode.model.project_role import PROJECT_ROLES
135+
136 # a simple link_to implementation that handles a confirm kwarg that prints
137 # very simple javascript asking the user if they really want to continue.
138 from webhelpers.html.tags import link_to as _link_to
139 def link_to(label, url, **kwargs):
140- """A patched version of the link_to function that accepts a confirm
141+ """A patched version of the link_to function that accepts a confirm
142 parameter.
143 """
144 # only add JS if the user has not already put something in onclick handler
145
146=== modified file 'sloecode/model/project_role.py'
147--- sloecode/model/project_role.py 2011-01-15 00:07:01 +0000
148+++ sloecode/model/project_role.py 2011-01-22 09:02:13 +0000
149@@ -7,6 +7,11 @@
150
151 from sloecode.model.meta import Base, QueryMixin
152
153+ROLE_OBSERVER = 'Observer'
154+ROLE_DEVELOPER = 'Developer'
155+ROLE_MANAGER = 'Manager'
156+PROJECT_ROLES = (ROLE_OBSERVER, ROLE_DEVELOPER, ROLE_MANAGER)
157+
158 class ProjectRole(Base, QueryMixin):
159 """A model that stores different project roles.
160 """
161
162=== modified file 'sloecode/public/style.css'
163--- sloecode/public/style.css 2011-01-13 07:37:49 +0000
164+++ sloecode/public/style.css 2011-01-22 09:02:13 +0000
165@@ -159,11 +159,12 @@
166 margin: 0px;
167 }
168
169+table.manage_list th,
170 .details_list dt {
171 font-weight: bold;
172 background-color: black;
173 color: white;
174- padding: 1px 4px 1px 4px;
175+ /* padding: 1px 4px 1px 4px; */
176 }
177
178 .details_list dt.action {
179@@ -176,11 +177,23 @@
180 padding-top: 2px;
181 padding-bottom: 2px;
182 }
183+
184+.details_list dd span.user_role {
185+ font-style: italic;
186+ color: silver;
187+}
188+
189 .details_list dt.action {
190 background: silver url(/plus-circle-frame.png) center left no-repeat;
191 padding-left: 18px;
192 }
193
194+/* Footer stylings */
195+#ft a {
196+ text-decoration: none;
197+}
198+
199+/* Help Stylings */
200 a.help_link {
201 text-decoration: none;
202 }
203\ No newline at end of file
204
205=== modified file 'sloecode/templates/base.html'
206--- sloecode/templates/base.html 2011-01-13 07:37:49 +0000
207+++ sloecode/templates/base.html 2011-01-22 09:02:13 +0000
208@@ -36,12 +36,6 @@
209 {{ nav.display_nav() }}
210 </div>
211
212- <!-- <div id="pagenavbar"> -->
213- <!-- <span>Create Something</span> -->
214- <!-- <span>List Something</span> -->
215- <!-- <span>Mush Something</span> -->
216- <!-- </div> -->
217-
218 </div> <!-- hd -->
219
220 <div id="bd">
221@@ -74,7 +68,7 @@
222 </div> <!-- bd -->
223
224 <div id="ft" style="border-top: 1px dotted gray; margin-top: 15px; padding-top: 5px;">
225- <span style="color: gray;"><i>Powered by <b>Sloecode</b></i></span>
226+ <span style="color: gray;"><i>Powered by <b>{{ h.link_to("Sloecode", "https://launchpad.net/sloecode") }}</b></i></span>
227 </div> <!-- ft -->
228 </div> <!-- doc -->
229 </body>
230
231=== modified file 'sloecode/templates/project-details.html'
232--- sloecode/templates/project-details.html 2011-01-15 01:41:34 +0000
233+++ sloecode/templates/project-details.html 2011-01-22 09:02:13 +0000
234@@ -3,29 +3,26 @@
235 {% block title %}Welcome{% endblock %}
236
237 {% block span_content %}
238-{% if not project %}
239-<b>No Project with the name {{ project_name }} exists.</b>
240-Would you like to {{ h.link_to('Create It', '/project/create') }}?
241-{% else %}
242 <div id='page_header'>
243 <h1>{{ project.name }}</h1>
244 <h4>{{ project.description }}</h4>
245 </div>
246-{% endif %}
247 {% endblock span_content %}
248
249+{# list of users in this project #}
250 {% block side_content %}
251-{% if project %}
252 <div class="details_list">
253 <dl>
254 <dt>Users in this Project</dt>
255 {% for member in project.users %}
256- <dd class="user">{{ h.link_to(member.person.name, "/u/%s" % member.person.login, class="user") }}</dd>
257- {% endfor %}
258+ <dd class="user">
259+ {{ h.link_to(member.person.name, "/u/%s" % member.person.login, class="user") }}
260+ <span class="user_role">{{ member.roles[0].role }}</span>
261+ </dd>
262+ {% endfor %}
263 <dt class="action">{{ h.link_to('Add/Remove Users', '/p/%s/manage_users' % project.name) }}</dt>
264 </dl>
265 </div>
266-{% endif %}
267 {% endblock %}
268
269 {% block content %}
270
271=== modified file 'sloecode/templates/project-manage-users.html'
272--- sloecode/templates/project-manage-users.html 2011-01-07 06:06:38 +0000
273+++ sloecode/templates/project-manage-users.html 2011-01-22 09:02:13 +0000
274@@ -8,22 +8,31 @@
275 <h4>For: {{ project_name }}</h4>
276 </div>
277 {% endblock %}
278-
279+{# table of users to add/remove #}
280 {% block content %}
281-{{ h.form('/p/%s/process_manage_users' % project_name) }}
282-<table width="80%" cellspacing="0" cellpadding="0" border="0">
283+{{ h.form( h.url_for(controller='/project', project_name = project_name, action='process_manage_users') ) }}
284+<table class="manage_list" cellspacing="0" cellpadding="0" border="0">
285 <tr>
286- <th>Add/Remove</th>
287- <th>Username</th>
288+ <th>Add/Remove User</th>
289+ <th>Change Role</th>
290 </tr>
291 {% for person in all_people %}
292 <tr>
293- <td> {{ h.checkbox('usernames', value=person.login, id=person.login) }}</td>
294- <td>{{ person.name }}</td>
295+ <td>
296+ {{ h.checkbox('usernames', value=person.login, id=person.login, checked=person.sc_already_assigned or False) }}
297+ {{ person.name }}
298+ </td>
299+ <td>
300+ <div>
301+ {{ h.select('user_roles_' + person.login,
302+ options=h.PROJECT_ROLES,
303+ selected_values=person.sc_already_assigned or None) }}
304+ </div>
305+ </td>
306 </tr>
307 {% endfor %}
308 <tr>
309- <td colspan="2" style="text-align: right; padding-right: 8px;">{{h.submit('update', 'Update Project')}}</td>
310+ <td colspan="2" style="background-color: silver; text-align: right; padding-right: 8px;">{{h.submit('update', 'Update Project')}}</td>
311 </tr>
312 </table>
313 {{ h.end_form }}

Subscribers

People subscribed via source and target branches