Details

      Gliffy Diagrams

        Issue Links

          Activity

          Hide
          nukumaar Nuku Maar added a comment -

          Hello, i dont have git, but i tried a quick fix for update_courses:

          public static function update_courses_parameters() {
                  global $CFG;
                  return new external_function_parameters(
                      array(
                          'courses' => new external_multiple_structure(
                              new external_single_structure(
                                  array(
                                      'id'    => new external_value(PARAM_INT, 'ID of the course'),
                                      'fullname'    => new external_value(PARAM_RAW, 'Full name of the course', VALUE_OPTIONAL), 
                                      'shortname'    => new external_value(PARAM_RAW, 'Short name of the course', VALUE_OPTIONAL),    
                                      'categoryid'    => new external_value(PARAM_RAW, 'Category of the course', VALUE_OPTIONAL),   
                                      'idnumber'    => new external_value(PARAM_RAW, 'Idnumber of the course', VALUE_OPTIONAL),   
                                      'summary'    => new external_value(PARAM_RAW, 'Summary of the course', VALUE_OPTIONAL),   
                                      'summaryformat'    => new external_value(PARAM_RAW, 'Summaryformat name of the course', VALUE_OPTIONAL),    
                                      'format'    => new external_value(PARAM_RAW, 'Format of the course', VALUE_OPTIONAL),      
                                      'showgrades'    => new external_value(PARAM_RAW, 'Showgrades of the course', VALUE_OPTIONAL),            
                                      'newsitems'    => new external_value(PARAM_RAW, 'newsitems of the course', VALUE_OPTIONAL), 
                                      'startdate'    => new external_value(PARAM_RAW, 'startdate of the course', VALUE_OPTIONAL),    
                                      'numsections'    => new external_value(PARAM_RAW, 'numsections of the course', VALUE_OPTIONAL),   
                                      'maxbytes'    => new external_value(PARAM_RAW, 'maxbytes of the course', VALUE_OPTIONAL),   
                                      'showreports'    => new external_value(PARAM_RAW, 'showreports of the course', VALUE_OPTIONAL),   
                                      'visible'    => new external_value(PARAM_RAW, 'visible name of the course', VALUE_OPTIONAL),    
                                      'hiddensections'    => new external_value(PARAM_RAW, 'hiddensections of the course', VALUE_OPTIONAL),      
                                      'groupmode'    => new external_value(PARAM_RAW, 'groupmode of the course', VALUE_OPTIONAL),   
                                      'groupmodeforce'    => new external_value(PARAM_RAW, 'groupmodeforce of the course', VALUE_OPTIONAL), 
                                      'defaultgroupingid'    => new external_value(PARAM_RAW, 'defaultgroupingid of the course', VALUE_OPTIONAL),    
                                      'enablecompletion'    => new external_value(PARAM_RAW, 'enablecompletion of the course', VALUE_OPTIONAL),   
                                      'completionstartonenrol'    => new external_value(PARAM_RAW, 'completionstartonenrol of the course', VALUE_OPTIONAL),   
                                      'completionnotify'    => new external_value(PARAM_RAW, 'completionnotify of the course', VALUE_OPTIONAL),   
                                      'lang'    => new external_value(PARAM_RAW, 'lang of the course', VALUE_OPTIONAL),    
                                      'forcetheme'    => new external_value(PARAM_RAW, 'forcetheme of the course', VALUE_OPTIONAL)   
                                  )
                              )
                          )
                      )
                  );
              }
           
              public static function update_courses($courses) {
                  global $CFG, $DB;
                  require_once($CFG->dirroot."/course/lib.php");
           
                  $context = get_context_instance(CONTEXT_SYSTEM);
                  self::validate_context($context);
           
                  $params = self::validate_parameters(self::update_courses_parameters(), array('courses'=>$courses));
           
                  $transaction = $DB->start_delegated_transaction();
           
                  foreach ($params['courses'] as $course) {
           
                  if (!is_object($course)) {
                          $course = (object)$course;
                  }
           
                  $course->timemodified = time();
                  $DB->update_record('course', $course);
           
                  }
           
                  $transaction->allow_commit();
           
                  return null;
              }
           
              public static function update_courses_returns() {
                  return null;
              }

          yeah, it returns null...

          Show
          nukumaar Nuku Maar added a comment - Hello, i dont have git, but i tried a quick fix for update_courses: public static function update_courses_parameters() { global $CFG; return new external_function_parameters( array( 'courses' => new external_multiple_structure( new external_single_structure( array( 'id' => new external_value(PARAM_INT, 'ID of the course'), 'fullname' => new external_value(PARAM_RAW, 'Full name of the course', VALUE_OPTIONAL), 'shortname' => new external_value(PARAM_RAW, 'Short name of the course', VALUE_OPTIONAL), 'categoryid' => new external_value(PARAM_RAW, 'Category of the course', VALUE_OPTIONAL), 'idnumber' => new external_value(PARAM_RAW, 'Idnumber of the course', VALUE_OPTIONAL), 'summary' => new external_value(PARAM_RAW, 'Summary of the course', VALUE_OPTIONAL), 'summaryformat' => new external_value(PARAM_RAW, 'Summaryformat name of the course', VALUE_OPTIONAL), 'format' => new external_value(PARAM_RAW, 'Format of the course', VALUE_OPTIONAL), 'showgrades' => new external_value(PARAM_RAW, 'Showgrades of the course', VALUE_OPTIONAL), 'newsitems' => new external_value(PARAM_RAW, 'newsitems of the course', VALUE_OPTIONAL), 'startdate' => new external_value(PARAM_RAW, 'startdate of the course', VALUE_OPTIONAL), 'numsections' => new external_value(PARAM_RAW, 'numsections of the course', VALUE_OPTIONAL), 'maxbytes' => new external_value(PARAM_RAW, 'maxbytes of the course', VALUE_OPTIONAL), 'showreports' => new external_value(PARAM_RAW, 'showreports of the course', VALUE_OPTIONAL), 'visible' => new external_value(PARAM_RAW, 'visible name of the course', VALUE_OPTIONAL), 'hiddensections' => new external_value(PARAM_RAW, 'hiddensections of the course', VALUE_OPTIONAL), 'groupmode' => new external_value(PARAM_RAW, 'groupmode of the course', VALUE_OPTIONAL), 'groupmodeforce' => new external_value(PARAM_RAW, 'groupmodeforce of the course', VALUE_OPTIONAL), 'defaultgroupingid' => new external_value(PARAM_RAW, 'defaultgroupingid of the course', VALUE_OPTIONAL), 'enablecompletion' => new external_value(PARAM_RAW, 'enablecompletion of the course', VALUE_OPTIONAL), 'completionstartonenrol' => new external_value(PARAM_RAW, 'completionstartonenrol of the course', VALUE_OPTIONAL), 'completionnotify' => new external_value(PARAM_RAW, 'completionnotify of the course', VALUE_OPTIONAL), 'lang' => new external_value(PARAM_RAW, 'lang of the course', VALUE_OPTIONAL), 'forcetheme' => new external_value(PARAM_RAW, 'forcetheme of the course', VALUE_OPTIONAL) ) ) ) ) ); }   public static function update_courses($courses) { global $CFG, $DB; require_once($CFG->dirroot."/course/lib.php");   $context = get_context_instance(CONTEXT_SYSTEM); self::validate_context($context);   $params = self::validate_parameters(self::update_courses_parameters(), array('courses'=>$courses));   $transaction = $DB->start_delegated_transaction();   foreach ($params['courses'] as $course) {   if (!is_object($course)) { $course = (object)$course; }   $course->timemodified = time(); $DB->update_record('course', $course);   }   $transaction->allow_commit();   return null; }   public static function update_courses_returns() { return null; } yeah, it returns null...
          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          Check for change category need to be looked again after MDL-31750 gets integrated.

          Show
          rajeshtaneja Rajesh Taneja added a comment - Check for change category need to be looked again after MDL-31750 gets integrated.
          Hide
          rajeshtaneja Rajesh Taneja added a comment - - edited

          Capability check is done in course/edit.php and course/edit_form.php.

          Show
          rajeshtaneja Rajesh Taneja added a comment - - edited Capability check is done in course/edit.php and course/edit_form.php.
          Hide
          jerome Jérôme Mouneyrac added a comment -

          Thanks Raj. Very nice to read.

          • I think categoryid and fullname should be optional. I don't see why we should force people to update it.
          • For shortname it should be optional too.
          • For id I'm not sure. Shortname is supposed to be a unique field, so I would guess you could mark shortname and id as optional and throw an error if none of them is sent. however you need to confirm that shortname is always unique.
          • // Check if user can change visibility. => wrong phpdoc should be category
          Show
          jerome Jérôme Mouneyrac added a comment - Thanks Raj. Very nice to read. I think categoryid and fullname should be optional. I don't see why we should force people to update it. For shortname it should be optional too. For id I'm not sure. Shortname is supposed to be a unique field, so I would guess you could mark shortname and id as optional and throw an error if none of them is sent. however you need to confirm that shortname is always unique. // Check if user can change visibility. => wrong phpdoc should be category
          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          Thanks for pointing that Jerome,

          I have made fullname, shortname and idnumber as optional. Also, updated comment.

          I don't think we should rely on any string (shortname) match at this point. As per current behaviour we send course id for doing any operation in course and IMHO we should stick to it.

          Please let me know if you are fine with it, so I can push this for integration.

          Show
          rajeshtaneja Rajesh Taneja added a comment - Thanks for pointing that Jerome, I have made fullname, shortname and idnumber as optional. Also, updated comment. I don't think we should rely on any string (shortname) match at this point. As per current behaviour we send course id for doing any operation in course and IMHO we should stick to it. Please let me know if you are fine with it, so I can push this for integration.
          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          As discussed in scrum, id should be mandatory and shortname optional. I am pushing this for integration.

          Show
          rajeshtaneja Rajesh Taneja added a comment - As discussed in scrum, id should be mandatory and shortname optional. I am pushing this for integration.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          uhm... how can we integrate this when it's blocked by MDL-31750?

          We use to hold blocked ones till all the dependencies are met.

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - uhm... how can we integrate this when it's blocked by MDL-31750 ? We use to hold blocked ones till all the dependencies are met.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          FYI: It's Wednesday and I'm moving all MY "integration in progress" issues (3) out from current integration with message:

          The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

          This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

          This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

          Apologies for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - FYI: It's Wednesday and I'm moving all MY "integration in progress" issues (3) out from current integration with message: The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologies for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
          Hide
          stronk7 Eloy Lafuente (stronk7) 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
          stronk7 Eloy Lafuente (stronk7) 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
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Sorry, reopening, as commented last week, this continues being blocked by another issue. Please clarify it.

          https://tracker.moodle.org/browse/MDL-30062?focusedCommentId=199026&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-199026

          Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Sorry, reopening, as commented last week, this continues being blocked by another issue. Please clarify it. https://tracker.moodle.org/browse/MDL-30062?focusedCommentId=199026&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-199026 Ciao
          Hide
          cibot CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          cibot CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          Thanks Eloy,

          It would be nice to get this right in first place, depending on decision made in MDL-37615.
          But seems MDL-37615, has been lingering for sometime now.

          IMHO, this is good for integration in master and MDL-37615 can update webservice logic if plan to go some different way.

          Will leave comment in MDL-37615 once this gets integrated.

          FYI:
          Changing linked issue from blocked to related to avoid confusion.

          Show
          rajeshtaneja Rajesh Taneja added a comment - Thanks Eloy, It would be nice to get this right in first place, depending on decision made in MDL-37615 . But seems MDL-37615 , has been lingering for sometime now. IMHO, this is good for integration in master and MDL-37615 can update webservice logic if plan to go some different way. Will leave comment in MDL-37615 once this gets integrated. FYI: Changing linked issue from blocked to related to avoid confusion.
          Hide
          stronk7 Eloy Lafuente (stronk7) 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
          stronk7 Eloy Lafuente (stronk7) 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
          damyon Damyon Wiese added a comment -

          Hi Raj,

          I checked a whole heap of webservice related things and you have done most of them (yay!).

          A couple of things do need addressing here:

          The external function has not been added to "lib/db/services.php" so it won't be callable by anyone.

          The test instructions need updating too - they are referring to a webservice client that doesn't exist.

          Also - (minor):

          While you are looking at the other things - there are a couple of comments missing full stops - it would be good if you could run code checker and make sure there are no "new" errors/warnings introduced.

          Thanks, Damyon

          Show
          damyon Damyon Wiese added a comment - Hi Raj, I checked a whole heap of webservice related things and you have done most of them (yay!). A couple of things do need addressing here: The external function has not been added to "lib/db/services.php" so it won't be callable by anyone. The test instructions need updating too - they are referring to a webservice client that doesn't exist. Also - (minor): While you are looking at the other things - there are a couple of comments missing full stops - it would be good if you could run code checker and make sure there are no "new" errors/warnings introduced. Thanks, Damyon
          Hide
          cibot CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          Thanks for pointing that Damyon,

          Seems I forgot to add services.php file. Have added it now and attached client.php

          Also, fixed . (dots) in comments.

          Sending this back for your review.

          Show
          rajeshtaneja Rajesh Taneja added a comment - Thanks for pointing that Damyon, Seems I forgot to add services.php file. Have added it now and attached client.php Also, fixed . (dots) in comments. Sending this back for your review.
          Hide
          damyon Damyon Wiese added a comment -

          Hi Raj,

          I just checked this with Jerome - it is OK to catch exceptions and translate them into external warnings like this (but I don't really like it). But if you are doing that then you should not use a transaction.

          Consider when an exception is thrown half way through update_course - the client will get an error but the half done update would be committed.

          Can you remove the transaction code from update_courses ?

          Thanks!

          Show
          damyon Damyon Wiese added a comment - Hi Raj, I just checked this with Jerome - it is OK to catch exceptions and translate them into external warnings like this (but I don't really like it). But if you are doing that then you should not use a transaction. Consider when an exception is thrown half way through update_course - the client will get an error but the half done update would be committed. Can you remove the transaction code from update_courses ? Thanks!
          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          Thanks Damyon, it's removed.

          Show
          rajeshtaneja Rajesh Taneja added a comment - Thanks Damyon, it's removed.
          Hide
          damyon Damyon Wiese added a comment -

          Hurrah! This new webservice is joining it's friends in integration master.

          Thanks for all the changes Raj!

          Show
          damyon Damyon Wiese added a comment - Hurrah! This new webservice is joining it's friends in integration master. Thanks for all the changes Raj!
          Hide
          jerome Jérôme Mouneyrac added a comment -

          Hurrah! That's an important one!

          Show
          jerome Jérôme Mouneyrac added a comment - Hurrah! That's an important one!
          Hide
          dmonllao David Monllaó added a comment -

          It passes, this is the returned message although:

          <?xml version="1.0" encoding="UTF-8" ?>
          <RESPONSE>
          <SINGLE>
          <KEY name="warnings"><MULTIPLE>
          </MULTIPLE>
          </KEY>
          </SINGLE>
          </RESPONSE>
          

          Show
          dmonllao David Monllaó added a comment - It passes, this is the returned message although: <?xml version="1.0" encoding="UTF-8" ?> <RESPONSE> <SINGLE> <KEY name="warnings"><MULTIPLE> </MULTIPLE> </KEY> </SINGLE> </RESPONSE>
          Hide
          jerome Jérôme Mouneyrac added a comment -

          Note: Don't forget to add your function to the WS API list: http://docs.moodle.org/dev/Web_services_Roadmap#Core_web_service_functions (this is an automatic message)

          Show
          jerome Jérôme Mouneyrac added a comment - Note: Don't forget to add your function to the WS API list: http://docs.moodle.org/dev/Web_services_Roadmap#Core_web_service_functions (this is an automatic message)
          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          Added this to list.

          Show
          rajeshtaneja Rajesh Taneja added a comment - Added this to list.
          Hide
          damyon Damyon Wiese added a comment -

          Congratulations! This issue has been resolved. Thanks for helping to make Moodle better for everyone!

          Regards, Damyon

          Show
          damyon Damyon Wiese added a comment - Congratulations! This issue has been resolved. Thanks for helping to make Moodle better for everyone! Regards, Damyon
          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          Removing dev docs as this has been documented in http://docs.moodle.org/dev/Web_services_Roadmap#Core_web_service_functions.

          Also, removing integration_held label as this has been integrated.

          Show
          rajeshtaneja Rajesh Taneja added a comment - Removing dev docs as this has been documented in http://docs.moodle.org/dev/Web_services_Roadmap#Core_web_service_functions . Also, removing integration_held label as this has been integrated.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                14/May/13