Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ambiguity in schedule.urls #148 #148

Closed
wants to merge 1 commit into from

Conversation

massmediagroup-16
Copy link

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

r"^sessions/$" was changed to r"^sessions/all/$" for the purpose of avoiding a conflict with r"^([\w\-]+)/$"

Fixes pinax#148
@martey
Copy link
Contributor

martey commented Dec 19, 2016

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
Copy link
Author

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
Copy link
Contributor

martey commented Dec 20, 2016

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 a41fb8b, 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
Copy link
Author

massmediagroup-16 commented Dec 20, 2016

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants