Fix/#7197/Change custom populate state to own service

[juandl]

The service "fetch" is used to fetch user data from endpoints "/users/{id}" so only is showing the populated field "role" to avoid this issue, I change to an own service call "fetchState" in this way can still personalize the state user in the header and solve the current issue and allow full customization only for state.user

This commit lets the user change this rule by overwriting the file /extensions/users-permissions/services/User.js using the function fetchState

Fixes #7197
Relate PRs #6770 #4540

My PR is a:

• 💥 Breaking change
• 🐛 Bug fix
• 💅 Enhancement
• 🚀 New feature

Main update on the:

• Admin
• Documentation
• Framework
• Plugin

Manual testing is done on the following databases:

• Not applicable
• MongoDB
• MySQL
• Postgres
• SQLite

[juandl]

One thing i guess will be good if we keep in the same "fetch" service using this

fetch(params, populate) {
    let defaultPopulate = ['role'];
    if (populate) _.merge(defaultPopulate, populate);
    return strapi.query('user', 'users-permissions').findOne(params, defaultPopulate);
  }

but also will be safe to have the "fetchState" in a separate service, so can only customize the state of the current user

[rokasaitenta]

Why don't you just customize the policy if you need to populate more related fields and leave everything else as is in the Strapi's codebase? I don't think that things as "fetchState" are meaningful in a generic service like this one. And I think that it's logical to explicitly populate only 'role' field in this policy. If you need more fields, just customize the policy, not the services. :)

[juandl]

@rokasaitenta that will require some work and maybe will have break changes, also the population is done by the query, add that in the policy maybe can have an issue in performance

[rokasaitenta]

I mean, if it's possible, customize the policy locally, not in this repository. If it's not possible to customize policies locally, then I don't know the solution yet. Either way, I don't think that it's a good idea to create such a specific method in a general purpose service. I am talking about "fetchState". Could we ask for someone else's opinion?

[juandl]

@rokasaitenta I don think is a bad idea.. also I mention a second way to do to don't add another service function, I create separate method because in this case it better to have separate and to do whatever the user needs to do to change the "state of a user" in the header "ctx.state"user" without affecting another thing.

fetch(params, populate) {
    let defaultPopulate = ['role'];
    if (populate) _.merge(defaultPopulate, populate);
    return strapi.query('user', 'users-permissions').findOne(params, defaultPopulate);
  }

[juandl]

@rokasaitenta with this custom service "developers" can have high customization for "ctx.state.user" if someone is doing deep functions and required it, they can do it by overwriting this service.

example: Lets image you are doing a marketplace, and you required to have a "users" and users will have one "customers" or "vendor" and I want in the "ctx"state.user" know if a vendor "has a store", why make another query to know that.. when i can personalize "fetchState" to have it globally.. and more cases can happen

[rokasaitenta]

Can you customize the policy locally just like a service? Or it's not possible?

EDIT: I just checked it - you can customize the policy as mush as you want. So why create additional methods in general services for a very specific purpose?

[juandl]

@rokasaitenta Yes can be, but then you will have "2 queries" running.. one for your own local policy + the one is doing here packages/strapi-plugin-users-permissions/config/policies/permissions.js... and I guess we want to keep fewer queries running no? I'm my side I want fewer queries to save response time :) ... so if u edit directly "fetchState", you will have only one query running and save performance and response time

[rokasaitenta]

You can do it with one query if you customize the permissions.js policy. Just pass an array of your desired fields to populate as a second argument to the find method like this ctx.state.user = await strapi.plugins['users-permissions'].services.user.fetch({ id }, ['role', 'anyOtherField', 'etc']);. You don't need to create another policy. Strapi allows you to override default policies.

[juandl]

@rockmandash you can't do that, for that you have to copy the entire file packages/strapi-plugin-users-permissions/config/policies/permissions.js to overwrite that line only.... and that is not really good idea because if strapi team change something there you have also to change in your local files.

[juandl]

@rokasaitenta Please i will like you to check how the flow of a user with policies work internally and externally

[rokasaitenta]

@rockmandash you can't do that, for that you have to copy the entire file packages/strapi-plugin-users-permissions/config/policies/permissions.js to overwrite that line only.... and that is not really good idea because if strapi team change something there you have also to change in your local files.

@juandl Well, that's the way to go with Strapi currently. I still don't agree with the idea of creating additional methods for a specific purpose in general purpose services.

@rokasaitenta Please i will like you to check how the flow of a user with policies work internally and externally

What do you mean?

[juandl]

@rockmandash you can't do that, for that you have to copy the entire file packages/strapi-plugin-users-permissions/config/policies/permissions.js to overwrite that line only.... and that is not really good idea because if strapi team change something there you have also to change in your local files.

@juandl Well, that's the way to go with Strapi currently. I still don't agree with the idea of creating additional methods for a specific purpose in general purpose services.

@rokasaitenta Please i will like you to check how the flow of a user with policies work internally and externally

What do you mean?

Doing what you mention is not a good idea, because you have to touch the core of strapi and copy an entire file when you can just do it with 1 service and save time/effor/problems in future updates. there's nothing wrong doing a small line of a service to fetch a "state user" when is something very used by strapi

[rokasaitenta]

I would like to hear other people opinions. Do you know anyone from Strapi's core team who could help us? Maybe @alexandrebodin ?

[juandl]

@derrickmehaffy any thoughts here? :)

[derrickmehaffy]

strapi/packages/strapi-plugin-users-permissions/controllers/User.js

Lines 42 to 57 in e81c6ba

	/**
	* Retrieve user records.
	* @return {Object|Array}
	*/
	async find(ctx, next, { populate } = {}) {
	let users;
	if (_.has(ctx.query, '_q')) {
	// use core strapi query to search for users
	users = await strapi.query('user', 'users-permissions').search(ctx.query, populate);
	} else {
	users = await strapi.plugins['users-permissions'].services.user.fetchAll(ctx.query, populate);
	}
	ctx.body = users.map(sanitizeUser);
	},

You should be able to tweak the populate to an array to populate what you need, just copy those lines into a module.exports = {} in ./extensions/users-permissions/controllers/User.js

[derrickmehaffy]

You may actually need to do it at the service level if you need the population at the /users/me endpoint:

strapi/packages/strapi-plugin-users-permissions/config/policies/permissions.js

Line 19 in e81c6ba

ctx.state.user = await strapi.plugins['users-permissions'].services.user.fetch({ id });

[juandl]

@derrickmehaffy thank you for your time, I know you are busy xD ... but talking about in general, it's okay to have an own service to populate/edit/change the "ctx.state.user" ?, so we can override the service to edit it. ?

[rockmandash]

@juandl you mentioned the wrong guy😂
I was scared of being mentioned🤒

[juandl]

@juandl you mentioned the wrong guy😂
I was scared of being mentioned🤒

feel free to give your opinion :) :P

[rokasaitenta]

So what's the final verdict? I think that it's not necessary to add additional methods to User service, like "fetchState", because other solutions exist that would not require to change the core and could be done locally as per developers' requirements. @derrickmehaffy , @rockmandash , @alexandrebodin ? What do you guys think? I feel responsible for this and don't want bad decision to reach such wonderful project's core, no matter whether they're made by me or by others. :)

With regard to @juandl concerns about Strapi changing the core someday and all the developers having to adjust their custom policies... Well, that's how Strapi works today. If they decide to change something in the services code, the same thing will happen, so I believe saying that overriding services instead of policies is better is wrong.

[juandl]

@rokasaitenta I'm not sure i guess is better change 1 line of service instead of a change a entire file with more functions etc..

Anyways i left here my part, strapi decide if good or not 😅

[rokasaitenta]

@juandl From number of lines point of view, yes, but there is another point of view - the project's architecture point of view. Your solution touches the core, adds functionality meant for a very specific use case to a general purpose service that should not be concerned with some state or whatever it is. My solution does not touch the core, it is very simple - just customize the project to meet your needs without any changes in the project's repository and affecting other users. That's how Strapi is made. You can customize it pretty deeply.

[rokasaitenta]

@rokasaitenta I'm not sure i guess is better change 1 line of service instead of a change a entire file with more functions etc..

Anyways i left here my part, strapi decide if good or not 😅

Yes, I think it will be best if we let the smart guys decide. 😄

[alexandrebodin]

Hi guys, thanks you for your amazing involvement in trying to improve Strapi ! That's awesome :D

Here is what I think would be the best approach here:

I would revert fetch like you just did @juandl and create a specific fetchAuthenticatedUser that would only take a userId so we know the intent and would not be mistakeen with the generic fetch.

module.exports = {

 fetch(params, populate) {
   return strapi.query('user', 'users-permissions').findOne(params, populate);
 },

 fetchAuthenticatedUser(id) {
   return strapi.query('user', 'users-permissions').findOne({ id }, ['roles']);
 }

}

I think in this specific case it would be easier to override the service function as it is a oneliner and it wouldn't require updates as often as the permissions policy.

Reading the policy a user would understand really fast that it is calling a specific function and could update it.

In summary: make the specific code really specific so there are no confusion :)

Hope this make sense

[rokasaitenta]

Hi guys, thanks you for your amazing involvement in trying to improve Strapi ! That's awesome :D

Here is what I think would be the best approach here:

I would revert fetch like you just did @juandl and create a specific fetchAuthenticatedUser that would only take a userId so we know the intent and would not be mistakeen with the generic fetch.

module.exports = {

 fetch(params, populate) {
   return strapi.query('user', 'users-permissions').findOne(params, populate);
 },

 fetchAuthenticatedUser(id) {
   return strapi.query('user', 'users-permissions').findOne({ id }, ['roles']);
 }

}

I think in this specific case it would be easier to override the service function as it is a oneliner and it wouldn't require updates as often as the permissions policy.

Reading the policy a user would understand really fast that it is calling a specific function and could update it.

In summary: make the specific code really specific so there are no confusion :)

Hope this make sense

I guess that's the sweet spot. Totally agree with this solution!

[juandl]

Great so i guess only change the name of the function and default populate "role" with default id as parameter

[rokasaitenta]

@rokasaitenta you know is the same logic as do hahahha just with restrictions 😅. Anyways glade to finish :)

As I said, that's the architecture point of view. Small details make a huge impact.

[rokasaitenta]

Now that's a PR! Well done! 👍 The only thing I can offer is a little more clear comment, something like "Promise to fetch a plain user for internal purposes", because it's not clear what the "state" or "header" means if you are looking at just this one file, because this service is not concerned with any state or header things...

[juandl]

@rokasaitenta I think you are going to far, please feel free to make your own PR adding the changes you prefer 👍

[rokasaitenta]

Sorry, man, for me it's all or nothing. ;)

[alexandrebodin]

@juandl sorry but your PR contains invalid commits coming from other PRs :/ Will wait until this is cleaned up to merge :)

[juandl]

@juandl sorry but your PR contains invalid commits coming from other PRs :/ Will wait until this is cleaned up to merge :)

Sorry, I think was when I do DCO :/ , you guys will clean up or how we proceed?

[alexandrebodin]

I can't do much on my side :/ you can rebase and drop the commits or cherry pick the correct ones so we can move forward

[juandl]

I can't do much on my side :/ you can rebase and drop the commits or cherry pick the correct ones so we can move forward

Okay, is ready :) , sorry for that.

[derrickmehaffy]

This pull request has been mentioned on Strapi Community. There might be relevant details there:

https://forum.strapi.io/t/ctx-state-user-returns-undefined/1048/1

[strapi-bot]

This pull request has been mentioned on Strapi Community Forum. There might be relevant details there:

https://forum.strapi.io/t/overriding-plugin-service/24218/1