-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Support for plugins to register URL patterns #1237
Conversation
isn't this what apphooks are for? |
No. This is for plugins. For example, I have a markup plugin for Django CMS which renders previews when user is editing the plugin content in Django CMS. So it has to have some views accessible from users. By using API in this pull request, it can publish such views under |
tests please |
also should be just a classmethod |
i want this for 3.0 |
@mitar do you want to give this an other go? |
@digi604 What do you mean? |
I still like the idea... and with some tests i might include it in 3.0 |
Against which branch should I do it? |
develop |
Rebased to current develop branch and added tests. But tests do not really work because plugin should be registered before Django urls are constructed. In a similar way Django admin solves this (with autodiscovery). So in my code I just made sure that plugins were loaded from admin autodiscovery. I am not sure what could be a solution to this. |
|
||
def get_patterns(self): | ||
# We want untranslated name of the plugin for its slug so we deactivate translation | ||
lang = get_language() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think you should call: self.discover_plugins() here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And how will this find plugins in tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can do a overwrite settings and add something to installed apps... or add the plugin in cms/plugin_utils/cli.py to the INSTALLED_APPS
Fixed. :-) I used my own API wrong. :-) I had |
could you check python 3.3 support? The tests fail for that. |
Fixed. It fails now on some other test on Django 1.5, not affected with this pull request. |
Travis still flaky... after restart all the tests passed |
Great! |
url_patterns = [] | ||
for plugin in self.get_all_plugins(): | ||
p = plugin() | ||
slug = slugify(force_unicode(p.name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plugin name is not guaranteed to be unique, probably the plugin class name would be a better option here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And plugin class is unique?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there some other "verbose name" available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plugin class name is unique in the project (it's the registration key in the plugin pool)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there already some function which makes nice verbose name from the class name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope.
Fixed. |
.. method:: get_plugin_urls(instance) | ||
|
||
Returns URL patterns for which the plugin wants to register views for. | ||
They are included under Django CMS PageAdmin in the plugin path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit-picking: django CMS not Django CMS :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What? I didn't know that. But Django is Django? And django CMS is django CMS? Strange.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Fixed.)
Fixed. |
LGTM |
Sorry @mitar, it remained unmerged and a further rebase is needed against develop. |
@yakky: Done. |
Support for plugins to register URL patterns
@mitar thanks! it took a while but it landed at last :-) |
yep thanks @mitar |
New pull request for #738. Hopefully this time not in the time of feature freeze.