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:

      Description

      Return users matching a specific field

        Gliffy Diagrams

          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: