Skip to content
This repository has been archived by the owner on Mar 5, 2021. It is now read-only.

Dry url issue69 #71

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion arkestra_utilities/mixins.py
@@ -1,5 +1,5 @@
from django.db import models

from django.core.urlresolvers import reverse
from links.models import ExternalLink


Expand All @@ -21,6 +21,8 @@ def __unicode__(self):
def get_absolute_url(self):
if self.external_url:
return self.external_url.url
elif self.url_path in ["news","event","vacancy","studentship"]:
return reverse(self.url_path,kwargs={"slug":self.slug})
else:
return "/%s/%s/" % (self.url_path, self.slug)

Expand Down
22 changes: 15 additions & 7 deletions contacts_and_people/models.py
Expand Up @@ -8,7 +8,7 @@
from django.contrib.auth.models import User
from django.template.defaultfilters import slugify
from django.conf import settings

from django.core.urlresolvers import reverse
from cms.models import Page, CMSPlugin
from cms.models.fields import PlaceholderField

Expand Down Expand Up @@ -98,7 +98,7 @@ def __unicode__(self):
return building_identifier

def get_absolute_url(self):
return "/place/%s/" % self.slug
return reverse("contact_place", kwargs={"slug":self.slug})

def save(self):
if not self.slug or self.slug == '':
Expand Down Expand Up @@ -317,9 +317,9 @@ def get_absolute_url(self):
if self.external_url:
return self.external_url.url
elif self.get_website:
return "/contact/%s/" % self.slug
return reverse("contact", kwargs={"slug":self.slug})
else:
return "/contact/"
return reverse("contact_base")

@property
def _get_real_ancestor(self):
Expand Down Expand Up @@ -412,12 +412,20 @@ def get_related_info_page_url(self, kind):

If the entity is the base entity, doesn't add the entity slug to the URL
"""
kinds = ["contact","news-and-events","vacancies-and-studentships","forthcoming-events","news-archive","previous-events"] # "publications"
if self.external_url:
return ""
elif self == Entity.objects.base_entity():
return "/%s/" % kind
if kind in kinds:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I understand the purpose of this. Since, if kind in kinds is False we will use "/%s/" % kind anyway, and since that will work just the same even if kind in kinds is True, why do we need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If kind is not in the list then it probably wont get proper reverse so it has to have fallback "/%s/" % kind. I didn't know if and when you'll pull this code into origin repo and if you define some other kind I still want that part to work without debugging. Looking to it now it would be better to write:

try:
    url = reverse(kind+"_base") 
except NoReverseMatch:
    url = "/%s/" % kind 

If and when choose to use reverse everywhere then it is not needed anymore.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it will be better to move towards using reverse everywhere.

If I understand correctly, reverse will match all url patterns in the system, so as long as - for example - the Publications application we use has named url patterns that conform to the same scheme it will work with them too.

url = reverse(kind+"_base")
else:
url = "/%s/" % kind
else:
return "/%s/%s/" % (kind, self.slug)
if kind in kinds:
reverse(kind,kwargs={"slug":self.slug})
else:
url = "/%s/%s/" % (kind, self.slug)
return url

def get_template(self):
"""
Expand Down Expand Up @@ -595,7 +603,7 @@ def get_absolute_url(self):
if self.external_url:
return self.external_url.url
else:
return "/person/%s/" % self.slug
return reverse("contact_person", kwargs={"slug":self.slug})

@property
def get_role(self):
Expand Down
Expand Up @@ -47,7 +47,7 @@
<h{{ IN_BODY_HEADING_LEVEL }}>All people A-Z by surname</h{{ IN_BODY_HEADING_LEVEL }}>
<ul class= "index">
{% for initial in initials_list %}
<li><a href="/people/{{ entity.slug }}/{{initial|lower|urlencode}}/">{{ initial }}</a></li>
<li><a href="{% url contact_people_letter entity.slug,initial|lower %}">{{ initial }}</a></li>
{% endfor %}
</ul>
{% endif %}
Expand Down
Expand Up @@ -5,18 +5,18 @@
Lists members of an entity - either all people by surname, or surnames beginning {{ letter }}.

{% endcomment %}
{% load entity_tags %}
{% load i18n entity_tags %}
<div class="row columns2">
<div class="column firstcolumn">
<h{{ IN_BODY_HEADING_LEVEL }} style="clear:both">{% if letter %}People: surnames beginning {{letter|upper}}{% else %}All people by surname{% endif %}</h{{ IN_BODY_HEADING_LEVEL }}>
<h{{ IN_BODY_HEADING_LEVEL }} style="clear:both">{% if letter %}{% trans "People: surnames beginning" %} {{letter|upper}}{% else %}{% trans "All people by surname" %}{% endif %}</h{{ IN_BODY_HEADING_LEVEL }}>
{% people_with_roles letter %}
</div>
<div class="column lastcolumn">
{% if initials_list %}
<h{{ IN_BODY_HEADING_LEVEL }}>Index of people by surname</h{{ IN_BODY_HEADING_LEVEL }}>
<h{{ IN_BODY_HEADING_LEVEL }}>{% trans "Index of people by surname" %}</h{{ IN_BODY_HEADING_LEVEL }}>
<ul class="index">
{% for initial in initials_list %}
<li><a href="/people/{{ entity.slug }}/{{initial|lower}}/">{{ initial }}</a></li>
<li><a href="{% url contact_people_letter entity.slug initial|lower %}">{{ initial }}</a></li>
{% endfor %}
</ul>
{% endif %}
Expand Down
16 changes: 9 additions & 7 deletions contacts_and_people/urls.py
@@ -1,20 +1,22 @@
from django.conf.urls.defaults import patterns, include, url

urlpatterns = patterns('',
urlpatterns = patterns('contacts_and_people.views',

# person
(r"^person/(?P<slug>[-\w]+)/(?P<active_tab>[-\w]*)/?$", "contacts_and_people.views.person"),
(r"^person/(?P<slug>[-\w]+)/?$", "person", {}, "contact_person"),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the advantage of splitting up a pattern that could catch URLs with or without a tab into two different ones?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there were only one pattern i.e.

(r"^person/(?P<slug>[-\w]+)/(?P<active_tab>[-\w]*)/?$", "person", {}, "contact_person_tab")

then calling

reverse("contact_person_tab", kwargs={"slug":"foo","active_tab":"bar"})

would return '/person/foo/bar', but calling

reverse("contact_person_tab", kwargs={"slug":"foo"})

would return NoReverseMatch

OTH calling

reverse("contacts_and_people.views.person", kwargs={"slug":"foo"})

would return '/person/foo', so if the view name is used instead of the pattern name optional arguments are resolved correctly but at this moment reverse is called by name and in some cases it resolves the pattern name dynamically.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, good, I think I understand this now.

(r"^person/(?P<slug>[-\w]+)/(?P<active_tab>[-\w]*)/?$", "person", {}, "contact_person_tab"),

# place
(r"^place/(?P<slug>[-\w]+)/(?P<active_tab>[-\w]*)/?$", "contacts_and_people.views.place"),
(r"^place/(?P<slug>[-\w]+)/?$", "place", {}, "contact_place"),
(r"^place/(?P<slug>[-\w]+)/(?P<active_tab>[-\w]*)/?$", "place", {}, "contact_place_tab"),

# lists of people in an entity
(r"^people/(?P<slug>[-\w]+)/(?P<letter>\w)/$", "contacts_and_people.views.people"),
(r"^people/(?P<slug>[-\w]+)/$", "contacts_and_people.views.people"),
(r"^people/(?P<slug>[-\w]+)/$", "people", {}, "contact_people"),
(r"^people/(?P<slug>[-\w]+)/(?P<letter>\w)/$", "people", {}, "contact_people_letter"),

# main contacts & people page
(r'^contact/(?P<slug>[-\w]+)/$', "contacts_and_people.views.contacts_and_people"), # non-base entities
(r'^contact/$', "contacts_and_people.views.contacts_and_people"), # base entity only
(r'^contact/(?P<slug>[-\w]+)/$', "contacts_and_people", {}, "contact"), # non-base entities
(r'^contact/$', "contacts_and_people", {}, "contact_base"), # base entity only

# news, events, vacancies, studentships
(r'^', include('news_and_events.urls')),
Expand Down
19 changes: 11 additions & 8 deletions news_and_events/urls.py
@@ -1,18 +1,21 @@
from django.conf.urls.defaults import *
from news_and_events import views
# from news_and_events.views import NewsAndEventsViews

urlpatterns = patterns('',
urlpatterns = patterns('news_and_events.views',

# news and events items
url(r"^news/(?P<slug>[-\w]+)/$", views.newsarticle, name="newsarticle"),
url(r"^event/(?P<slug>[-\w]+)/$", views.event, name="event"),
url(r"^news/(?P<slug>[-\w]+)/$", "newsarticle", {}, "news" ),
url(r"^event/(?P<slug>[-\w]+)/$", "event", {}, "event"),

# named entities' news and events
url(r'^news-archive/(?:(?P<slug>[-\w]+)/)?$', views.news_archive, name="news_archive"),
url(r'^previous-events/(?:(?P<slug>[-\w]+)/)?$', views.previous_events, name="previous_events"),
url(r'^forthcoming-events/(?:(?P<slug>[-\w]+)/)?$', views.all_forthcoming_events, name="forthcoming_event"),
url(r"^news-and-events/(?:(?P<slug>[-\w]+)/)?$", views.news_and_events, name="news_and_events"),
url(r'^news-archive/?$', "news_archive", {}, "news-archive_base"),
url(r'^news-archive/(?:(?P<slug>[-\w]+)/)?$', "news_archive", {}, "news-archive"),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an advantage here in replacing a single pattern with two - one that can catch slugs and one that doesn't?

url(r'^previous-events/?$', "previous_events", {}, "previous-events_base"),
url(r'^previous-events/(?:(?P<slug>[-\w]+)/)?$', "previous_events", {}, "previous-events"),
url(r'^forthcoming-events/?$', "all_forthcoming_events", {}, "forthcoming-events_base"),
url(r'^forthcoming-events/(?:(?P<slug>[-\w]+)/)?$', "all_forthcoming_events", {}, "forthcoming-events"),
url(r"^news-and-events/?$", "news_and_events", {}, "news-and-events_base"),
url(r"^news-and-events/(?:(?P<slug>[-\w]+)/)?$", "news_and_events", {}, "news-and-events"),
)
#(r"^entity/(?P<slug>[-\w]+)/news/$", "news_and_events.views.news"), # in development

10 changes: 5 additions & 5 deletions news_and_events/views.py
@@ -1,5 +1,5 @@
import datetime

from django.utils.translation import ugettext as _
from django.shortcuts import render_to_response, get_object_or_404
from django.template import RequestContext
from django.http import Http404
Expand Down Expand Up @@ -39,17 +39,17 @@ def common_settings(request, slug):
return instance, context, entity


def news_and_events(request, slug):
def news_and_events(request, slug=None):
instance, context, entity = common_settings(request, slug)

instance.type = "main_page"

meta = {"description": "Recent news and forthcoming events",}
title = unicode(entity) + u" news & events"
title = unicode(entity) + _(u" news & events")
if MULTIPLE_ENTITY_MODE:
pagetitle = unicode(entity) + u" news & events"
pagetitle = unicode(entity) + _(u" news & events")
else:
pagetitle = "News & events"
pagetitle = _("News & events")
CMSNewsAndEventsPlugin().render(context, instance, None)

context.update({
Expand Down
31 changes: 15 additions & 16 deletions vacancies_and_studentships/urls.py
@@ -1,26 +1,25 @@
from django.conf.urls.defaults import patterns

urlpatterns = patterns('',
urlpatterns = patterns('vacancies_and_studentships.views',

# vacancies and studentships
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These need some more names on the patterns - for example, name="archived-vacancies_base" and so on.

I will also rationalise hyphens and underscores - "archived-vacancies_base" looks messy.

(r"^vacancy/(?P<slug>[-\w]+)/$", "vacancies_and_studentships.views.vacancy"),
(r"^studentship/(?P<slug>[-\w]+)/$", "vacancies_and_studentships.views.studentship"),
(r"^vacancy/(?P<slug>[-\w]+)/$", "vacancy", {}, "vacancy"),
(r"^studentship/(?P<slug>[-\w]+)/$", "studentship", {}, "studentship"),

# named entities' vacancies and studentships
(r"^vacancies-and-studentships/(?P<slug>[-\w]+)/$", "vacancies_and_studentships.views.vacancies_and_studentships"),

(r'^archived-vacancies/(?P<slug>[-\w]+)/$', "vacancies_and_studentships.views.archived_vacancies"),
(r'^all-open-vacancies/(?P<slug>[-\w]+)/$', "vacancies_and_studentships.views.all_current_vacancies"),

(r'^archived-studentships/(?P<slug>[-\w]+)/$', "vacancies_and_studentships.views.archived_studentships"),
(r'^all-open-studentships/(?P<slug>[-\w]+)/$', "vacancies_and_studentships.views.all_current_studentships"),

(r"^vacancies-and-studentships/(?P<slug>[-\w]+)/$", "vacancies_and_studentships", {}, "vacancies-and-studentships"),
# base entity's vacancies and studentships
(r'^vacancies-and-studentships/$', "vacancies_and_studentships.views.vacancies_and_studentships"),
(r'^vacancies-and-studentships/$', "vacancies_and_studentships", {}, "vacancies-and-studentships_base"),

(r'^archived-vacancies/(?P<slug>[-\w]+)/$', "archived_vacancies"),
(r'^all-open-vacancies/(?P<slug>[-\w]+)/$', "all_current_vacancies"),

(r'^archived-vacancies/$', "vacancies_and_studentships.views.archived_vacancies"),
(r'^all-open-vacancies/$', "vacancies_and_studentships.views.all_current_vacancies"),
(r'^archived-vacancies/$', "archived_vacancies"),
(r'^all-open-vacancies/$', "all_current_vacancies"),

(r'^archived-studentships/(?P<slug>[-\w]+)/$', "archived_studentships"),
(r'^all-open-studentships/(?P<slug>[-\w]+)/$', "all_current_studentships"),

(r'^archived-studentships/$', "vacancies_and_studentships.views.archived_studentships"),
(r'^all-open-studentships/$', "vacancies_and_studentships.views.all_current_studentships"),
(r'^archived-studentships/$', "archived_studentships"),
(r'^all-open-studentships/$', "all_current_studentships"),
)