Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Course, Web Services
    • Labels:
      None
    • Testing Instructions:
      Hide

      General setup

      a) Create on second level category hidden with a not hidden sub category. The web service function should not return the hidden category, neither its sub category. Try again with a user having the capability 'moodle/category:viewhiddencategories', the function should return both categories.
      b) Create a third level category with a sub category. Set the max category level of your Moodle to 3. The web service function should return the third level category but not its sub category.
      c) call the web service function as a user having 'moodle/category:manage' capability, the web service function should return all categories (hidden categories and deep level categories).

      Take the REST demo client, then run each function tests:
      https://github.com/moodlehq/sample-ws-clients/blob/master/PHP-REST/client.php

      core_course_get_categories

      /// PARAMETERS

      $params = array('criteria' => array(array('key' => 'visible', 'value' => '1')), 'addsubcategories' => 0);

      'key' => The category column to search, expected keys (value format) are:
      "id" (int) the category id,
      "name" (string) the category name,
      "parent" (int) the parent category id,
      "idnumber" (string) category idnumber,
      "visible" (int) whether the category is visible or not,
      "theme" (string) category theme'),

      core_course_create_categories

      /// PARAMETERS
      $params = array('categories' => array(array('name' => 'newcategory with parent id = 1', 'parent' => '1',
      'idnumber' => 'idnumber123', 'description' => 'description of the category')));

      core_course_update_categories

      /// PARAMETERS

      $params = array('categories' => array(array('id'=>'3','name' => 'category id = 3 move under parent id = 2', 'parent' => '2',
      'idnumber' => 'idnumber1234', 'description' => 'description of the category updated')));

      core_course_delete_categories

      /// PARAMETERS

      $params = array('categories' => array(array('id'=>'3','newparent' => 1)));

      Show
      General setup a) Create on second level category hidden with a not hidden sub category. The web service function should not return the hidden category, neither its sub category. Try again with a user having the capability 'moodle/category:viewhiddencategories', the function should return both categories. b) Create a third level category with a sub category. Set the max category level of your Moodle to 3. The web service function should return the third level category but not its sub category. c) call the web service function as a user having 'moodle/category:manage' capability, the web service function should return all categories (hidden categories and deep level categories). Take the REST demo client, then run each function tests: https://github.com/moodlehq/sample-ws-clients/blob/master/PHP-REST/client.php core_course_get_categories /// PARAMETERS $params = array('criteria' => array(array('key' => 'visible', 'value' => '1')), 'addsubcategories' => 0); 'key' => The category column to search, expected keys (value format) are: "id" (int) the category id, "name" (string) the category name, "parent" (int) the parent category id, "idnumber" (string) category idnumber, "visible" (int) whether the category is visible or not, "theme" (string) category theme'), core_course_create_categories /// PARAMETERS $params = array('categories' => array(array('name' => 'newcategory with parent id = 1', 'parent' => '1', 'idnumber' => 'idnumber123', 'description' => 'description of the category'))); core_course_update_categories /// PARAMETERS $params = array('categories' => array(array('id'=>'3','name' => 'category id = 3 move under parent id = 2', 'parent' => '2', 'idnumber' => 'idnumber1234', 'description' => 'description of the category updated'))); core_course_delete_categories /// PARAMETERS $params = array('categories' => array(array('id'=>'3','newparent' => 1)));
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:

      Description

      merge all commits from MDL-30081,MDL-30082,MDL-30083,MDL-30084

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            jerome Jérôme Mouneyrac added a comment -

            As suggested by Eloy, I merge all commits in one branch. I also reworded the commit messages to this new MDL number.

            Show
            jerome Jérôme Mouneyrac added a comment - As suggested by Eloy, I merge all commits in one branch. I also reworded the commit messages to this new MDL number.
            Hide
            jerome Jérôme Mouneyrac added a comment - - edited

            I tested with REST-XML/JSON and XMLRPC.

            Note that the $params in XMLRPC demo client is different, for example for core_course_update_categories the demo client $params would be:

            //notice that we dont have an extra array with the categories name.
            $params = array(array('id'=>'29','name' => 'category id = 29 move under parent id = 1', 'parent' => '1',
            'idnumber' => 'idnumber1234', 'description' => 'description of the category updated'));

            Show
            jerome Jérôme Mouneyrac added a comment - - edited I tested with REST-XML/JSON and XMLRPC. Note that the $params in XMLRPC demo client is different, for example for core_course_update_categories the demo client $params would be: //notice that we dont have an extra array with the categories name. $params = array(array('id'=>'29','name' => 'category id = 29 move under parent id = 1', 'parent' => '1', 'idnumber' => 'idnumber1234', 'description' => 'description of the category updated'));
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Thanks Jerome, I'll be on this as soon as I roll current weeklies!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Thanks Jerome, I'll be on this as soon as I roll current weeklies!
            Hide
            fabiomsouto Fábio Souto added a comment - - edited

            Hello Jerome and Eloy,

            I have only read the progresses now. Are you taking care of all the related issues or can I help with something?
            I didn't know that the code freeze was going to be next week, I thought it would be at 7th June...
            I would really need for get_users function as well. Is there still any chance of it making to 2.3?

            Thank you,
            Fabio

            Show
            fabiomsouto Fábio Souto added a comment - - edited Hello Jerome and Eloy, I have only read the progresses now. Are you taking care of all the related issues or can I help with something? I didn't know that the code freeze was going to be next week, I thought it would be at 7th June... I would really need for get_users function as well. Is there still any chance of it making to 2.3? Thank you, Fabio
            Hide
            jerome Jérôme Mouneyrac added a comment - - edited

            I've tested the update function with the XMLRPC web client integrated to Moodle it works too. Not reason other protocol don't work. Submitting for integration.

            Fabio for get_users I'll have a look today but as get_users didn't even go for integration review, I suppose there are few chances that it makes it . Edit: ah in fact I just saw your message you're still working on get_users, so forget the mention about I'll look today .

            Show
            jerome Jérôme Mouneyrac added a comment - - edited I've tested the update function with the XMLRPC web client integrated to Moodle it works too. Not reason other protocol don't work. Submitting for integration. Fabio for get_users I'll have a look today but as get_users didn't even go for integration review, I suppose there are few chances that it makes it . Edit: ah in fact I just saw your message you're still working on get_users, so forget the mention about I'll look today .
            Hide
            fabiomsouto Fábio Souto added a comment -

            Hello Jerome,

            Sorry for polutting this with another issue (again). After the code freeze is there any chance to integrate webservices? Or would it only be doable on another major release (ie. six months from now)? Just so that I can plan what to do next.

            Thanks for all,
            Fabio

            Show
            fabiomsouto Fábio Souto added a comment - Hello Jerome, Sorry for polutting this with another issue (again). After the code freeze is there any chance to integrate webservices? Or would it only be doable on another major release (ie. six months from now)? Just so that I can plan what to do next. Thanks for all, Fabio
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Fabio: after code freeze, it will be about how convincing we can be with the integrators For your thesis, it is really easy to add your web service from a local plugin if get_users is not in 2.3.0: https://github.com/moodlehq/moodle-local_wstemplate. Try to write a plugin now so you can have an idea. Apart from the services.php and naming convention, creating a web service plugin for get_users will be a copy/paste of the externallib.php code.

            Show
            jerome Jérôme Mouneyrac added a comment - Fabio: after code freeze, it will be about how convincing we can be with the integrators For your thesis, it is really easy to add your web service from a local plugin if get_users is not in 2.3.0: https://github.com/moodlehq/moodle-local_wstemplate . Try to write a plugin now so you can have an idea. Apart from the services.php and naming convention, creating a web service plugin for get_users will be a copy/paste of the externallib.php code.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Eloy: I just noticed there could be some AMOS issue. I never used AMOS yet, but let me know if it is needed, I'll have a look.

            Show
            jerome Jérôme Mouneyrac added a comment - Eloy: I just noticed there could be some AMOS issue. I never used AMOS yet, but let me know if it is needed, I'll have a look.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            About AMOS... if the strings are brand-new... then you don't need to include anything. Only if you are copying/moving those strings from another existing file & key the AMOS scripts are needed, so all langs get that copy/move automatically done.

            BTW, finally you went the "translated messages" way... does that mean that we are already returning "errorcode/warningcode" (untranslated) as we chatted recently?

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - About AMOS... if the strings are brand-new... then you don't need to include anything. Only if you are copying/moving those strings from another existing file & key the AMOS scripts are needed, so all langs get that copy/move automatically done. BTW, finally you went the "translated messages" way... does that mean that we are already returning "errorcode/warningcode" (untranslated) as we chatted recently?
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Aha, I've set this as blocked by MDL-32949, so we cannot be using the translated strings until we introduce the errorcode/warningcode in all the exceptions/warnings.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Aha, I've set this as blocked by MDL-32949 , so we cannot be using the translated strings until we introduce the errorcode/warningcode in all the exceptions/warnings. Ciao
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            With MDL-32949 and MDL-32998 already integrated... back to this.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - With MDL-32949 and MDL-32998 already integrated... back to this.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Some tiny things:

            1) Now that we have sorted out errorcode, message and debug... which is the point of this:

            // $criteriaerrormsg is a code error, do not fix typo, do not edit it.
            $criteriaerrormsg = 'Missing permissions to search on a criteria.';

            IMO it should be one "criterianotallowed" code, with or without translating, as you prefer, with debug info giving more (untranslated) details (that part is ok). But the complete (english) sentence has no sense there anymore (as code).

            2) Some functions like compare_categories_by_path() are defined as private in the phpdocs but the declaration says public. IMO they should be "private static".

            3) The core_course_create_categories_form is missing support for some criteria (visible, theme).

            4) Looking for uses of exceptions, I ended with this grep:

            grep -Pr "new.*exception[^\"']*get_string" *

            There are a lot of bad uses of exceptions with get_string() calls within them. We need to take rid of that ASAP, both in "normal" exceptions (first param is the errorcode no point about one get_string() call there) and also webservice_access_exception (first param is debuginfo and it cannot be translated). I'd recommend to create one issue about his point (in case it does not exist yet).

            And that's all I've found... please add one new commit fixing 1, 2 & 3, create one new issue for 4 and this will, finally land, yay!

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Some tiny things: 1) Now that we have sorted out errorcode, message and debug... which is the point of this: // $criteriaerrormsg is a code error, do not fix typo, do not edit it. $criteriaerrormsg = 'Missing permissions to search on a criteria.'; IMO it should be one "criterianotallowed" code, with or without translating, as you prefer, with debug info giving more (untranslated) details (that part is ok). But the complete (english) sentence has no sense there anymore (as code). 2) Some functions like compare_categories_by_path() are defined as private in the phpdocs but the declaration says public. IMO they should be "private static". 3) The core_course_create_categories_form is missing support for some criteria (visible, theme). 4) Looking for uses of exceptions, I ended with this grep: grep -Pr "new.*exception[^\"']*get_string" * There are a lot of bad uses of exceptions with get_string() calls within them. We need to take rid of that ASAP, both in "normal" exceptions (first param is the errorcode no point about one get_string() call there) and also webservice_access_exception (first param is debuginfo and it cannot be translated). I'd recommend to create one issue about his point (in case it does not exist yet). And that's all I've found... please add one new commit fixing 1, 2 & 3, create one new issue for 4 and this will, finally land, yay! Ciao
            Hide
            jerome Jérôme Mouneyrac added a comment - - edited

            1) and 2) are done. I also rebased.
            3) is not done because I don't see the point. PHPunit should be the one doing complete unit test. These web clients are just useful for the admin to test quickly if his/her setup works : MDL-33059
            4) MDL-33060

            Show
            jerome Jérôme Mouneyrac added a comment - - edited 1) and 2) are done. I also rebased. 3) is not done because I don't see the point. PHPunit should be the one doing complete unit test. These web clients are just useful for the admin to test quickly if his/her setup works : MDL-33059 4) MDL-33060
            Hide
            jerome Jérôme Mouneyrac added a comment -

            I guess you can integrate

            Show
            jerome Jérôme Mouneyrac added a comment - I guess you can integrate
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated, yay!

            Note I've added 2 extra commits:

            1) Reorganize a bit both services.php and course/externallib.php to have similar functions together.
            2) Add missing transactions support to delete_categories.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated, yay! Note I've added 2 extra commits: 1) Reorganize a bit both services.php and course/externallib.php to have similar functions together. 2) Add missing transactions support to delete_categories.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Passing, I've tested the 4 functions with single and multiple categories, forcing errors and different criteria.

            All them seemed to work ok (but the missing transaction support in delete_categories) that already have been fixed.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Passing, I've tested the 4 functions with single and multiple categories, forcing errors and different criteria. All them seemed to work ok (but the missing transaction support in delete_categories) that already have been fixed.
            Hide
            fabiomsouto Fábio Souto added a comment -

            Yay! I feel like a grown programmer now!
            Thank you and thanks to Jerome as well, you're all great.

            Cheers
            Fabio

            Show
            fabiomsouto Fábio Souto added a comment - Yay! I feel like a grown programmer now! Thank you and thanks to Jerome as well, you're all great. Cheers Fabio
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            U P S T R E A M I Z E D !

            Many thanks for the hard work, closing this as fixed.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - U P S T R E A M I Z E D ! Many thanks for the hard work, closing this as fixed. Ciao
            Hide
            lewis.carroll David added a comment -

            Hi!

            I seen the condition:

            // Check name.
            if (textlib::strlen($category['name'])>30) {
            throw new moodle_exception('categorytoolong');
            }

            in create and update category.

            Is totally necessari? I think that 30 is a small value for this field.

            Thanks!

            Show
            lewis.carroll David added a comment - Hi! I seen the condition: // Check name. if (textlib::strlen($category ['name'] )>30) { throw new moodle_exception('categorytoolong'); } in create and update category. Is totally necessari? I think that 30 is a small value for this field. Thanks!
            Hide
            jerome Jérôme Mouneyrac added a comment - - edited

            I just had a look to editcategory_form and it is set to 30. So it's matching the UI, it seems correct to me.

            Show
            jerome Jérôme Mouneyrac added a comment - - edited I just had a look to editcategory_form and it is set to 30. So it's matching the UI, it seems correct to me.
            Hide
            lewis.carroll David added a comment -

            Yes, but the 30 is the visual size of the input box, isn't a condition for the value:

            $mform->addElement('text', 'name', get_string('categoryname'), array('size'=>'30'));

            I can write a category name with 50 characters (for example) without problems in the form.

            Show
            lewis.carroll David added a comment - Yes, but the 30 is the visual size of the input box, isn't a condition for the value: $mform->addElement('text', 'name', get_string('categoryname'), array('size'=>'30')); I can write a category name with 50 characters (for example) without problems in the form.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            ah yes I've been looking to it too quickly, it was 12:30AM Please can you write an issue and assign it to me? I'll fix that. Thanks.

            Show
            jerome Jérôme Mouneyrac added a comment - ah yes I've been looking to it too quickly, it was 12:30AM Please can you write an issue and assign it to me? I'll fix that. Thanks.
            Hide
            lewis.carroll David added a comment -

            Don't worry! Perfect , I write the issue.

            Show
            lewis.carroll David added a comment - Don't worry! Perfect , I write the issue.
            Hide
            lewis.carroll David added a comment -
            Show
            lewis.carroll David added a comment - The URL is: http://tracker.moodle.org/browse/MDL-33966 Thanks!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  25/Jun/12