Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Critical 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:
    • Rank:
      40022

      Description

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

        Issue Links

          Activity

          Hide
          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
          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
          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
          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
          Eloy Lafuente (stronk7) added a comment -

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

          Show
          Eloy Lafuente (stronk7) added a comment - Thanks Jerome, I'll be on this as soon as I roll current weeklies!
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Eloy Lafuente (stronk7) added a comment -

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

          Show
          Eloy Lafuente (stronk7) added a comment - With MDL-32949 and MDL-32998 already integrated... back to this.
          Hide
          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
          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
          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
          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
          Jérôme Mouneyrac added a comment -

          I guess you can integrate

          Show
          Jérôme Mouneyrac added a comment - I guess you can integrate
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          David added a comment -

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

          Show
          David added a comment - Don't worry! Perfect , I write the issue.
          Hide
          David added a comment -
          Show
          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: