update actions to use correct names

[tobes]

eg package_show should be dataset_show

we need to ensure that the old names continue to work etc - probably best to patch this via get_action - need to ensure that auth functions also behave the same way.

[vitorbaptista]

Hi @tobes,

Cool! It's about time to start removing references to package 👍 I know this is still WIP, but just to add my 2 cents...

What do you think about renaming the methods from package_* to dataset_* first? Then, you'd just need to call clean_action_name once at the first lines of _get_auth_function and get_action. As we would only have dataset_* methods, there's no need to clean the actions when building the cache.

Also, while you're at it, it would be great to refactor _get_auth_function and get_action. They're basically copy/pasted from each other...

[tobes]

@vitorbaptista the thing is that extensions can override the actions/auth functions so we do need to do it at least for those. Also doing it this way allows the renaming to be done at our leisure.

We probably should do that but it doesn't strictly need to be part of this branch to get the feature working. Also I suspect that some tests do nasty direct use of the functions so it may be more work even if needed.

[tobes]

@vitorbaptista and yes there is refactoring that could be done but sometimes these things need to wait and also it is good to keep these changes separated.

[vitorbaptista]

@tobes You mean that some extension might create a package_blah, and you want to override that to dataset_blah as well? Yeah, I haven't thought about that. Maybe doing something like:

if _actions:
    cleaned_action = new_authz.clean_action_name(action)
    action_method = _actions.get(action) or _actions.get(cleaned_action)
    if not action_method:
        raise KeyError("Action '%s' not found" % action)
    return action_method

This could be extracted in a method and used by both _get_auth_function and get_action, so you get a small refactoring as a bonus. It would be easier to unit test as well.

I usually prefer to sneak refactorings inside normal cards, leaving pure refactoring cards just for big stuff. But I agree with you, this might be big.

The main thing I would like to avoid is to call clean_action_name in a bunch of places.

[vitorbaptista]

There's a tidier way of having a fallback key in a hash, if the first one doesn't exist, used in https://github.com/okfn/ckan/blob/master/ckan/lib/search/query.py#L337

query['facet.limit'] = query.get('facet.limit', config.get('search.facets.limit', '50'))

I prefer the first one that I suggested though, as it's more explicit.

[tobes]

You mean that some extension might create a package_blah, and you want to override that to dataset_blah as well? Yeah, I haven't thought about that. Maybe doing something like:

No I mean we have package_show and an extension will override it as dataset_show or the other way round. This is why it is called in a bunch of places. As we cleanup the logic functions we can start removing it several places. This just allows the functionality that makes sense and then we can do the actual fixes later. Then we can start shouting about using package not dataset. These changes will happen over the next year or so. Eventually we will remove all this code. It is only here to help the changeover without breaking anything.

[tobes]

query['facet.limit'] = query.get('facet.limit', config.get('search.facets.limit', '50'))

This line is horrible on many levels and is the sort of code I try to kill :) mainly the default value should not be here and also I think we should do the config stuff differently but lets not discuss this here. Also query['facet.limit'] = query.get('facet.limit') or config.get('search.facets.limit', '50') feels easier to read and more pythonic

[vitorbaptista]

It makes sense, @tobes. I've created #820 so we can make sure that we'll remove this code whenever we decide to transition fully to dataset_*. IMO, we shouldn't let this linger for too long, as it's a trivial change.