Skip to content

Conversation

RamezIssac
Copy link
Contributor

@RamezIssac RamezIssac changed the title Make model/app urls configurable for admin Fixes 29532: Make model/app urls configurable for admin Jun 29, 2018
@collinanderson
Copy link
Contributor

collinanderson commented Jun 29, 2018

Hmm... _app_label_url_map seems a little hacky to me, and get_url_for_app_model should probably have a different name. So I don't really like the implementation, but it does give a fairly easy to use API (all you need to do is override get_url_for_app_model (or whatever it's called)).

Does this code actually work? (does reversing app_index actually work correctly?)

I personally don't see huge use case for this, and it makes things more complicated, but if django can provide a way to do this without having to totally copy/paste the entire get_urls() method, that seems like a win to me. Like maybe we don't automatically handle the app_index case automatically for you, but if you really want to do it you just need to override app_index() to translate the app name, and override a get_app_index_url() so the url is correct.

@RamezIssac
Copy link
Contributor Author

With you on the name get_url_for_app_model .. what would be an appropriate name ?
About _app_label_url_map i don't see it is hacky... just a way to map the url to the "real" app_label.
I dislike that i reused the app_label parameter

reversing app_index would work.. however, because of the its unique regex style, it would expect the "modified" app_label.
Changing this unique regex style would make everything work smoother (reverse will accept the real app_label not the modified one), but, we will most probably loose the unique and rather helpful error message when there is a typo in the app name when reversing app_index. :)

Finally, like you, i just want a way to achieve this without copy/paste the whole get_urls

@RamezIssac RamezIssac force-pushed the ticket_29532 branch 2 times, most recently from 32df32d to 7e0972b Compare June 30, 2018 06:05
@RamezIssac
Copy link
Contributor Author

In this commit i implemented a new url pattern for app_index which behave similar to the other admin patterns, ie, the app_label is in the name and not a parameter, which will avoid problems in case of a custom app url.

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.

2 participants