-
Notifications
You must be signed in to change notification settings - Fork 14
add UsersPermissionsUsersManager #108
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
|
I'm confused on this issue and it seems like a pretty deep dive to find all the answers, can you give some more details on what the actual problem is in client vs cms? Does users-permissions in CMS accept a db id rather than a document id as it should? Reading the bug report in #80 , it seems like this change would just enforce that it's a numeric id, but not provide a solution for updating by documentId, is that right? Does CMS provide a method for updating users by documentId? Given that client is supposed to be a clean, simple interface for interacting with Strapi, I'm wondering if we should intentionally bring some complexity here on our end to simplify it for users and have strapi/client users accept Sorry if I'm completely misunderstanding the issue 😆 |
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.
If anyone can confirm the runtime is actually leaning toward the resolution of the bug, cause I can't put my head around it 😅
Even before this, do we want to actually support the API described in the #80 issue?
—
Edit: didn't see @innerdvations comment before. I guess his inputs are same than mine, just better explained 🙈
src/client.ts
Outdated
| this._httpClient | ||
| ); | ||
|
|
||
| return manager as UsersPermissionsUsersManager & UsersPermissionsUsersIdOverloads; |
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'm a bit concerned by the type inference of the collection method
|
Content API (/api/users) doesn't support documentId operations and uses strapi.db.query() directly with the provided id - https://github.com/strapi/strapi/blob/9cb2f2326113dd3523a6c9ee540922e04cf54ff6/packages/plugins/users-permissions/server/controllers/user.js#L102-L105 akaik any documentID based solution in the client would require CMS changes first or an additional query during the client logic to retrieve the relevant up_user id? (e.g. SELECT id FROM up_users WHERE document_id etc) and that doesn't seem ideal Hence the initial approach here was to make another exception in the client logic for users-permissions so the DX is clear that the user should provide a numeric ID in these cases What do you think the best way forward is? |
662703e to
ef7a4e2
Compare
nclsndr
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.
We did conclude w/ @jhoward1994 that this PR will not block the future DX improvements while slightly helping users working with u&p "users" resources.
We acknowledge its limitations:
- return types from
client.collection('users').*()methods are still expected to be off 'roles'resources are not covered
What does it do?
Special handling for users-permission user content type operations to prevent the error seen in
#80
Related issue(s)/PR(s)
DX-2082
fixes #80