Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.3
    • Component/s: Course, Web Services
    • Labels:

      Description

      delete one or more courses according to criteria

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            pigui Jordi Piguillem Poch added a comment -

            We have to work in this function definition. What about associated ressources, roles ... Must be destroyed?

            /**

            • Deletes a course from the course table
            • @private ?
            • @param string $field
            • Possible values:
            • <ul>
            • <li>id</li>
            • <li>name</li>
            • <li>idnumber</li>
            • <li>shortname</li>
            • </ul>
            • @param string $value
            • @return boolean
              */
              function delete_course($value, $field){
              }

            /**

            • Deletes some courses from the course table
            • @param string $field
            • Possible values:
            • <ul>
            • <li>id</li>
            • <li>name</li>
            • <li>idnumber</li>
            • <li>shortname</li>
            • </ul>
            • @param array $values
            • @uses delete_course
            • @return boolean
              */
              function delete_courses($field, $values){
              }
            Show
            pigui Jordi Piguillem Poch added a comment - We have to work in this function definition. What about associated ressources, roles ... Must be destroyed? /** Deletes a course from the course table @private ? @param string $field Possible values: <ul> <li>id</li> <li>name</li> <li>idnumber</li> <li>shortname</li> </ul> @param string $value @return boolean */ function delete_course($value, $field){ } /** Deletes some courses from the course table @param string $field Possible values: <ul> <li>id</li> <li>name</li> <li>idnumber</li> <li>shortname</li> </ul> @param array $values @uses delete_course @return boolean */ function delete_courses($field, $values){ }
            Hide
            dcastro David Castro added a comment -

            This funcionality is completed in CVS.
            We have commit a simpletest.

            Show
            dcastro David Castro added a comment - This funcionality is completed in CVS. We have commit a simpletest.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Will not be resolve for 2.0 as it's a pretty sensitive area, moving fix version to 2.0.1

            Show
            jerome Jérôme Mouneyrac added a comment - Will not be resolve for 2.0 as it's a pretty sensitive area, moving fix version to 2.0.1
            Hide
            jleyva Juan Leyva added a comment -

            We have funding for implementing this function:

            This function will perform a full delete of the specified courses

            Parameters:
            ----------

            array of course ids

            Return value:
            -----------
            Array of course ids deleted

            array [id]

            Jerome, are we in time for getting this in 2.3? I will have the code for review the 20th of this month

            Show
            jleyva Juan Leyva added a comment - We have funding for implementing this function: This function will perform a full delete of the specified courses Parameters: ---------- array of course ids Return value: ----------- Array of course ids deleted array [id] Jerome, are we in time for getting this in 2.3? I will have the code for review the 20th of this month
            Hide
            jleyva Juan Leyva added a comment -

            Adding the main function code for a peer review:

            https://gist.github.com/2399550

            Sorry for not adding the link to the full branch, but I'm developing this as a local plugin so once reviewed I will create the pull Moodle DEV branch (this is mainly for avoid rebasing)

            Show
            jleyva Juan Leyva added a comment - Adding the main function code for a peer review: https://gist.github.com/2399550 Sorry for not adding the link to the full branch, but I'm developing this as a local plugin so once reviewed I will create the pull Moodle DEV branch (this is mainly for avoid rebasing)
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Thanks Juan. Can you also add in the test instruction: a link to the demo client (https://github.com/moodlehq/sample-ws-clients/tree/master/PHP-REST) with a parameters section example:

            /// PARAMETERS
            $params = array('2','3') //pick some course ids you want to delete ;)

            I'll test it before submitting otherwise the code seems all good to me.

            Show
            jerome Jérôme Mouneyrac added a comment - Thanks Juan. Can you also add in the test instruction: a link to the demo client ( https://github.com/moodlehq/sample-ws-clients/tree/master/PHP-REST ) with a parameters section example: /// PARAMETERS $params = array('2','3') //pick some course ids you want to delete ;) I'll test it before submitting otherwise the code seems all good to me.
            Hide
            jleyva Juan Leyva added a comment -

            Jerome,

            I've added the pull URLs and testing instructions for your tests

            Regards

            Show
            jleyva Juan Leyva added a comment - Jerome, I've added the pull URLs and testing instructions for your tests Regards
            Hide
            jerome Jérôme Mouneyrac added a comment -

            it's good to me, submitting to integration.

            Show
            jerome Jérôme Mouneyrac added a comment - it's good to me, submitting to integration.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            yay, 99% ok:

            • get_context_instance() is deprecated since 2.2. context_course::instance() is the new way.
            • moodle_exception(get_string(...)) seems incorrect. moodle_exception() itself performs the get_string() for you (yes, I've seen the file is plagued of them, grrr).

            Also, I don't know if I returning nulls on success is the best of the ideas, but I've seen the same approach used in other delete operations... so, np.

            So, reopening... thanks and ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - yay, 99% ok: get_context_instance() is deprecated since 2.2. context_course::instance() is the new way. moodle_exception(get_string(...)) seems incorrect. moodle_exception() itself performs the get_string() for you (yes, I've seen the file is plagued of them, grrr). Also, I don't know if I returning nulls on success is the best of the ideas, but I've seen the same approach used in other delete operations... so, np. So, reopening... thanks and ciao
            Hide
            jleyva Juan Leyva added a comment -

            Errors fixed:

            Here is the diff with the previous version for a quick review of the changes made:

            https://github.com/jleyva/moodle/commit/4222ed6b259f689abe6423000e5260fb8174a94d#course/externallib.php

            Show
            jleyva Juan Leyva added a comment - Errors fixed: Here is the diff with the previous version for a quick review of the changes made: https://github.com/jleyva/moodle/commit/4222ed6b259f689abe6423000e5260fb8174a94d#course/externallib.php
            Hide
            jerome Jérôme Mouneyrac added a comment -

            I resubmitted, thanks Juan.

            Show
            jerome Jérôme Mouneyrac added a comment - I resubmitted, thanks Juan.
            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
            poltawski Dan Poltawski added a comment -

            Thanks Juan/Jerome i've integrated this.

            I squashed the two commits together because the second one didn't have the bug url in and I think it makes sense as an atomic commit.

            Note I noticed that trying to delete the site course silently fails. I think that the solution here is to make delete_course throw an exception in this case since this is an exceptional circumstance.

            Show
            poltawski Dan Poltawski added a comment - Thanks Juan/Jerome i've integrated this. I squashed the two commits together because the second one didn't have the bug url in and I think it makes sense as an atomic commit. Note I noticed that trying to delete the site course silently fails. I think that the solution here is to make delete_course throw an exception in this case since this is an exceptional circumstance.
            Hide
            poltawski Dan Poltawski added a comment -

            Created MDL-32696

            Show
            poltawski Dan Poltawski added a comment - Created MDL-32696
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Attempting to add function to external service produce "Not Found" error. This is due to a typo for $PAGE->set_url().

            Raj has created an issue with patch to fix this. MDL-32732

            Test failed.

            Show
            rwijaya Rossiani Wijaya added a comment - Attempting to add function to external service produce "Not Found" error. This is due to a typo for $PAGE->set_url(). Raj has created an issue with patch to fix this. MDL-32732 Test failed.
            Hide
            poltawski Dan Poltawski added a comment -

            The fix to the URL bug has been integrated, please restart testing

            Show
            poltawski Dan Poltawski added a comment - The fix to the URL bug has been integrated, please restart testing
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Agree with Dan to create an exception when attempting to delete site course.

            Other than that, this looks great.

            Test passed.

            Show
            rwijaya Rossiani Wijaya added a comment - Agree with Dan to create an exception when attempting to delete site course. Other than that, this looks great. Test passed.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            UPDATE tracker_issues
               SET status = 'Closed',
                  comment = 'Thanks!'
            WHEN participants = 'Did a gorgeous work'

            This landed upstream some hours ago (some - me - developer fell slept in the sofa yesterday before spamming this).

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - UPDATE tracker_issues SET status = 'Closed', comment = 'Thanks!' WHEN participants = 'Did a gorgeous work' This landed upstream some hours ago (some - me - developer fell slept in the sofa yesterday before spamming this).

              People

              • Votes:
                4 Vote for this issue
                Watchers:
                10 Start watching this issue

                Dates

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