search

pinax / symposion

a Django project for conference websites
BSD 3-Clause "New" or "Revised" License
299 stars 147 forks source link

Fix ambiguity in schedule.urls #148 #148

Closed massmediagroup-16 closed 7 years ago

massmediagroup-16 commented 7 years ago

r"^sessions/$" was changed to r"^sessions/all/$" for the purpose of avoiding a conflict with r"^([\w-]+)/$". It raised "Page not found" (404) error.

Fixes #148

martey commented 7 years ago

I think it would be better to just move the schedule-related URLs (schedule_detail, schedule_edit, schedule_list, schedule_list_csv, and schedule_slot_edit) to the bottom of the list of urlpatterns. This would be backwards compatible (the schedule_session_list URL would not change) and it would help prevent future URLs that might be added from accidentally conflicting with schedule_detail.

massmediagroup-16 commented 7 years ago

If we move r"^edit/$", r"^list/$", r"^sessions/$" lower then r"^([\w-]+)/$", they will be recognized as r"^([\w-]+)/$", and view 'schedule_detail' will be called for all of them. I think it would be better move r"^sessions/$" upper then r"^([\w-]+)/$" or explicitly change these urls.

martey commented 7 years ago

If we move r"^edit/$", r"^list/$", r"^sessions/$" lower then r"^([\w-]+)/$",

I actually meant ^([\w\-]+)/edit/$and ^([\w\-]+)/list/$ in my earlier comment when I was referring to schedule_edit and schedule_list. Technically, only the schedule_detail pattern needs to be moved to the bottom, since it is less restrictive than any of the other urlpatterns.

I think it would be better move r"^sessions/$" upper then r"^([\w-]+)/$" or explicitly change these urls.

This solves the issue with the schedule_session_list urlpattern, but any future urlpatterns added below the schedule_detail urlpattern will run into the same issue.

This bug was caused by a41fb8bd35420f900001959ad12e520fa2e870d9, when all of the session-related URLs were placed at the bottom of the list of patterns. If you just move the schedule_session_list urlpattern up the list, you increase the chances of a similar regression happening in the future.

massmediagroup-16 commented 7 years ago

If you just move the schedule_session_list urlpattern up the list, you increase the chances of a similar regression happening in the future.

I agree with this. But it will solve a conflict with urls at this point, if any other urls are not added. If we want to avoid later conflicts we should avoid ambiguous urls. Maybe a change r"^([\w-]+)/$" to r"^([\w-]+)/detail/$" can help.