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 show_excluded_items in @navigation api #1816

Merged
merged 8 commits into from
Sep 21, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions news/1816.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix incorrect condition for show_ecluded_items setting in the @navigation API.
mamico marked this conversation as resolved.
Show resolved Hide resolved
[mamico]
4 changes: 3 additions & 1 deletion src/plone/restapi/services/navigation/get.py
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,9 @@ def navtree(self):
if brain_parent_path == navtree_path:
# This should be already provided by the portal_tabs_view
continue
if brain.exclude_from_nav and not context_path.startswith(brain_path):
if brain.exclude_from_nav and not f"{brain_path}/".startswith(
f"{context_path}/"
):
# skip excluded items if they're not in our context path
continue
url = brain.getURL()
Expand Down
29 changes: 29 additions & 0 deletions src/plone/restapi/tests/test_services_navigation.py
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,35 @@ def test_dont_broke_with_contents_without_review_state(self):
)
self.assertIsNone(response.json()["items"][1]["items"][3]["review_state"])

def test_show_exclude_items(self):
mamico marked this conversation as resolved.
Show resolved Hide resolved
createContentInContainer(
self.folder,
"Folder",
id="excluded-subfolder",
title="Excluded SubFolder",
exclude_from_nav=True,
)
transaction.commit()
response = self.api_session.get(
"/folder/@navigation", params={"expand.navigation.depth": 2}
)
self.assertNotIn(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is settings.show_excluded_items==True at this point? That could explain the test failure.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mamico I noticed that there are differences between the the Python 3.8/Plone 5.2 and all other runs. There are two extra lines, excluding any deprecation warnings, between the test setup and test results: https://github.com/plone/plone.restapi/actions/runs/10954244520/job/30415848888?pr=1816#step:10:216

*** Relations rebuild. flush: False
*** Relations rebuild. flush: True

Perhaps that is a clue?

Also do we care about Plone 5.2/Python 3.8 for this bugfix? I don't think it is a security issue, but a maintenance fix, and Plone 5.2 is out of maintenance support.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stevepiercy probably your first suggestion is the right one. It seems that Plone 5.2 and Plone 6.0 have different defaults for show_ecluded_items (?). Now I explicitly set it in the test. I am waiting for the results of the CI...

"Excluded SubFolder",
[item["title"] for item in response.json()["items"][1]["items"]],
)

registry = getUtility(IRegistry)
settings = registry.forInterface(INavigationSchema, prefix="plone")
settings.show_excluded_items = True
transaction.commit()
response = self.api_session.get(
"/folder/@navigation", params={"expand.navigation.depth": 2}
)
self.assertIn(
"Excluded SubFolder",
[item["title"] for item in response.json()["items"][1]["items"]],
)

def test_navigation_sorting(self):
registry = getUtility(IRegistry)
registry["plone.displayed_types"] = (
Expand Down
Loading