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

[#4310] Add global properties controller and action #4319

Merged

Conversation

smotornyuk
Copy link
Member

@smotornyuk smotornyuk commented Jul 3, 2018

fixes #4310.

There are a bunch of extensions that rely on this props. This PR
is attempting to add them inside before_each_request hook. If
url.endpoint contains a dot, then part before dot will be considered
as controller and part after the dot - as action. otherwise(routes
without the dot, like test /hello route), view name is used for both,
controller and action name

There are a bunch of extensions that relies on this props. This PR
is attempting to add them inside `before_each_request` hook. If
url.endpoint contains dot, than part before dot will be considered
as controller and part after the dot - as action. otherwise(routes
without dot, like test /hello route), view name is used for both,
controller and action name
@torfsen
Copy link
Contributor

torfsen commented Jul 4, 2018

Should we also deprecate these attributes, for example by adding a note in the change log?

@amercader amercader self-assigned this Jul 10, 2018
@amercader
Copy link
Member

Looks good. This is not going to cover all cases though. eg any dataset endpoint will set c.controller == dataset instead of c.controller == package, and the resource related endpoints will have c.controller == resource (eg this won't help with stadt-karlsruhe/ckanext-discovery#1).

@torfsen I'm keen on your opinion as a extension maintainer here. Is it fair to document this in the changelog and ask extensions to do a bit of handling like

try:
    is_dataset_request = toolkit.c.controller == 'package'    # CKAN < 2.8
except AttributeError:
    is_dataset_request = toolkit.request.endpoint.startswith('dataset.')   # CKAN >= 2.8

I guess that for the package case it would be all right to add an exception, but personally I'd like to avoid it. It's hard to keep the balance between moving forward and support all cases for extensions.

@torfsen
Copy link
Contributor

torfsen commented Jul 10, 2018

Personally, I'm definitely +1 for moving towards a consistent behavior without hard-coded exceptions, even if that requires modifications for our extensions. As long as the changes are well-documented, this should be no problem.

You could make things easier for extension maintainers by providing something like the following as a toolkit function (untested code):

def get_endpoint():
    try:
        # CKAN >= 2.8
        return tuple(toolkit.request.endpoint.split('.'))
    except AttributeError:
        try:
            # CKAN < 2.8
            return toolkit.c.controller, toolkit.c.action
        except AttributeError:
            return None

That way one also has to update the extensions, but

  1. the version-dependend code doesn't need to be rewritten every time
  2. usage of the new x.y format is encouraged

One might still need to perform multiple checks due to difference in the versions (e.g. dataset vs. package), but one would need to do that anyway. And IMO it's better to do that only when necessary instead of manually adding selected exceptions to CKAN core:

if get_endpoint()[0] in ['dataset', 'package']:
    do_something()

@amercader
Copy link
Member

@smotornyuk will add the toolkit function suggested by @torfsen

@smotornyuk smotornyuk force-pushed the flask-views-global-controller-and-action branch from 4deec0b to 6880ef3 Compare July 22, 2018 10:54
@smotornyuk
Copy link
Member Author

I've added function and updated changelog notes

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

Successfully merging this pull request may close these issues.

Missing c.action attribute in 2.8.0 templates
4 participants