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

fix(templatetags): limit menu visibility + missing page id key = crash #55

Merged

Conversation

wesleyboar
Copy link
Member

@wesleyboar wesleyboar commented Dec 6, 2022

Overview

Fix that if any menu item does not have a reverse_id*†, then limit_visibility_in_menu.py will crash.

If a CMS page has no ID* set, it will still have a reverse_id, it's just blank. This fixes edge case of menu item not having the attr/key.†

* I.e. an "ID" in Advanced Settings.
† E.g. CMS News category or article.

Related

Changes

  • check for reverse_id before using it

Testing

  1. Load this branch on existing APCD setup.
  2. Open any page of the site (not the CMS Administration).
  3. Verify no crash (from newly introduced code).
  4. Verify any of the test cases from #45 still work.

UI

N/A

@wesleyboar wesleyboar changed the title fix(templatetags): limit visibility + cms news = ☠ fix(templatetags): limit visibility + cms news = crash Dec 6, 2022
@wesleyboar wesleyboar changed the title fix(templatetags): limit visibility + cms news = crash fix(templatetags): limit visibility + missing page id attr = crash Dec 6, 2022
@wesleyboar wesleyboar changed the title fix(templatetags): limit visibility + missing page id attr = crash fix(templatetags): limit menu visibility + missing page id key = crash Dec 6, 2022
@wesleyboar
Copy link
Member Author

wesleyboar commented Jun 21, 2023

Merged based on its unobtrusive and safety, and it having been tested via Core in TACC/Core-CMS#571.

@wesleyboar wesleyboar merged commit 59d3e00 into main Jun 21, 2023
@wesleyboar wesleyboar deleted the hotfix/conflict-with-cms-news-and-limit-visitbility-in-menu branch June 21, 2023 14:02
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.

None yet

1 participant