Details

    • Rank:
      24695

      Issue Links

        Activity

        Hide
        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
        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
        Rajesh Taneja added a comment -

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

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

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

        Show
        Rajesh Taneja added a comment - - edited Capability check is done in course/edit.php and course/edit_form.php.
        Hide
        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
        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
        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
        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
        Rajesh Taneja added a comment -

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

        Show
        Rajesh Taneja added a comment - As discussed in scrum, id should be mandatory and shortname optional. I am pushing this for integration.
        Hide
        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
        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
        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
        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
        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
        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
        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
        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 added a comment -

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

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

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

        Show
        CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
        Hide
        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
        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
        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
        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 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 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 added a comment -

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

        Show
        CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
        Hide
        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
        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 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 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
        Rajesh Taneja added a comment -

        Thanks Damyon, it's removed.

        Show
        Rajesh Taneja added a comment - Thanks Damyon, it's removed.
        Hide
        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 Wiese added a comment - Hurrah! This new webservice is joining it's friends in integration master. Thanks for all the changes Raj!
        Hide
        Jérôme Mouneyrac added a comment -

        Hurrah! That's an important one!

        Show
        Jérôme Mouneyrac added a comment - Hurrah! That's an important one!
        Hide
        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
        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
        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
        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
        Rajesh Taneja added a comment -

        Added this to list.

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

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

        Regards, Damyon

        Show
        Damyon Wiese added a comment - Congratulations! This issue has been resolved. Thanks for helping to make Moodle better for everyone! Regards, Damyon
        Hide
        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
        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: