Details

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

      Description

      delete one or more courses according to criteria

        Issue Links

          Activity

          Hide
          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
          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
          David Castro added a comment -

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

          Show
          David Castro added a comment - This funcionality is completed in CVS. We have commit a simpletest.
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          Juan Leyva added a comment -

          Jerome,

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

          Regards

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

          it's good to me, submitting to integration.

          Show
          Jérôme Mouneyrac added a comment - it's good to me, submitting to integration.
          Hide
          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
          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
          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
          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
          Jérôme Mouneyrac added a comment -

          I resubmitted, thanks Juan.

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

          Created MDL-32696

          Show
          Dan Poltawski added a comment - Created MDL-32696
          Hide
          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
          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
          Dan Poltawski added a comment -

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

          Show
          Dan Poltawski added a comment - The fix to the URL bug has been integrated, please restart testing
          Hide
          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
          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
          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
          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: