Skip to content

Clean _make_menu_item helper #6263

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

Merged
merged 4 commits into from
Sep 7, 2021

Conversation

pdelboca
Copy link
Member

@pdelboca pdelboca commented Jul 17, 2021

This PR aims to clean the helpers to build navigation bars and some old code and configs that are now obsolete.

  • Removing config['routes.named_routes'] should be straight forward since we don't require a mapping anymore. (assuming that all templates are updated to use flask routes when creating nav links._
  • _make_menu_item will no longer raise an exception if the helper (in the template) is missing a required argument for the endpoint. I think this is an acceptable tradeoff aiming to clean lot of legacy code that it is still tied to old Pylons logic.

@pdelboca pdelboca marked this pull request as ready for review July 17, 2021 23:57
@amercader amercader self-assigned this Jul 20, 2021
@amercader
Copy link
Member

This looks good @pdelboca but we should add a note to the CHANGELOG (a changes/6263.removal file) about dropping support for old pylons-based routes on the helpers like build_nav_main() and link_to(), eg use dataset.search instead of search or dataset_search.

@pdelboca
Copy link
Member Author

pdelboca commented Jul 21, 2021

Done @amercader ! Let me know if that's okay :)

link_to() does not use _make_menu_item() but I added all the helpers that call it internally.

@frafra
Copy link
Contributor

frafra commented Oct 29, 2021

I see that there are still warnings for the old route style, saying that is deprecated, but it has been dropped instead: old code will give errors.

ckan/ckan/lib/helpers.py

Lines 985 to 999 in bb5e40c

def map_pylons_to_flask_route_name(menu_item):
'''returns flask routes for old fashioned route names'''
# Pylons to Flask legacy route names mappings
mappings = config.get('ckan.legacy_route_mappings')
if mappings:
if isinstance(mappings, str):
LEGACY_ROUTE_NAMES.update(json.loads(mappings))
elif isinstance(mappings, dict):
LEGACY_ROUTE_NAMES.update(mappings)
if menu_item in LEGACY_ROUTE_NAMES:
log.info('Route name "{}" is deprecated and will be removed. '
'Please update calls to use "{}" instead'
.format(menu_item, LEGACY_ROUTE_NAMES[menu_item]))
return LEGACY_ROUTE_NAMES.get(menu_item, menu_item)

Should such message be changed?

@frafra
Copy link
Contributor

frafra commented Dec 15, 2021

Will this be backported to 2.9?

nickumia-reisys added a commit to GSA/ckanext-datagovtheme that referenced this pull request Apr 18, 2023
nickumia-reisys added a commit to GSA/ckanext-usmetadata that referenced this pull request Apr 20, 2023
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.

3 participants