Details

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

      Description

      Return users matching a specific field

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              rwijaya 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
              rwijaya 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
              jerome 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
              jerome 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
              jerome 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
              jerome 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
              jerome 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
              jerome 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
              poltawski 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
              poltawski 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
              samhemelryk 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
              samhemelryk 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
              jerome Jérôme Mouneyrac added a comment -

              Hi Sam,
              it's not needed in 2.4.1.

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

              Awesome thanks Jerome, adding the integration_held label now.

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

              Ok, this has now landed. Thanks Jerome.

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

              Thanks Jerome,

              Phpunit passes and webservice return matching user records.

              Show
              rajeshtaneja Rajesh Taneja added a comment - Thanks Jerome, Phpunit passes and webservice return matching user records.
              Hide
              stronk7 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
              stronk7 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
              jerome Jérôme Mouneyrac added a comment -
              Show
              jerome 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:
                    Fix Release Date:
                    14/May/13