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

          Attachments

            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