core_user_get_users()

I hope to see this under active development soon. This will be important for us in some external integration projects with external systems such as a CMS and SIS. Thanks for your work!

Hi Drew, it is not under active development this month. Moodle HQ is focussing on documentation in January. However I'll increase the issue priority, so we'll look at it quickly when starting the Roadmap implementation.

Hello,

So here it is, another function that I need and only now I realized it!
At the roadmap API, I can see that a key/value pair should be specified for this function, but I failed to understand if one can specify more than one pair, and how multiple pairs would be processed (should be AND, or OR, or whatever, like username=X and/or auth=Y), if multiple pairs are to be supported, of course.

I would like to contribute with an implementation for this function but I need to understand what are your plans for it first.

Fábio

Currently I have an implementation that supports multiple key/values AND'ed together and compared by equality.
I think it could be extended so that it supports other types of comparison, namely minor, major and equal.

I would like to know your thoughts. The inspiration for this function is similar to get_users_by_id, the only thing that changes is the possibility of submitting key/values instead of a single id.

The compare can be checked here:

https://github.com/fabiomsouto/moodle/compare/MOODLE_22_STABLE...MDL-29938-MOODLE_22_STABLE

Cheers
Fábio

Hi Fabio,
I'll look to your contributions. I come back to you soon on this.
cheers,
Jerome

Hey Jerome,

Sure. If I can help on something, let me know, I'll be glad to.

Cheers,
Fábio

Hi Fabio,

• at this moment in core we have two external functions to retrieve user profiles. One let you retrieve system user profile (get_users_by_id), the other one let you retrieve course user profile (get_course_user_profiles). Except as admin, most of time students can only use the course user profile function. It was a design error. I think this core_user_get_users function should support both logic of these two functions. The core_user_get_users function should:
  • check for the course context where the $USER is enrolled and look for participant in this course contexts. (the logic of get_course_user_profiles)
  • check for system context if the $USER has the right capability. (the logic of get_users_by_id)
    It should be a mixed of both get_users_by_id and get_course_user_profiles.

• I also think we should clearly mention the keys and only accept these keys (like in https://github.com/jleyva/moodle/compare/master...MDL-30084):

  public static function get_users_parameters() {
   	 return new external_function_parameters(
   	     array(
   	         'criteria' => new external_multiple_structure(
   	             new external_single_structure(
   	                 array(
   	                      'key'        => new external_value(PARAM_ALPHA, 'the user column to search - HERE WE SHOULD LIST ALL POSSIBLE KEYS. ALL THESE KEY MUST BE SWITCH {CASE: CLEAN_PARAM(PARAM_XXX,VALUE)} WITH THE CORRECT PARAM_XXX in get_users()'),
   	                     'value'      => new external_value(PARAM_RAW, 'the value to match')
   	                  )
   	              )
   	          )
   	      )
   	   );
   	}
  

Can you change these two points? I let you pick the key list your prefer, we still can add some if we miss. Also add the userid as criteria, we'll most likely deprecate the get_users_by_id once this new function land in core.

Thanks,
Jerome

Hello,

This seems relatively complex, so I would like to understand it better with you before making the changes blindly.
As it is now, this function will allow one to get the system profile of all users, filtered by the key/value pairs.

Would you please provide me an example or two of what you think would help me clarify the execution of this function as a mix of the other two (get_users_by_id and get_course_user_profiles)?

Edit: ok, from what I've understood get_course_user_profiles receives a user id and a course id, basically it returns the user profile in that course.
So from your perspective we should

• filter the users using the criteria, as it currently is, and then
• get, for that user, the courses he's enrolled
• for each enrolled course that the user is in, add the participant info

Is this it?

Fabio

Hi Fabio,
yes don't worry this is a very complex function. We spent lots of hours on each different functions returning users. From how I see it, core_user_get_users() is a mixed of all of them. Note that I'm just remembering another function that should be taken in consideration: core_enrol_get_enrolled_users().

Summary of function returning users currently in Moodle 2.2:

• get_course_user_profiles()
• get_users_by_id()
• get_enrolled_users()

I think for core_user_get_users() we should:

a) have criteria parameters to filter users. However we don't need a courseid criteria. I don't think the purpose of core_user_get_users() is to get enrolled user. It is a global search on some criteria like firstname. Also take note that all criteria need to be properly param_clean(PARAM_XXX,$value) and also sometimes having the proper permission checks - for example I know that very few users have the permission to search by username.
b) for each users that are returned by the search, the function returns it if:

• $USER has permission on the USER context: get_context_instance(CONTEXT_USER, $user->id)
• or $USER is enrolled in same course. If you can do this check in the same SQL request than the search it's great. However a second SQL request retrieving all participants enrolled in the same course as $USER is good enough, and easy to maintain.

From what I read it seems to me you got it. Go for it

Hi,

I'm currently studying the various implementations. Anyway:

If this bugfix ( http://tracker.moodle.org/browse/MDL-31520 ) I submitted earlier could be committed it would be great.
The title is misleading, because the problem itself is at user/lib.php. Basically the user email is only returned to the user itself or if he allows the course users to see it.
My fix would allow for the admin user to have access to the user's email addresses, which should make sense (after all he's the admin). It's a one-line fix.

Meanwhile I'll be in touch.

Edit:
A first approach on merging the functionality: https://gist.github.com/2559689
As I see it, it should work for returning users by default, and if the user wishes to get the various course profiles for the user, specifies the addcourseprofiles parameter to true.
The return will be different from all the other functions, as the several course profiles will be nested inside one user.
What do you think of this approach?

Fábio

Hi Fabio,
I submitted MDL-31520.

About this issue: I think we should not bother the client developer about course profile or system profile. The function should return the maximum of information it can. I guess you can remove $addcourseprofiles and consider it true all the time. If you think it's better to have it, I would set it to true by default to make it nice to the client developer. Otherwise all your code seems good to me.

Hello Jerome,

Just a clarification: when you say:
or $USER is enrolled in same course.

$USER is the user who is invoking the webservice I assume. So if $USER shares at least one course with the currently-being-checked user, and if $USER has the capability of viewing details of the user, we return the user profile.
But, in this case, we also have to append course info, right?
I was thinking to append to the user info the following:

courses => multiple (
id
shortname,
fullname,
idnumber,
visible,
roles => multiple ( roleid, rolename blabla.. //a user can have several roles in a course )
)

So for each common course $USER has with the current user, we append a course information.
But suppose, if an admin $USER is calling, should it be able to know all information about all courses the user is in? Should all this information be returned?

Fabio

I agree that would be useful but it makes the function way more complicated. I was thinking all these course info should be retrieved with get_enrolled_users().

AH ok. That simplifies things.
So what should be returned, is this set of attributes, that are only related to the user:

                    'id'    => new external_value(PARAM_NUMBER, 'ID of the user'),
                    'username'    => new external_value(PARAM_RAW, 'Username policy is defined in Moodle security config', VALUE_OPTIONAL),
                    'firstname'   => new external_value(PARAM_NOTAGS, 'The first name(s) of the user', VALUE_OPTIONAL),
                    'lastname'    => new external_value(PARAM_NOTAGS, 'The family name of the user', VALUE_OPTIONAL),
                    'fullname'    => new external_value(PARAM_NOTAGS, 'The fullname of the user'),
                    'email'       => new external_value(PARAM_TEXT, 'An email address - allow email as root@localhost', VALUE_OPTIONAL),
                    'address'     => new external_value(PARAM_MULTILANG, 'Postal address', VALUE_OPTIONAL),
                    'phone1'      => new external_value(PARAM_NOTAGS, 'Phone 1', VALUE_OPTIONAL),
                    'phone2'      => new external_value(PARAM_NOTAGS, 'Phone 2', VALUE_OPTIONAL),
                    'icq'         => new external_value(PARAM_NOTAGS, 'icq number', VALUE_OPTIONAL),
                    'skype'       => new external_value(PARAM_NOTAGS, 'skype id', VALUE_OPTIONAL),
                    'yahoo'       => new external_value(PARAM_NOTAGS, 'yahoo id', VALUE_OPTIONAL),
                    'aim'         => new external_value(PARAM_NOTAGS, 'aim id', VALUE_OPTIONAL),
                    'msn'         => new external_value(PARAM_NOTAGS, 'msn number', VALUE_OPTIONAL),
                    'department'  => new external_value(PARAM_TEXT, 'department', VALUE_OPTIONAL),
                    'institution' => new external_value(PARAM_TEXT, 'institution', VALUE_OPTIONAL),
                    'interests'   => new external_value(PARAM_TEXT, 'user interests (separated by commas)', VALUE_OPTIONAL),
                    'firstaccess' => new external_value(PARAM_INT, 'first access to the site (0 if never)', VALUE_OPTIONAL),
                    'lastaccess'  => new external_value(PARAM_INT, 'last access to the site (0 if never)', VALUE_OPTIONAL),
                    'description' => new external_value(PARAM_RAW, 'User profile description', VALUE_OPTIONAL),
                    'descriptionformat' => new external_value(PARAM_INT, 'User profile description format', VALUE_OPTIONAL),
                    'city'        => new external_value(PARAM_NOTAGS, 'Home city of the user', VALUE_OPTIONAL),
                    'url'         => new external_value(PARAM_URL, 'URL of the user', VALUE_OPTIONAL),
                    'country'     => new external_value(PARAM_ALPHA, 'Home country code of the user, such as AU or CZ', VALUE_OPTIONAL),
                    'profileimageurlsmall' => new external_value(PARAM_URL, 'User image profile URL - small version'),
                    'profileimageurl' => new external_value(PARAM_URL, 'User image profile URL - big version'),
                    'customfields' => new external_multiple_structure(
                        new external_single_structure(
                            array(
                                'type'  => new external_value(PARAM_ALPHANUMEXT, 'The type of the custom field - text field, checkbox...'),
                                'value' => new external_value(PARAM_RAW, 'The value of the custom field'),
                                'name' => new external_value(PARAM_RAW, 'The name of the custom field'),
                                'shortname' => new external_value(PARAM_RAW, 'The shortname of the custom field - to be able to build the field class in the code'),
                            )
                        ), 'User custom fields (also known as user profil fields)', VALUE_OPTIONAL),
                    'preferences' => new external_multiple_structure(
                        new external_single_structure(
                            array(
                                'name'  => new external_value(PARAM_ALPHANUMEXT, 'The name of the preferences'),
                                'value' => new external_value(PARAM_RAW, 'The value of the custom field'),
                            )
                    ), 'User preferences', VALUE_OPTIONAL),

If so, that makes it clearer

So, after "some" thinking and programming, here it is.
The logic is as is:

• Get a major set of users filtered by the specified criteria, if it exists.
• Try to obtain user info through system profile, if the capability exists.
• Else try to obtain user info through course profile, if at least one of the capabilities of the course that the user is enrolled allows it to.
• There is also a case when the user is the same as the one who is invoking the webservice or if he is a course contact.

I have created an extra function on user/lib.php that leads with those things and to reuse the existing function user_get_user_details, that deals with the capabilities.

Submitting for peer review. It would be great (in a way that my master thesis ends in June so, without this I'm screwed ) if we could integrate this on 2.3, meaning only three weeks left I guess...

Edit: there are some search keys that probably need capabilities to be used, like username or email, but I didn't find which ones to use. What would you advise for this?

Cheers,
Fabio

I can not promise anything Fabio as you know it takes quite sometimes to go from the specs to finally be integrated. Plus it's code freezing the 7th of June. I'll review it today, I'll keep watching for your changes this weekend so we can accelerate the process.

My peer-review:

• you don't need to redefine $basefields in user_get_user_details_courses(), neither to check if $userfields is empty. It's all going to be done in user_get_user_details()
• change value PARAM_TEXT for PARAM_RAW. If later we add new keys it could be less restrictif than PARAM_TEXT.
• use PARAM_USERNAME for username
• At this moment you implemented AND SQL for the search. This must be mentioned in the function description. If we keep AND SQL and we make the criterias a external_multiple_structure, then we could support search on multiple usernames/emails/... and also OR searches.
• some Moodle sites have million of user (moodle.org for example). It is really essential to look at the performance issue as one request could take seconds to run. Luckily the user table is indexed on common fields (username, email, idnumber, firstname, lastname, auth, idnumber ). The general rule is to search on one field at the time and to not do any LIKE with left %.

Let's review each search fields:

• the dev client can search by username if:
  • siteadmin($USER)
  • it's the own $USER email
    In all other cases the users should not be returned by the search.

• the dev client can search by email if:
  • the $USER has the capability 'moodle/course:useremail' on COURSE_CONTEXT
  • $user->maildisplay == 1
  • $user->maildisplay == 2 and enrol_sharing_course($user, $USER)
  • it's the own $USER email
    In all other cases the users should not be returned by the search.

• the dev client can search by idnumber if:
  • the $USER has the capability 'moodle/user:update' on SYSTEM_CONTEXT
    In all other cases the users should not be returned by the search.

• the dev client can search by auth if:
  • the $USER has the capability 'moodle/user:update' on SYSTEM_CONTEXT
    In all other cases the users should not be returned by the search.

• the dev client can search by fullname if:
  one of 3 possible searches should be done following the code logic in fullname():
  • one concatening firstname + lastname
  • one concatening lastname + firstname
  • one on firstname
    Note that the concatening search are not indexed so they will be terribly slow on massive sites. We need to indicate in the description than searching on fullname could be very slow on big sites (Moodle.org fullname search > 10 seconds).

For each of these fields I think we should do one very fast request on the field. We would then exclude the results that don't match the conditions mentioned above. Then we'll end up with an "allowed to be searched" user list. The rest of the code should be similar to what you have Fabio.

To setup your site for performance testing, use /admin/tool/generator/index.php to create many many users.

Thanks Jerome, I do have a failsafe of course (creating a local plugin) but I would like to avoid creating it, as I'm doing code which is useful to the community and I'm glad to help moodle progress, as I'm working with it for a few years now. But this shouldn't worry you at all.

We'll see how it goes. Thank you for the quick feedback. I'll start working on those issues asap.

Cheers,
Fabio

I've been working on this,
In the case where we don't have criteria to search, I can't do that initial "allowed to be searched" group.
So how do I know what users I can return?

On my current implementation I do a simple query to get all users, and then I iterate over the results so that the underlying get_user_details decides what to do. I will keep it like this for now, but if you have a better idea let me know. Performance-wise it can be heavy, but I don't see a true alternative to it.

Fabio

Edit: new one submitted, now using your algorithm. From my tests it vastly improved performance. I created around 25 000 users and done some searchs. Without criteria the thing gets slower though, as I explained before.
I have some questions regarding the use of IN in the query (ll. 471-474) , due to different databases (I'm not sure if they all support it as I use it, but from my experience I think they do: mysql, postgre, mssql and oracle at least).

(Once again) thank you for all,
Fabio

Hi Fabio, actually I though we would throw an exception when no criteria are mentioned (need to be indicated in the description for the client dev). There is still core_enrol_get_enrolled_users() that let you retrieve users for a specific courses. However the way you implemented without criteria seems correct to me. So it's really about performance. And if the performance are very bad I suppose for the moment it would be good to throw an exception. If later we decide it is required to return any users and that other existing functions don't do it, it will be easy to enable without breaking existing client.

Peer review :

• recently we integrated MDL-30986, have a look at it, it shows how phpdoc should be formatted.
• when the user has no capability to search by idnumber => throw exception otherwise the client dev will just think they don't exist. I think client developers who are going to implement a search by idnumber should not let unknown users search by idnumber. It is the same for username/auth, throw an exception too.
• do not let people search on firstname/lastname. What I meant it is to search by fullname. If you look to the Moodle function fullname() you will find which request can be executed. Sometimes fullname == (firstname + lastname), sometimes fullname == (lastname + firstname) and sometimes fullname == firstname. If you look to fullname() you'll find the logic to implement.
• for your IN SQL question there is a function $DB->get_in_or_equal() that you might use (not 100% sure but I suppose it should work). Have a look at /lib/dml/moodle_database.php there are plenty of $DB helper function that protect you against cross-db issue if you call them.
• we should mention on a //docline that we do the big SQL request on allowed users in the only purpose to save multiple get_context_instance sql requests.
• did you skip the comment about doing bulk search on username/id/... by purpose? I think it would be useful. For example in MDL-27985 Luis mentions he's interested to search by multiple idnumber, which makes a lot of sens for some site that use idnumber to identify their users. But in counterpart it makes the web service call more complex (it adds again an array layer to the parameters). Let me know what you think.

That's all. Can you add test instructions (mention "use the demo REST client (https://github.com/moodlehq/sample-ws-clients/tree/master/PHP-REST)" and indicates the $params=... to be tested.). I'll test this function after your change.

Cheers,
Jerome

After reading at http://moodle.org/mod/forum/discuss.php?d=201670#p880276 I think some devs are interested to get all users from Moodle (no criteria). So it would be good to implement it as you did. However mention clearly it's going to be very long for big site. I remember we tested retrieving all users from Moodle.org forums by web service, it took many minutes - not mentioning it was crashing the mobile device - only worked on the simulator.

Note: the best solution would be to offer a 'getmore' parameter where client are served by X number of users. I would suppose that when there are some criterias we should do the 'getmore' logic on a final sorted php array of users because it would not be possible on the multiple SQL requests... (however when no criteria it's easier to do the getmore selection in the SQL). Anyway if you don't have enough time I would implement this 'getmore' later, better to have a known slow "get my million of users... I can wait" than nothing.

Hello Jerome,

At least in my case, it would be useful to have a no criteria option, where in one call I could get all the Moodle users that were created.
Having to specify criteria without having a notion of the users that exist on Moodle in the first place is "tricky". So at first we need to have a way to get users without knowing what we're searching for, right?

The getmore is a limiting of results I suppose. I will take a look at the remaining issues and post my thoughts as I go.

Thanks!

Edit: so I was wondering through the datalib functions, trying to get a function that would help me get users...
Take a look at lib/datalib.php ll. 194-247
I was thinking that for getting users by email, fullname or username we could use this function, instead of reinventing a whole lot of logic for the first quickfetch (fullname is particularly complex).

The second query, I'm still thinking how to make the search by fullname.

So I've been playing with those queries for a bit... the conclusion that I'm reaching is that searching for fullname is a no-no.
In a search for the Admin User (fullname) on a DB with around 50 000 users, my computer hanged. Even after closing the php script, a background thread kept on going for almost 5 hours, and when I noticed it, it was too late ( ). This is because search by fullname is a concatenation of firstname + fullname, followed by a search with a left %. But even without the left %, the same happens. It is not feasible.
So, in my opinion, we should drop fullname/email (because they're big text fields) and keep only id, idnumber, username, auth, and maybe a few others, which are faster for searching (they are searched for equality). But it limits the function a bit. What do you think? Do you have a better idea?

PS: the gist for the code I was using (without left % searches): https://gist.github.com/2628358

—
Well, turns out it was a bug... The wheres array wasnt being set for fullname, so it was returning all the users, which is simply too much (50 k users... get all the contexts and stuff). So I really need to add a limit to the returned users, for the case without criteria.

I have pushed my latest code, which limits results. I did not ignore the possibility of specifying several sets of idnumbers/ids/... to be searched, I just didn't program it yet. Sorry for the wall of text and once again thank you.

Fabio

Hi Fabio,

• I forgot about the get_users search in datalib. It's doing left % so it's going to perform badly. I don't think your code is far away from been submitted without using this function.
• getmore => limitfrom + limitnum. Yes it's what I meant.
• In fact I wonder if we should allow to search on LIKE XXX%. The dev client should give the correct matching fullname/email. Later we could have a search parameter if necessary. From the criteria switch I would replace the LIKE by normal equal SQL condition. Email should be very quick to be retrieved. I remember implementing a search on email with a %, it was searching in less than a second with half a million of users. Fullname will still be very slow as not indexed.
• you can also remove this piece of code as the allowed users are already in the IN () part of the SQL. It should make the function faster:

  if (!empty($searchemail)) {
                  $searchemail = trim($searchemail);
                  $usersql .= " AND ".$DB->sql_like('email', ':searchemail', false);
                  $conditions['searchemail'] = "$searchemail%";
              }
              if (!empty($searchfullname)) {
                  $fullname = $DB->sql_fullname();
                  $searchfullname = trim($searchfullname);
                  $usersql .= " AND ".$DB->sql_like($fullname, ':searchfullname', false);
                  $conditions['searchfullname'] = "$searchfullname%";
              }
  

• actually about multiple id as parameters, your code already works. The array of criteria can contain multiple key == id.

Hello Jerome,

I can't remove that code I think. The first filtering is only to get allowed users to search, then afterwards we build a SQL query with the allowed users, but specifiyng AND, so that the result respects all of the given criteria. That is, the criterias are AND'ed together. Are you suggesting that one criteria gets one user, instead of AND'ing them together?

Currently multiple id's doesn't work because i'm filtering duplicates at the beggining of the foreach for params['criteria'].

Hi Fabio,

• multiple ids: yes. Maybe the web service function parameter will finally need to be array of array of objects... but it's not very friendly.
• for the filtering:
  I would apply this algo for each criteria:
  • it's the first criteria filtering: $allowedusers = 'criteria SQL' and $searchedacriteria = true.
  • if ($searchedacriteria == true) then remove the 'criteria SQL returned users' from $allowedusers.
  • if ($searchedacriteria == true && empty(allowedusers)) no need to execute more SQL requests in this external function. No user will be returned anyway.

I guess this algo will save the final SQL to have ANDs, which should make the sql faster. It will also save some criteria SQL requests when the algo stop. However it's not going to make multiple set of criteria (e.g. multiple ids) easier to implement

PS: if you find better algos go for them, it's just a suggestion.

I don't know if I quite follow your algortihm, but I did think of another:

• For each criteria, intersect the result of the SQL with the already existing $allowedusers. Those who remain have all the previous criteria in common.

This will only work for joining criteria with AND, and would avoid the second big SQL query to have ANDs

What do you think?
About the multiple ids... I'm still very skeptic to the idea. How would the user know which returned users would correspond to which set of criteria? Also it will complicate the function even more.

Edit: I implemented the algorithm that I explained above. All that's missing now is the multiple id's (if it's to be done).
On my installation, I have tested this implementation with 50k users, and requested for all users with auth = manual. It took a while to execute.
This is due to the second SQL query, which is joining all the 50k users with their contexts, and then limit the result to 30. It seems to me quite inefficient in those situations. So to avoid this I've paginated the allowed users before submitting them to the SQL query as well. So it's blazing fast now

Tell me what you think.

Fabio

Hi Fabio,

• for each criteria "if (empty($allowedusers) and !$firstcriteria)" do not execute the criteria sql: the intersection is going to be empty anyway.
• for the multiple ids, it would be possible to accept as parameter:
  • either multiple time the same criteria key
  • either different criteria key but only one occurence of a key (your current code)
  • throw an exception when both are sent.
    What do you think? I think it should not be too complicated, something like that: IF unique criteria key DO one sql on the key (idnumbers or usernames or ...) ELSE execute your current switch THEN continue on the current final sql.

You're very close to submit. The code looks good and the pagination will make it usable for everybody. Thanks Fabio.

Hello Jerome,

I'll start working on it asap.

Hello again,

I've finished doing the changes you suggested, they seemed OK.
So now we can have two search types:

• Specify several times a key (fullname => 'user1', fullname => 'user2', ...)
• Specifiy different keys only once (fullname => 'user1', auth => 'manual', ...)

Any other use will trigger an exception, as it will be illegal.

Amd that's pretty much it, it works
I'll update the testing instructions.

Hi Fabio,
you just missed some explanation into the description (so client dev can see the description in the generated doc). I added a commit, submitting to integration. Yeah !

Oh, the joy!

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

TIA and ciao

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

TIA and ciao

Hello Jerome,

Since you're encharged of the master pull rep, I ask you to change a tiny bug on the get_users_parameters() function, namely a parameter description error (similar to the one you fixed at get_categories a couple of days ago).

The description order is wrong, on the criteria field, it should be before the VALUE_DEFAULT and array(), but it is after it.

Thank you,
Fabio

Hi guys,

Unfortunately this is going to be delayed a week sorry.
This is a master only changes and presently we have to keep master and 23 in sycn until the next minor release on Friday.
I've had a quick look at it anyway as I imagine you are both keep to see this get in and I could instantly see a couple of things that got me thinking when I looked at it.

The very first thing that I noted and I think needs some discussion is about the criteria.
The way in which two means of passing criteria into this new service instantly suggests to me that something is a miss.
Just quickly to summarise the two means of selecting users are:

1. Based upon a single field searching for multiple values.
2. Based upon multiple fields with the restriction of one value per field.

Without going any further I think either
A: There should be only one search function and it should accept multiple fields and multiple values for each field. This I'm sure could be achieved by reorganising the structure of the params that get passed in and with the addition of an optional aggregation param for each field.
B: There should be two functions the first for searching allowing multiple fields each with a single value and the second a single field with multiple values.

I think without that separation (one function that does it all, or two specific functions) things are liable to get confusing for developers of utilising this service and likely people will start/continue to write their own services.

So that initial reaction to the criteria organisation was the first thing.
The next thing I noticed was the way in which filter_user_to_criteria is being used both when it is diverse criteria and when it is not.
If the criteria are considered diverse that function is called once and is provided the array of criteria. For each field it calls get_records to get users matching the value given for the field.
The results of that statement are then combined with the existing results using an intersect.
This is a pretty unefficent way to process, imagine someone provides the criteria (username => johndoe, email => '%gmail.com'). Now we know thats not a smart idea, but someone out there will probably write code like that to ascertain if there is a user called johndoe who is using a gmail email.
Because of the way things presently are a quick search will be run and return one record for the username, and then a painstaking query will be run for email. If that was built into a single query and executed it would be quick, but as it presently its going to be painful.
Certainly I think this highlights a need to consider building queries rather than executing a query for each field.
When the criteria are not diverse handling is better, although perhaps convoluted in code as the params['criteria'] array is still being iterated over despite us knowing it has only a single record. Certainly had to read it very carefully to check what was going on there.

Just quickly to reinforce the build a query argument, if you build a query you can preload as part of that query so there would be no need to run yet another query to preload contexts after we have collected all of the users (thank you for thinking of preloading though!)

The final thing to mention is fields.
At the moment this service supports only a very limited number of user fields, much less than other user services.
I think either as part of this issue, or in an issue created when this is integrated we need to address that.

In summary I think the approach needs to be looked at here. Much of the code is good, I just think it is in the wrong form, and I do think it needs to be addressed.
At the very least it needs to be discussed.
Personally I think the creation of a single complete search service that supports a many to many relationship between fields and values is needed.
I think that beats splitting the service into two, if only because it is the more complete option.
An approach where criteria were "AND"'d and values "OR"d would be spot on for the first iteration, and organisation of aggregates could perhaps be looked at later.

Anyway, I think the best thing to happen now, given this issue is being delayed for a few days would be for both of you Fabio + Jerome to have a read of this and share your thoughts.
I'm not saying no to this solution at this point by any means but I'm very keen to hear your thoughts. For sure we will have to properly consider things during integration either way.

Cheers
Sam

Hello Sam,

Thanks for the feedback. It seems that me and Jerome are a bit busy at the moment, but I'll make an effort to return to this as soon as possible. Meaning that, it won't happen probably until Aug.

Simplifying usually means better solutions, so I'm all in favor of creating two separate webservices. That's the webservice philosophy anyway: a set of simple, distinct functions. But perhaps Jerome has a different view on it. For what I understand you prefer a unique, more complex function.

The complexity of the function is mainly because of the need of camouflaging the existence of course profiles and system profiles. It doesn't embarrass me to say that a much better function could be created, because I am a bit inexperienced on these kind of developments. So there is much room for improvement, as you suggest.

Finally I agree on most of it. Specifically the querying for each criteria, which seems to me very inefficient. If only I have written this code, it would have been so much better.... Oh wait, it was me... What the hell was I thinking

Cheers,
Fábio

Hi guys, go for the two simple functions, it seems the way to go.

For the different criteria queries it is to perform a very quick search on indexed fields. From what I remember Eloy told me it is much faster this way (and I did obtain almost instant result on million users search when I wrote a user selector 'a la facebook', see my last commit on https://github.com/mouneyrac/moodle/commits/wip-MDL-30140: https://github.com/mouneyrac/moodle/commit/6113b2a731ce04e42d600b05d13740e2291af718#L7R106). But if you can obtain decent search result time with one relationship query (with/without non indexed field), it would be much more short and readable code.

I'm reopening this, as it seems there is concencous to move adifferent direction to the attached patch.

cheers.

Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

Whats the progress of this? I would really need some kind of get_users_by_idnumber in the weekly releases.

Hi Fabio, if you don't mind I'm taking over the issue. Creating another issue for search by field: MDL-35543

Sure, no problem. Let me know if I can help with something.

I marked the issue as blocked by MDL-35543.

Hi Jerome, a few points:

1. The list of capabilities in the array could be put on two lines.
2. You should change the use of PARAM_USERNAME in create_users_parameters() to PARAM_RAW as PARAM_USERNAME should not be used when syncing with external systems (as stated in the code).
3. You changed the param type for the username in get_users_by_field from PARAM_USERNAME to PARAM_RAW. However, I notice PARAM_NOTAGS is used in login/signup_form.php and PARAM_RAW in other places - strange.
4. The get_users_parameters function has some unnecessarily large spacing between the keys and the values in the array.
5. I would change the word of "Note: you can use % for searching but it can be slow!" to "Note: you can use % for searching but it may be considerably slower"
6. You have "auth" (plugin), do you mean string here?
7. The PHP doc for get_users is incorrect, it specifies two parameters, when there is only one.
8. There is a typo "Specifiy".
9. There is a typo "containg".
10. In the function get_users you use PARAM_RAW for the 'idnumber' but in user/editlib.php PARAM_NOTAGS is used.
11. You have two '$' signs before criteria on line 552.
12. On line 576 of the same function you do not put a space between concatenating ':' and $criteria['key'].
13. I don't understand the usage of validating that the user has all the criteria again in the foreach loop on line 597. Isn't this already done when you use AND in your SQL statement?
14. There have been two randomly introduced lines at 696 and 697.
15. The get_users_returns function uses user_description() to specify what fields will be returned, however wouldn't this vary depending on what was passed to the function get_users? The function user_description returns a whole list of variables which will never be returned by this function.
16. The user_description function on line 835 takes a parameter '$additionalfiels', but this is called as '$additionalfields' when used throughout the code. Meaning (!empty($additionalfields)) will never be true. The PHP doc is also misspelt.
17. The comment "also known as user profil fields" has profile misspelt.
18. The get_users_by_id_returns calls core_user_external::get_users_by_id_returns($additionalfields); which does not accept this extra parameter.

Generic Issues.

1. Your comments are missing "." at the end of them, and some do not start with capitals (Moodle guideline, I am not being anal retentive).
2. Occasionally you leave a line after a foreach and sometimes you do not, I like to keep this consistent.

Thanks a lot Mark, here are the things I'm not going to change:

1. Agree, but I keep it like that because it's not longer that other ones around.
2. I understand your point and I think it's a good point. However I cannot change it without breaking the function as it is validating against the Moodle policy in user_create_user(). At this moment the external function is kind of matching manual enrolement only. This may be improved later for client admin tool devs. No-one asked for this improvement yet.
3. Yes I know, but it's ok like that. For a return value it's not a big deal - at the end devs should never trust the inputs and should always clean them. I choose PARAM_RAW because I could imagine a Moodle site creating/hacking usernames with no restriction at all. I didn't want the web service failing because these sites use weird username.
6. I didn't change it, it's the previous code. I think it's a string that must match PARAM_PLUGIN format.
10. I didn't change it, it's the previous code. I think you are right, but for same reason I explain in 3. it doesn't break/damage anything here. Note that I would not write an issue for that it as it's never good to restrict PARAM_XXX (once one PARAM_YYY has been chosen and integrated) for backward compatibility and it's pretty minor.
13. it's because user_get_user_details_courses() will unset the field that the user is not allowed to access. So if you did search on this field, but in fact are not allow on it, then you should not be able to search on it. So I need to check that.
15. you can not change/generate on the fly a description (otherwise you'll break soap client expecting an specific wsdl). The best way is to mention all possible fields returned and mark them optionals. The server will not return them any way if they are not present, the _returns description is used to check that a value is allowed to be returned.

Thanks for all your catches

Thanks Mark I fixed all the rest + some change in the PHPunit (check result against the description + fix enrolment call)

Note: Don't forget to add your function to the WS API list: http://docs.moodle.org/dev/Web_services_Roadmap#Core_web_service_functions (this is an automatic message)

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

Cheers!

Hi Jerome,

This looks good, but I noticed a few small issues so i'm going to need to reopen this:

1. In get_users() - It's not safe that you let the loop continue after an invalid parameter has been found. It it would be safer to use continue; after you set the $warnings. Imagine for example that the first of multiple crtiteria will be invalid, you'll set the $sql to ' AND ' as the start and create ab invalid sql statement. I proved this problem with a change to the unit tests, please ensure this doens't cause an invalid sql error:

   diff --git a/user/tests/externallib_test.php b/user/tests/externallib_test.php
   index eed9fce..5491720 100644
   --- a/user/tests/externallib_test.php
   +++ b/user/tests/externallib_test.php
   @@ -168,6 +168,12 @@ class core_user_external_testcase extends externallib_advanced_testcase {
                    $this->assertEquals(implode(', ', $generateduser->interests), $returneduser['interests']);
                }
            }
   +
   +        $searchparams = array(
   +            array('key' => 'foo', 'value' => 'bar'),
   +            array('key' => 'firstname', 'value' => 'bob')
   +        );
   +        $result = core_user_external::get_users($searchparams);
        }
   

   (It'd be good if you expaneded that into a proper testcase verifying behaviour on incorrec tinput)

2. The unit tests should avoid logic where possible (see http://xunitpatterns.com/Conditional%20Test%20Logic.html ). So I really don't like the look of all your if (empty()) { checks in your tests. It looks like only 1 user should be returned and in that test you know exactly what fields should be expected to returned, so please test these fields explicitly, don't do it conditionally. Avoiding logic in tests avoids bugs in the unit tests themselves.
3. Regarding the PARAM_ incostiencies, it would be good to haven issue tracking these so we can decide what to do about them once and for all (rather than getting lost in the good comments here)

thanks,
Dan

Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

Thanks Dan for catching the issue with invalid criteria. Resubmitting.

Note: I didn't use continue I found it ambiguous as it is in a switch, we would need two. Also to keep the same logic (ignore invalid keys) I unset the invalid key.

I created https://tracker.moodle.org/browse/MDL-37960

Hi Jerome, I'm sorry but think this needs another round:

A) It seems, from the description of the issue that this new core_user_get_users function was going to land to deprecate core_user_get_users_by_id and core_user_get_course_user_profiles. Where is the the deprecation defined? Which are the associated issues where we'll take rid of them, for which version?

B) Whitespace, there are a bunch of lines with tabs.

C) What happens if one search criteria is repeated? I've seen some comments about only the last one being considered by have failed to find where the others are ignored. Perhaps it's a good thing to get that covered by some test?

D) There are a bunch of core_user_external::xxxxx() uses within the core_user_external class. IMO all them should be self:: instead.

E) I've seen that the whole function doesn't take into account at all both mnethostid and deleted. The 1st is important because it makes possible to have repeated usernames, and the 2nd too because I don't know which is the expected behavior for deleted users. Needs some direction.

F) Some "foreach(" occurrences and some inline comments not starting by upper and ending with dot (these are not important, just reporting them).

G) The inconsistencies about which param type to use for usernames... I've seen the new issue, yes, but shouldn't we decide that before allowing this to land? Just to think about.

So, reopening, ciao =

Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

Thanks for the review Eloy.

Won't fix:
A) it's get_users_by_field() which replaces them. get_users() is a search function. I'll write an issue for deprecating these functions in favor of get_users_by_field(). (MDL-38030)
B) since a bit more than a month I've only been using vim which is even better than netbeans (this was an advertising). It's very ultra highly improbable I added any tabs with both of them. I checked, the existing tab are from somewhere else. I think the good practice is to not edit a line which is not related, otherwise the git blame mean nothing. Let me know if I'm wrong, often I restrain myself to fix these.
G) For the specific username case: I consider the ws core_user_create_user() as a "manual" create_user() so same input validation => PARAM_USERNAME. However any external authentication system can create a PARAM_RAW username if I understood. So ouput => PARAM_RAW
F) same as B) I think your speaking of other code location.

Will fix:
C)
D) I was surprised you mentioned it but it's worth it from reading that: http://stackoverflow.com/questions/3481085/self-vs-classname-inside-static-classname-metods-in-php
E) After thinking I don't think it's useful yet to return mnethostid because it currently means nothing to the client. Moreover the issue really come from the core function not returning it. However I agree that it will be useful. So I created MDL-38045. Fixing the "deleted user" issue thought.

Done.

Hi Jerome,

Just coming into look at this issue I need a couple of things explained to me.

DA) Why the duplication of functionality between core_user_get_users and core_user_get_users_by_field?

DB) The performance of this webservice looks terrible (and it is a hard one to fix because of the design of the function). In Moodle, when getting a list of users, first the visible fields are worked out by the context of the user list (e.g. is it in a course) - then, all records are retrieved in one call and the list is shown. In your function, you get a list of users and then workout which fields are visible for each user in the list - this will not scale well.

DC) These new functions in user/externallib.php should be in user/lib.php so they can be reused outide of the external API. IMO except for very simple cases, I don't think the functions in externallib.php should be making DB calls directly (and especially complex searches filtered by permissions).

DD) Whitespace error in user/tests/externallib_test.php

DE) Not convinced by your argument for not returning mnethostid - it seems like a critical piece of information.

Thanks, I'll wait for your feedback.

My answers

A) get_users => search: email = mark@moodle.com, firstname = mark, ....
get_users_by_fields => id=3, id=2, id=5
Initially we did everything is one function, but it was unmaintainable, so we clearly separated the function in two functions: https://tracker.moodle.org/browse/MDL-29938?focusedCommentId=167642&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-167642
B) this would require a new search core function. I recommend to create an issue.
C) see above.
E) dealing with mnethostid impact user_get_user_details() core lib function which impacts some external functions. In this case I need to fix it first, I would become even messier to commit into this issue. What I suggest is good in my opinion, integrate the current code and add the mnethostid later. Except if you tell me that all client need this information (and I doubt it strongly except very few admin clients)

Some consideration

I would love to do B) and E). And also refactoring the front end to use the function created in B). And all that in this same issue. But I do believe we should have smaller issues, so it's:
a) reasonable amount of dev time
b) trackable for the project managers
c) easier to peer-review
d) avoid even more integrator ping-pong (I feel we are going in circle with this issue. I think we had all integrators in this issue asking sometimes some questions we already dealt with)

What I suggest

create the issue for B). I already created the issue for E). Then you decide if you want to integrate the current code. If yes, you explain in B) that the dev will need to move the code from the current external lib function to a core lib function. If no, then you mark this issue as blocked by B) and E). This issue would become then very easy as we would just need to call the code created in B). However the issue will be put on the side again (and I'm starting to be scared for another year?).

PS: we can also rename this function in search_users() which is what it is. It would be less scary for you to integrate. The reason I didn't call it search_users it's because "we" want to restrict the external API at maximum with get/create/update prefix.

Will get some more integrators opinions on B) so we don't all tell you different things.

MY opinion is that the API should have a specific courseid parameter (optional implies system context) so you can determine the list of visible fields before you do the search rather calling user_get_user_details for each user in the list. I think this applies to both get_users and get_users_by_fields.

After talking with Damyon the idea is:

• optional courseid => return the course profile
• no courseid => return system profile

This implies that a phone number that would be available in one course profile only, would not be displayed on a request for a different course profile. Which I find weird from a web service API point of view. It's matching how the code logic behave in Moodle but in my opinion it's not matching what's the user can do in Moodle... the user could see the phone number with multiple click/click navigating from one course to another. We kind of force the client devs to create similar navigation.

If we go this way (and I don't mind... at the end I have a limited experienced creating clients, it may be the right solution, checking every courses may be overkilled afterall), it will be required to fix the get_users_by_fields() too before we release MOODLE_25_STABLE.

There are some integrator votes for getting this integrated and dealing with the performance issues if they arise separately in order to get progress on this issue - I'll take another look in a few minutes.

Thanks Jerome I have integrated this issue as is. I'll vote for your issue for the missing mnethostid and we can leave the performance issue and see if it is reported when people start using it.

Sorry for the tabs Eloy/Damyon. I did introduced them and more problematic I have no idea how they came in.

Ran the unit tests on my own machine. Passing.

Thanks Andrew. I updated http://docs.moodle.org/dev/Web_services_Roadmap#Core_web_service_functions

Thanks for your hard work - this issue has made it! Moodle is now a little bit better.

Regards, Damyon

I am using Moodle 2.0 Rest API in java to call core_user_get_users function and it is displaying the following error,

<EXCEPTION class="invalid_parameter_exception">
<ERRORCODE>invalidparameter</ERRORCODE>
<MESSAGE>Invalid parameter value detected</MESSAGE>
<DEBUGINFO>Missing required key in single structure: criteria</DEBUGINFO>
</EXCEPTION>

I used link similar to :
https://moodletest.domo.com/moodle/webservice/rest/simpleserver.php?wsusername=%s&wspassword=%s&wsfunction=core_user_get_users

It would be highly appreciated if someone could help me in how to pass the criteria parameter ?

Is there a way to make this happen in SOAP (WS)? I think the arguments are not so clear...enough.