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

`search` legacy route missing #4346

Closed
metaodi opened this issue Jul 18, 2018 · 10 comments

Comments

Projects
None yet
3 participants
@metaodi
Copy link
Member

commented Jul 18, 2018

CKAN Version if known (or site URL)

2.8+ (master)

Please describe the expected behaviour

There is a shortcut for the search legacy route in CKAN 2.8+, I guess it's missing in LEGACY_ROUTE_NAMES.

Please describe the actual behaviour

On master search is not available as route, basically the same as described in #4066

What steps can be taken to reproduce the issue?

h.build_nav_main(
          ('home', _('Homepage')),
          ('search', _('Datasets')),
          ('group_index', _('Groups'))
        )
@tino097

This comment has been minimized.

Copy link
Member

commented Jul 18, 2018

@metaodi it was not added as the 'package' controller was not merged in that time, but i think it could be added in the .ini file
http://docs.ckan.org/en/2.8/maintaining/configuration.html#ckan-legacy-route-mappings

edit: but you are right, we should add it in the helper

@metaodi

This comment has been minimized.

Copy link
Member Author

commented Jul 18, 2018

Well yes, but I think all legacy routes should be added there for compatibility reasons. If I provide a CKAN extension, it should work on all CKAN versions. The ini file might be a solution for tests, but I don't want all users of an extension to add ckan.legacy_route_mappings to their instances ini file, just because the extension runs on multiple versions of CKAN.

@tino097 tino097 referenced this issue Jul 18, 2018

Merged

[#4346] Update missing legacy routes #4348

0 of 5 tasks complete

amercader added a commit that referenced this issue Jul 19, 2018

@metaodi

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2018

@tino097 Why have you removed group_index again? Now my build fails, because this route is still missing.

@metaodi

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2018

Here is the link to the build: https://travis-ci.org/opendatazurich/ckanext-stadtzh-theme/jobs/405325390#L1427

I'd propose to add a test for this, calling build_nav_main() with the legacy routes should not raise an exception.

@tino097

This comment has been minimized.

Copy link
Member

commented Jul 19, 2018

@metaodi
in the review was pointed out that those routes should be handled in the helper
ping @amercader

@metaodi

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2018

@tino097 @amercader just let me know if you want help. Happy to provide a PR with the test and/or the updated legacy routes.

@tino097

This comment has been minimized.

Copy link
Member

commented Jul 19, 2018

@metaodi why just not update the nav menu in the template in your extension? As the migration to Flask will be finishing, those routes would be deprecated

@metaodi

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2018

@tino097 because the extension runs on multiple versions of CKAN, older version do not support the new flask-based names obviously, so the new CKAN version should support the old names to be backwards compatible.

@amercader

This comment has been minimized.

Copy link
Member

commented Jul 20, 2018

I was mistaken in the way build_nav_main works (a truly terrible way btw), I assumed it was calling url_for directly (which is where the support for old Pylons routes exists) but apparently is doing some other voodoo first, which means that yes, we need to add it to LEGACY_ROUTE_NAMES.

@tino097 just to be clear the group_index item only affects master so it doesn't need to go to 2.8.1
@metaodi a PR readding the route would be great thanks.

@metaodi

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2018

@amercader my PR #4367 is ready. It re-adds the group_index route and adds a test for build_nav_main with the old and the new route names.

tino097 added a commit that referenced this issue Aug 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.