aptivate/openconsent — Issue #34: Code review comments
Author: qris
Created: 2012-02-14 12:12:18+00:00
State: open

Hi all,

I had a quick look at the OpenConsent code and I have a few suggestions:

In the global urls.py, `urlpatterns` is assigned and then appended to. A single assignment would work just as well.

The commented-out `#url(r'^public/', include('openconsent.publicweb.urls'))` could be removed.

What's the reason for all the `decision_create_snippet_info` dicts in urls.py? It seems to make sense to pass the template name into a view function, but why pass the queryset in? It might actually be wrong/out of date as it's effectively a global variable, but it's not immutable, so it might be evaluated once and cached, which would be wrong (not include new decisions).

list_detail and other function-based generic views have been deprecated in favour of class-based views.

#Codes are used to dodge translation in urls.
#Need to think of a better way to do this...

Probably all views in views.py should be protected by @login_required for security.

Some duplicate code in create_feedback() view:

return_page = unicode(decision.id)
return HttpResponseRedirect(reverse('publicweb_item_detail', args=[return_page]))
...
return_page = unicode(decision.id)
return HttpResponseRedirect(reverse('publicweb_item_detail', args=[return_page]))

Similarly in process_post_and_redirect().

Why does sorting use a form? How about django-tables? Why does it need both a form and a 'sort' parameter in object_list_by_status()?

Decision.get_fields() is unused.

Decision.unresolvedfeedback() will be inefficient if there are too many feedback items. I think you could return a boolean (let the UI code handle how it's displayed) and use a one-line database query like:

self.feedback_set.filter(resolved=False).count()

Decision.feedbackcount doesn't need to be a model method at all, could be queried directly. .all().count() is redundant, just .count() should work.

`#work in progress... remove email sending from model save`: this can be done by listening to signals: https://docs.djangoproject.com/en/dev/topics/signals/

subject_template = Template(...) could use a file instead.

JQueryUIDateWidget looks useful, can we make this a separate component (perhaps an app) to be used in other projects?