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

Filter inactive items also for admins. #4099

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gyst
Copy link
Member

@gyst gyst commented Feb 3, 2025

Fixes #4098.

This keeps the existing default behavior, while allowing suppressing inactive content for admins, if show_inactive=False is explicitly passed as a keyword or query.

@mister-roboto
Copy link

@gyst thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

@gyst gyst requested a review from thet February 3, 2025 12:39
@gyst
Copy link
Member Author

gyst commented Feb 3, 2025

Hang on, I think I can fix the issue while keeping the current default behavior.

@gyst gyst force-pushed the filter-inactive-for-admins branch from b599482 to 0858e05 Compare February 3, 2025 13:13
@gyst
Copy link
Member Author

gyst commented Feb 3, 2025

I improved this PR to retain the current default behavior, while still fixing the bug.

show_inactive = kw.get("show_inactive", False)
if isinstance(query, dict) and not show_inactive:
show_inactive = "show_inactive" in query
show_inactive = kw.get("show_inactive", None)
Copy link
Member Author

Choose a reason for hiding this comment

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

I know I could've just used kw.get("show_inactive") here and below, but since this logic depends so crucially on None being different from False, I preferred the explicit variant.

@yurj
Copy link
Contributor

yurj commented Feb 3, 2025

Seems fine to me, show inactive = False is ok. If it is True, that means "let me show inactive" even if the user should not? show_inactive is used here:

catalog_vars["show_inactive"] = True

" # Include inactive content in result list. This is
# especially important for content scheduled to go public
# in the future, but needs to be reviewed before this."

and here:

query["show_inactive"] = False

respect effective/expiration date

and some other places:

https://github.com/search?q=org%3Aplone+show_inactive&type=code

@gyst
Copy link
Member Author

gyst commented Feb 3, 2025

If it is True, that means "let me show inactive" even if the user should not?

Yes, that's unchanged from the current implementation. Also the default behavior remains unchanged for both admins and non-admins. The only change is, that if you're an admin, it was previously hardcoded that you would always see inactive items; and this is still the default; however if you as an admin request show_inactive=False it will now honor that setting.

Since show_inactive=False before did not work for admins, any code that explicitly asks for that and then still expects inactive items was broken by design and needs to be fixed.

@erral
Copy link
Member

erral commented Feb 3, 2025

FWIW, in a recent Plone 6 project, we have a lot of users who can add content, but are not Managers. They have "can add" permissions wherever they need it.

We had the case where those users set some content to be published in the future, but they can't see those contents.

We went to the /manage_access and set the "Access inactive portal content" permission for "Authenticated" users, and this way they can see these content.

We thought that this was a regression in Plone 6, because this site was migrated from Plone 4 and those users had no problems in the previous versions. So we went Plone 4 and found that there we had also the same permission set for "Authenticated" users.

@libargutxi

@gyst
Copy link
Member Author

gyst commented Feb 3, 2025

We went to the /manage_access and set the "Access inactive portal content" permission for "Authenticated" users, and this way they can see these content.

This will still be required, this PR does not change that.

What it does change, that if you have that permission and you then query searchResults(show_inactive=False), you will actually not see that content.

Copy link
Member

@davisagli davisagli left a comment

Choose a reason for hiding this comment

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

Looks okay to me, and thanks for adding tests. Let's run the tests for all Plone packages and make sure there are no regressions:

@jenkins-plone-org please run jobs

news/4098.bugfix Outdated Show resolved Hide resolved
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.

Filtering expired/scheduled items should be possible even for managers
5 participants