Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: Future Dev
    • Fix Version/s: 2.5
    • Component/s: Web Services
    • Labels:
    • Rank:
      44255

      Description

      Return users matching a specific field

        Issue Links

          Activity

          Hide
          Rossiani Wijaya added a comment -

          Hi Jerome,

          Some feedback for the patch.
          user/externallib.php

          1. Line 274: @return should be stated last and it is missing return type
          2. Line 375: @since should be 2.4?
          3. Line 381: 'the search field can be:id, idnumber, username, email' -> it needs 'or' indication
          4. Line 394: @since should be stated right after the function definition.
          5. Line 457: missing @return type
          6. Line 458: @since should be stated right after the function definition with 2.4 version?
          7. Line 469: email - can it use PARAM_EMAIL instead of PARAM_TEXT?
          8. Line 491: descriptionformat - missing param type on first param

          user/lib.php

          1. Line 489: typo for 'through'
          2. Line 491: param type definition for documentation
          3. Line 492: return type should be null|array
          4. Line 502 - 513: make the logic friendlier
          5. line 525: Change the definition for the function.
          6. Line 526: remove the question
          7. Line 528: missing param type for $user
          8. Line 529: $course param type should be null|stdClass
          9. Line 530: return definition should be word out instead of using T for true

          Other than that it looks great.

          Check list:
          [y] Syntax
          [y] Output
          [y] Whitespace
          [-] Language
          [-] Databases
          [y] Testing
          [-] Security
          [-] Documentation
          [y] Git
          [y] Sanity check

          Show
          Rossiani Wijaya added a comment - Hi Jerome, Some feedback for the patch. user/externallib.php Line 274: @return should be stated last and it is missing return type Line 375: @since should be 2.4? Line 381: 'the search field can be:id, idnumber, username, email' -> it needs 'or' indication Line 394: @since should be stated right after the function definition. Line 457: missing @return type Line 458: @since should be stated right after the function definition with 2.4 version? Line 469: email - can it use PARAM_EMAIL instead of PARAM_TEXT? Line 491: descriptionformat - missing param type on first param user/lib.php Line 489: typo for 'through' Line 491: param type definition for documentation Line 492: return type should be null|array Line 502 - 513: make the logic friendlier line 525: Change the definition for the function. Line 526: remove the question Line 528: missing param type for $user Line 529: $course param type should be null|stdClass Line 530: return definition should be word out instead of using T for true Other than that it looks great. Check list: [y] Syntax [y] Output [y] Whitespace [-] Language [-] Databases [y] Testing [-] Security [-] Documentation [y] Git [y] Sanity check
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Thanks Rosie. I didn't check the codechecker/moodlechecker on it, I'm too used to get the smurf file from the integrators. Below, some explanation:

          user/externallib.php:
          1. http://docs.moodle.org/dev/Coding_style#.40return does not mention that @return has to be the last annotation. The return type external_function_parameters is correct. You can write the class name instead of 'object', it's actually better in my opinion.
          2. We promised the web service contributors that we'll backport web service function to 2.3 when possible, so I mention 2.3 yet (I don't know yet if it will pass the integration). I'll change for the right version => 2.3.X.
          3. Fixing. As the 'field' parameter is a string and is not an occurrence (it's not a list) I thought it was implicit enough, but I'll add an 'or'
          4. http://docs.moodle.org/dev/Coding_style#.40since does not mention anything about placement too.
          5. same as 1.
          6. same as 2.
          7. As it is mentioned in the description of this field "An email address - allow email as root@localhost". PARAM_EMAIL would fail root@localhost email address during the web service return value validation process. To be honest I'm not even sure about the rightness to do any email validation... We would need to check other email field of other external function and comment about that in Moodledocs.
          8. The code is correct. Note that is a external_format_value() not a external_value()

          user/lib.php:
          1. Fixing.
          2. Fixing.
          3. Fixing.
          4. Thanks for noticing the perf issue Rosie. I myself don't use this kind of logic, at the end we learned about the difference about |= and ||= (the first one test all condition, the second one stop at the first true), it was a good exercice
          5. Fixing.
          6. Fixing.
          7. Fixing.
          8. Fixing.

          Show
          Jérôme Mouneyrac added a comment - - edited Thanks Rosie. I didn't check the codechecker/moodlechecker on it, I'm too used to get the smurf file from the integrators. Below, some explanation: user/externallib.php: 1. http://docs.moodle.org/dev/Coding_style#.40return does not mention that @return has to be the last annotation. The return type external_function_parameters is correct. You can write the class name instead of 'object', it's actually better in my opinion. 2. We promised the web service contributors that we'll backport web service function to 2.3 when possible, so I mention 2.3 yet (I don't know yet if it will pass the integration). I'll change for the right version => 2.3.X. 3. Fixing. As the 'field' parameter is a string and is not an occurrence (it's not a list) I thought it was implicit enough, but I'll add an 'or' 4. http://docs.moodle.org/dev/Coding_style#.40since does not mention anything about placement too. 5. same as 1. 6. same as 2. 7. As it is mentioned in the description of this field "An email address - allow email as root@localhost". PARAM_EMAIL would fail root@localhost email address during the web service return value validation process. To be honest I'm not even sure about the rightness to do any email validation... We would need to check other email field of other external function and comment about that in Moodledocs. 8. The code is correct. Note that is a external_format_value() not a external_value() user/lib.php: 1. Fixing. 2. Fixing. 3. Fixing. 4. Thanks for noticing the perf issue Rosie. I myself don't use this kind of logic, at the end we learned about the difference about |= and ||= (the first one test all condition, the second one stop at the first true), it was a good exercice 5. Fixing. 6. Fixing. 7. Fixing. 8. Fixing.
          Hide
          Jérôme Mouneyrac added a comment -

          After talking on the dev chat, you were right Rosie, we should enforce PARAM_EMAIL everywhere, Moodle doesn't support email different from YYY @ DOMAIN.NAME

          Show
          Jérôme Mouneyrac added a comment - After talking on the dev chat, you were right Rosie, we should enforce PARAM_EMAIL everywhere, Moodle doesn't support email different from YYY @ DOMAIN.NAME
          Hide
          Jérôme Mouneyrac added a comment -

          I only added this function to 2.4 as it does some change into user/lib.php. I'd like integrator opinion on this as it is pretty similar to MDL-29938 (core_user_get_users()).

          Show
          Jérôme Mouneyrac added a comment - I only added this function to 2.4 as it does some change into user/lib.php. I'd like integrator opinion on this as it is pretty similar to MDL-29938 (core_user_get_users()).
          Hide
          Dan Poltawski added a comment -

          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

          Show
          Dan Poltawski added a comment - 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
          Hide
          Sam Hemelryk added a comment -

          $can_view_user_details_cap($user, $course)

          will look at this properly tomorrow now, Jerome is this needed (absolutely essential) in 2.4.1, or can is master only ok?
          Noting as this is a new feature it would land to just master.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - $can_view_user_details_cap($user, $course) will look at this properly tomorrow now, Jerome is this needed (absolutely essential) in 2.4.1, or can is master only ok? Noting as this is a new feature it would land to just master. Many thanks Sam
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Sam,
          it's not needed in 2.4.1.

          Show
          Jérôme Mouneyrac added a comment - Hi Sam, it's not needed in 2.4.1.
          Hide
          Sam Hemelryk added a comment -

          Awesome thanks Jerome, adding the integration_held label now.

          Show
          Sam Hemelryk added a comment - Awesome thanks Jerome, adding the integration_held label now.
          Hide
          Sam Hemelryk added a comment -

          Ok, this has now landed. Thanks Jerome.

          Show
          Sam Hemelryk added a comment - Ok, this has now landed. Thanks Jerome.
          Hide
          Rajesh Taneja added a comment -

          Thanks Jerome,

          Phpunit passes and webservice return matching user records.

          Show
          Rajesh Taneja added a comment - Thanks Jerome, Phpunit passes and webservice return matching user records.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And your fantastic code has met core, hope they become good friends for a long period.

          Closing, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - And your fantastic code has met core, hope they become good friends for a long period. Closing, thanks!
          Hide
          Jérôme Mouneyrac added a comment -
          Show
          Jérôme Mouneyrac added a comment - The function has been added to ws docs: http://docs.moodle.org/dev/Web_services_Roadmap#Core_web_service_functions

            People

            • Votes:
              6 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: