Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Dry url issue69 #71

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

darbula commented Mar 15, 2013

This pull request replaces pull/issue #69 as that issue seems to have some work ahead and by my mistake it did not have a dedicated named branch. Now I have made a dedicated branch issue69 to be used to fix everything concerning following problem:

Description:

Urls for contacts, news and vacancies were defined in multiple places:

  • urls.py,
  • models.py - in url_path and
  • menus.py - in url_attribute.

This situation prevents changing/translating the default urls (i.e. contacts/ news/ etc). In this commit url patterns in urls.py are given names and then reverse is used to resolve urls where needed i.e. in Entity.get_related_info_page_url or in URLModelMixin.get_absolute_url.

Url patterns are named so that their name can be deduced from url_path and url_attribute when needed.

Owner

evildmp commented Apr 12, 2013

I'd like to do something with some of these ideas, but I will need to discuss it first. One issue is that it is important that it should be easy to provide Entities with connections to other kinds of information through other applications.

For example, our Publications application makes it possible to see an Entity's academic research publications, and I am not sure how that will be affected by these changes.

Contributor

darbula commented Apr 12, 2013

There should not be any consequences to external applications but if you can provide some more details about certain parts that could become problematic we can discuss it.

Owner

evildmp commented Jun 18, 2013

I'm taking a good look at this now. There are some changes in the develop branch since you made this pull request, so I will reconcile those first, and make some a reminder to myself below about things that need tests and/or comments to explain their behaviour.

tests/comments required

  • Person.get_absolute_url() - test for self.active
  • basic tests for news and events, vacancies and studentships views
  • tests for get_absolute_url() methods where not present

@evildmp evildmp commented on the diff Jun 18, 2013

contacts_and_people/urls.py
# person
- (r"^person/(?P<slug>[-\w]+)/(?P<active_tab>[-\w]*)/?$", "contacts_and_people.views.person"),
@evildmp

evildmp Jun 18, 2013

Owner

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

@darbula

darbula Jun 18, 2013

Contributor

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.

@evildmp

evildmp Jun 18, 2013

Owner

OK, good, I think I understand this now.

@evildmp evildmp commented on the diff Jun 18, 2013

news_and_events/urls.py
# named entities' news and events
- url(r'^news-archive/(?:(?P<slug>[-\w]+)/)?$', views.news_archive, name="news_archive"),
@evildmp

evildmp Jun 18, 2013

Owner

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

@evildmp evildmp commented on the diff Jun 18, 2013

contacts_and_people/models.py
if self.external_url:
return ""
elif self == Entity.objects.base_entity():
- return "/%s/" % kind
+ if kind in kinds:
@evildmp

evildmp Jun 18, 2013

Owner

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?

@darbula

darbula Jun 18, 2013

Contributor

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.

@evildmp

evildmp Jun 18, 2013

Owner

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.

@evildmp evildmp commented on the diff Jul 11, 2013

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
@evildmp

evildmp Jul 11, 2013

Owner

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.

Owner

evildmp commented Jul 24, 2013

Have a look at https://github.com/evildmp/Arkestra/tree/merge-darbula-issue69. This:

  • incorporates your changes
  • rationalises the naming of reverse patterns
  • rationalises and reformats URL patterns
  • includes tests for reverse patterns

I will do some more testing, and if this looks OK it's ready to be merged.

@evildmp evildmp referenced this pull request Jul 24, 2013

Merged

Merge darbula issue69 #106

Owner

evildmp commented Jul 24, 2013

Merged in #106 - thanks.

@evildmp evildmp closed this Jul 24, 2013

Contributor

darbula commented Jul 24, 2013

Great, glad that this has worked its way into upstream :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment