-
Notifications
You must be signed in to change notification settings - Fork 23
Enhance Users Resource with adapter-based info retrieval and filtering support #78
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
Conversation
xispa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @nnpvaan. I've left two comments.
Would you mind adding some documentation about the IUsersFilter adapter in the Customizing section of the documentation?
It would also be great if you could extend the users doctest to include examples for both the IUsersFilter and IInfo adapters. You can refer to the push doctest for an example of how to create a DummyAdapter for convenience.
| continue | ||
| info[k] = v | ||
|
|
||
| for _, adapter in getAdapters((pu,), IInfo): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better use name. As a general rule, we use _ for instances of MessageFactory
| user_ids = [username] | ||
|
|
||
| # Allow addons to filter the user list via adapters | ||
| for _, adapter in getAdapters((request,), IUsersFilter): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as before. Use name instead of _
|
|
||
| # Allow addons to filter the user list via adapters | ||
| for _, adapter in getAdapters((request,), IUsersFilter): | ||
| user_ids = adapter.filter(user_ids) or user_ids |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove the or user_ids condition to avoid returning the full list when the adapter has already filtered all entries.
Besides, If I request a user by a specific username, I'd expect the system to return that user regardless. Therefore, this filtering should only be applied when no username is provided (i.e., when username is None).
|
Hi @xispa, I added the documentation and the doctest for extend adapters. Please help me review it again. |
xispa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent, thanks!
Description of the issue/feature this PR addresses
This PR adds compatibility for
IInfoadapters in user info retrieval, enabling custom extensions to dynamically enhance the user API response.It also adds a new
IUsersFilterinterface to allow flexible and extensible user filtering during query operations.Current behavior before PR
Desired behavior after PR is merged
IInfoadapters.IUsersFilterimplementations, providing more control and modularity for custom add-ons.--
I confirm I have tested the PR thoroughly and coded it according to PEP8
standards.