FinalsClub / karmaworld

KarmaNotes.org v3.0
GNU Affero General Public License v3.0
7 stars 6 forks source link

some model paths are constructed as strings #325

Closed btbonval closed 10 years ago

btbonval commented 10 years ago

Rather than strings, Model.get_absolute_url() should be called. https://docs.djangoproject.com/en/1.5/ref/models/instances/#django.db.models.Model.get_absolute_url

For example, this should call self.object.get_absolute_url rather than construct it as a string. https://github.com/FinalsClub/karmaworld/blob/3686a4064ca8f37281f1b8c45193dff7553ce4cc/karmaworld/apps/courses/views.py#L49-L51

Additionally, models should define get_absolute_url in terms of reverse wherever possible rather than defining by string. The suggestion for this is listed in the Django Docs above. https://docs.djangoproject.com/en/1.5/ref/urlresolvers/#django.core.urlresolvers.reverse

An example of where a model might implement reverse for get_absolute_url: https://github.com/FinalsClub/karmaworld/blob/f9d44c70c9457969fa05e09c8b03af4bd7a08b0c/karmaworld/apps/courses/models.py#L249-L251

reverse might not work with regular expressions, but in the above case, there is no reason to include the school in the URL at all. Might as well be /course/<course_id> in urls.py rather than /<school_id>/<course_id>

btbonval commented 10 years ago

Hierarchical URLs like /<school>/<course>/ don't add much because each course slug is unique across all courses regardless of school. Generally, each model is identified by a similar unique and non-dependent slug.

btbonval commented 10 years ago

The regular expressions have variables in them. Pass in variables with reverse('url-code-name', kwargs={'variable': value}). It's a bit verbose. See above commit (dda1663) for an example.

charlesconnell commented 10 years ago

Pretty sure there's no more places we are doing this in the code.