Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Backup, Web Services
    • Labels:
    • Testing Instructions:
      Hide

      For testing this new webservice:

      Use this client:
      https://github.com/moodlehq/sample-ws-clients/blob/master/PHP-REST/client.php

      functionname
      core_course_duplicate_course

      /// PARAMETERS
      $params = array(
      'courseid' => 4, // Course to be duplicated idnumber // CHANGE IT FOR AN EXISTING COURSE ID
      'fullname' => 'Nuevo curso', // New course full name
      'shortname' => 'NuevoCurso'.rand(), // New course shortname
      'categoryid' => 1, // New course category id
      'visible' => 1, // Make the course visible after duplicating
      'options' => array(array('name'=>'blocks', 'value'=>1), array('name'=>'activities', 'value'=>1), array('name'=>'filters', 'value'=>1)) // Backup options
      );

      Show
      For testing this new webservice: Use this client: https://github.com/moodlehq/sample-ws-clients/blob/master/PHP-REST/client.php functionname core_course_duplicate_course /// PARAMETERS $params = array( 'courseid' => 4, // Course to be duplicated idnumber // CHANGE IT FOR AN EXISTING COURSE ID 'fullname' => 'Nuevo curso', // New course full name 'shortname' => 'NuevoCurso'.rand(), // New course shortname 'categoryid' => 1, // New course category id 'visible' => 1, // Make the course visible after duplicating 'options' => array(array('name'=>'blocks', 'value'=>1), array('name'=>'activities', 'value'=>1), array('name'=>'filters', 'value'=>1)) // Backup options );
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-32233-core_course_duplicate_course-usersdata
    • Rank:
      39013

      Description

      This web service function will duplicate a course creating a new one.
      It will perform a backup of an existing course and a restore to a new one

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

      course id - int The course id to duplicate
      fullname - string The new course (duplicated) fullname
      shortname - string The new course shortname
      category id - int The category id of the new course - Optional, default to Miscellaneous
      backup settings - struct of backup settings:
      'users' => 'Default to true',
      'role_assignments' => 'Default to true',
      'user_files' => 'Default to true',
      'activities' => 'Default to true',
      'blocks' => 'Default to true',
      'filters' => 'Default to true',
      'comments' => 'Default to true',
      'completion_information' => 'Default to true',
      'logs' => 'Default to true',
      'histories' => 'Default to true'

      Return value:
      -----------
      The new course created

      struct{
      course struct [id, fullname, shortname, timemodified]
      }

      Note:
      I don't know who is the core_course maintainer (moodle.com ?)
      This functionality is currently not present in Moodle, the most similar is duplicate activity at course level

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for suggesting that. Perhaps you could describe the application of such a function. How would you use it?

          Show
          Michael de Raadt added a comment - Thanks for suggesting that. Perhaps you could describe the application of such a function. How would you use it?
          Hide
          Juan Leyva added a comment -

          Hi Michael,

          I'm following this doc

          http://docs.moodle.org/dev/Web_services_Roadmap#How_to_contribute_a_web_service_function_to_core

          for contributing a Web Service to core. We have plans to develop this webservice so we want to validate it in order to see if is suitable for core.

          The next step is a comment from Jerome and the core course maintainer for feedback.

          The application is to provide a webservice for creating automatically courses in Moodle duplicating an existing course (that is a course template) this is something usual in universities or other institutions and we implemented it in Moodle 1.9 also several time ago

          Regards

          Show
          Juan Leyva added a comment - Hi Michael, I'm following this doc http://docs.moodle.org/dev/Web_services_Roadmap#How_to_contribute_a_web_service_function_to_core for contributing a Web Service to core. We have plans to develop this webservice so we want to validate it in order to see if is suitable for core. The next step is a comment from Jerome and the core course maintainer for feedback. The application is to provide a webservice for creating automatically courses in Moodle duplicating an existing course (that is a course template) this is something usual in universities or other institutions and we implemented it in Moodle 1.9 also several time ago Regards
          Hide
          Jérôme Mouneyrac added a comment -

          Nobody is core maintainer of course code, so the main expert logic review phase will be during integration. However I'll peer-review the issue before you send it to integration as there is no maintainer.

          I see no problem with this function specs. I don't think it's a problem to create a backup then delete it after restore.

          Eloy can you confirm that the function is legitimate? I'll move it in the roadmap as soon it's confirmed.

          Show
          Jérôme Mouneyrac added a comment - Nobody is core maintainer of course code, so the main expert logic review phase will be during integration. However I'll peer-review the issue before you send it to integration as there is no maintainer. I see no problem with this function specs. I don't think it's a problem to create a backup then delete it after restore. Eloy can you confirm that the function is legitimate? I'll move it in the roadmap as soon it's confirmed.
          Hide
          Jérôme Mouneyrac added a comment -

          Note, I though about the backup/restore that will be quite bad in performance term (so no bulk function if we use backup/restore system). However the code should be pretty safe in term of bugs. So if the use case of this function can wait for the all process to happen, I think this function can go in core. Don't forget to mention that the function execution can be very long. This mention should be put in the function text description in the concerned services.php declaration.

          Show
          Jérôme Mouneyrac added a comment - Note, I though about the backup/restore that will be quite bad in performance term (so no bulk function if we use backup/restore system). However the code should be pretty safe in term of bugs. So if the use case of this function can wait for the all process to happen, I think this function can go in core. Don't forget to mention that the function execution can be very long. This mention should be put in the function text description in the concerned services.php declaration.
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Juan,
          Eloy confirmed that using backup/restore without user data is the best way to achieve this feature. He also added:

          TARGET_EXPORT/IMPORT in this case is the way I think

          I'm moving the function in the roadmap and I'll assign it to you as soon as I can get someone to upgrade your tracker permission.

          Show
          Jérôme Mouneyrac added a comment - Hi Juan, Eloy confirmed that using backup/restore without user data is the best way to achieve this feature. He also added: TARGET_EXPORT/IMPORT in this case is the way I think I'm moving the function in the roadmap and I'll assign it to you as soon as I can get someone to upgrade your tracker permission.
          Hide
          Jérôme Mouneyrac added a comment -

          All yours Juan

          Show
          Jérôme Mouneyrac added a comment - All yours Juan
          Hide
          Juan Leyva added a comment -

          Hi Jerome,

          I request you a review of the code, here you have the relevant part of the WS:

          https://gist.github.com/2397035

          If is ok for you, I will create a pull branch for you in the current Moodle HEAD

          Regards

          Show
          Juan Leyva added a comment - Hi Jerome, I request you a review of the code, here you have the relevant part of the WS: https://gist.github.com/2397035 If is ok for you, I will create a pull branch for you in the current Moodle HEAD Regards
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Juan, very good contribution

          Some small things to change:

          • add the default to visible in the function param (maybe reorder them to put it at the end)
          • I would remove $courseid = $course->id; and use $course->id directly
          • we usually use params['shortname'] instead of $shortname (same comment for other params). It's preferable to use the validated values that the one for the parameter value. The reason is that people reading your function will understand that all values need to be validated (otherwise it doesn't break anything as if the validation failed, it would have throw an exception)
          • why is there a for loop for shortname, can different course have the same shortname? I don't know, I suppose yes as you put it. Just asking to confirm.
          • I don't know much about the backup logic, but it didn't catch anything wrong. I'll ask Eloy
          Show
          Jérôme Mouneyrac added a comment - Thanks Juan, very good contribution Some small things to change: add the default to visible in the function param (maybe reorder them to put it at the end) I would remove $courseid = $course->id; and use $course->id directly we usually use params ['shortname'] instead of $shortname (same comment for other params). It's preferable to use the validated values that the one for the parameter value. The reason is that people reading your function will understand that all values need to be validated (otherwise it doesn't break anything as if the validation failed, it would have throw an exception) why is there a for loop for shortname, can different course have the same shortname? I don't know, I suppose yes as you put it. Just asking to confirm. I don't know much about the backup logic, but it didn't catch anything wrong. I'll ask Eloy
          Hide
          Jérôme Mouneyrac added a comment -

          Eloy I sending it for peer review to you, can you review line 94 to 127 ?

          Show
          Jérôme Mouneyrac added a comment - Eloy I sending it for peer review to you, can you review line 94 to 127 ?
          Hide
          Juan Leyva added a comment -

          Points 1, 2, 3 done

          Point 4, I grabbed that code from the core course creation form

          Point 5, OK

          https://gist.github.com/2397035

          Once the peer review is finished, I will run the code and comments checker and I'll create the pull branch

          Show
          Juan Leyva added a comment - Points 1, 2, 3 done Point 4, I grabbed that code from the core course creation form Point 5, OK https://gist.github.com/2397035 Once the peer review is finished, I will run the code and comments checker and I'll create the pull branch
          Hide
          Jérôme Mouneyrac added a comment -

          I'm not sure Eloy would have time to peer review with all the integration work he has to do for 2.3.
          You can run the code/comments checkers and create the pull, I'll submit it for integration. We are two weeks away from code freeze, better sending now

          Show
          Jérôme Mouneyrac added a comment - I'm not sure Eloy would have time to peer review with all the integration work he has to do for 2.3. You can run the code/comments checkers and create the pull, I'll submit it for integration. We are two weeks away from code freeze, better sending now
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Ah in fact Eloy replied to me, I just saw his chat message in my history:

          -about the backup & restore way, yup, that's the way to do so.
          -surely without user information at all, unless the user has capability to handle users in backup.
          -I mean: backup::MODE_IMPORT causes no user info to be handled.
          and perhaps it could be interesting for admins to be able to dupe courses WITH user info.
          -So, IMO, we are missing the "with/without users" decision (to support both).

          In the chat message, Eloy also mentioned that we can duplicate ONTO EXISTING course and duplicate TO NEW course. Eloy wonder if we should also let the decision to the dev client to do so. I don't know anything about the ONTO EXISTING/TO NEW things.

          -does the external support duplicate ONTO EXISTING course. or duplicate TO NEW course? Or both ?

          -we are missing the "to new/existing course" decision.

          in conclusion:

          • we could add a parameter to allow duplicating user data.
          • you should change the target for TARGET_NEW_COURSE instead of TARGET_EXISTING_ADDING
          • we should add a parameter to support the different targets:
            • TARGET_NEW_COURSE: When one create_new_course() has been performed (the current code logic)
            • TARGET_EXISTING_DELETING: When one delete_course_content() has been performed
            • TARGET_EXISTING_ADDING: When we are restoring onto one course without any of the above.
          Show
          Jérôme Mouneyrac added a comment - - edited Ah in fact Eloy replied to me, I just saw his chat message in my history: -about the backup & restore way, yup, that's the way to do so. -surely without user information at all, unless the user has capability to handle users in backup. -I mean: backup::MODE_IMPORT causes no user info to be handled. and perhaps it could be interesting for admins to be able to dupe courses WITH user info. -So, IMO, we are missing the "with/without users" decision (to support both). In the chat message, Eloy also mentioned that we can duplicate ONTO EXISTING course and duplicate TO NEW course. Eloy wonder if we should also let the decision to the dev client to do so. I don't know anything about the ONTO EXISTING/TO NEW things. -does the external support duplicate ONTO EXISTING course. or duplicate TO NEW course? Or both ? -we are missing the "to new/existing course" decision. in conclusion: we could add a parameter to allow duplicating user data. you should change the target for TARGET_NEW_COURSE instead of TARGET_EXISTING_ADDING we should add a parameter to support the different targets: TARGET_NEW_COURSE: When one create_new_course() has been performed (the current code logic) TARGET_EXISTING_DELETING: When one delete_course_content() has been performed TARGET_EXISTING_ADDING: When we are restoring onto one course without any of the above.
          Hide
          Juan Leyva added a comment -

          -we could add a parameter to allow duplicating user data.
          I think that this method should work in a similar way that the activity duplication works inside a course (does not duplicate user data, uses import also)
          Also, using the MODE_IMPORT zip and unzip compressions are not necessary because all the data is stored temporary in a directory in moodledata.
          Using a different MODE means that it will necessary handle the backups compress and uncompress, I mean:
          Perform the backup (compressing the file)
          On restore, uncompress the file and then perform the restore
          On large backups this could be an important performance hit
          +1 To delay this feature but adding a new paramether to the method called "backupoptions"
          With this possible options right now:
          'activities' => 'Default to true',
          'blocks' => 'Default to true',
          'filters' => 'Default to true',
          So in a future, we can add new options like (users, logs, enrolments, etc..) without breaking the method params definition

          we should add a parameter to support the different targets:
          I think that using a function called duplicate course to import a backup into a existing course can create some confusion
          I don't see a clear way to define a good parameters definition for handling both cases (new course you need shortname, fullname, category) existing you need a courseid
          I suggest to create a new function called:
          core_course_import_course (course to import, course to import onto, target_type)

          Show
          Juan Leyva added a comment - -we could add a parameter to allow duplicating user data. I think that this method should work in a similar way that the activity duplication works inside a course (does not duplicate user data, uses import also) Also, using the MODE_IMPORT zip and unzip compressions are not necessary because all the data is stored temporary in a directory in moodledata. Using a different MODE means that it will necessary handle the backups compress and uncompress, I mean: Perform the backup (compressing the file) On restore, uncompress the file and then perform the restore On large backups this could be an important performance hit +1 To delay this feature but adding a new paramether to the method called "backupoptions" With this possible options right now: 'activities' => 'Default to true', 'blocks' => 'Default to true', 'filters' => 'Default to true', So in a future, we can add new options like (users, logs, enrolments, etc..) without breaking the method params definition we should add a parameter to support the different targets: I think that using a function called duplicate course to import a backup into a existing course can create some confusion I don't see a clear way to define a good parameters definition for handling both cases (new course you need shortname, fullname, category) existing you need a courseid I suggest to create a new function called: core_course_import_course (course to import, course to import onto, target_type)
          Hide
          Jérôme Mouneyrac added a comment -

          Concerning part a):
          I don't know much about backup but I remember Eloy mentioning activities option. Eloy is expecting the issue to go back and forward to integration as it's not an easy one. So go for it juan

          Concerning part b)
          As the current goal is to duplicate a course I agree that it could cause confusion to add TARGET_EXISTING_ADDING. But as the problem seem to be just the naming, I would still support everything in one function. It's easy to create another function core_course_import_course() that internally call the one function doing everything.

          I suppose if we go for one function, we can have a courseidtoimport parameter. if this parameter is empty it's a TARGET_NEW_COURSE. Otherwise it's a TARGET_EXISTING_ADDING or a TARGET_EXISTING_DELETING. The function could be called core_course_copy_course() which is a bit more generic.

          Of course this is to avoid duplicated code, if you end up to have two different logic then create two functions. What do you think?

          Show
          Jérôme Mouneyrac added a comment - Concerning part a): I don't know much about backup but I remember Eloy mentioning activities option. Eloy is expecting the issue to go back and forward to integration as it's not an easy one. So go for it juan Concerning part b) As the current goal is to duplicate a course I agree that it could cause confusion to add TARGET_EXISTING_ADDING. But as the problem seem to be just the naming, I would still support everything in one function. It's easy to create another function core_course_import_course() that internally call the one function doing everything. I suppose if we go for one function, we can have a courseidtoimport parameter. if this parameter is empty it's a TARGET_NEW_COURSE. Otherwise it's a TARGET_EXISTING_ADDING or a TARGET_EXISTING_DELETING. The function could be called core_course_copy_course() which is a bit more generic. Of course this is to avoid duplicated code, if you end up to have two different logic then create two functions. What do you think?
          Hide
          Juan Leyva added a comment -

          Part a) OK, I will add the new options parameter

          Part b) I still think that we should split the functionality in two methods. The code is going to be different because:

          • In the import method we don't have to create a new course
          • In the import method the capabilities checking are different (we have to check different capabilities in each case)
          • In the import method we have to handle the backup and restore using compress files
          • In the import method the code for backup and restore has different parameters
          • The method documentation will be different for each scenario
          • In Moodle the backup, restore and import options have their owns uis and theirs own settings in the block so I think that is most consistent to have two separete funcionts for duplication, importing and in a future, backup and restore methods

          So, I think that mix all the code in the same will create a code full of (if / else) not fairly readable code and also is not consistent with the rest of Moodle

          This is just my opinion, I think that the final decision is yours

          Show
          Juan Leyva added a comment - Part a) OK, I will add the new options parameter Part b) I still think that we should split the functionality in two methods. The code is going to be different because: In the import method we don't have to create a new course In the import method the capabilities checking are different (we have to check different capabilities in each case) In the import method we have to handle the backup and restore using compress files In the import method the code for backup and restore has different parameters The method documentation will be different for each scenario In Moodle the backup, restore and import options have their owns uis and theirs own settings in the block so I think that is most consistent to have two separete funcionts for duplication, importing and in a future, backup and restore methods So, I think that mix all the code in the same will create a code full of (if / else) not fairly readable code and also is not consistent with the rest of Moodle This is just my opinion, I think that the final decision is yours
          Hide
          Juan Leyva added a comment -

          Code updated with a new parameter:

          Options (include or exclude in the backup the following):
          'activities' => 'Default to true',
          'blocks' => 'Default to true',
          'filters' => 'Default to true',

          I have tested all the possible scenarios and works fine

          See the code:
          https://gist.github.com/2397035

          Please, check if this:

          // Strict check for a correct value (allways 1 or 0, true or false).
          if ($option['value'] !== 0 and $option['value'] !== 1)

          { throw new moodle_exception('invalidextparam', 'webservice', '', $option['name']); }

          Is ok for checking a backup setting value, or should I force a bool param?:

          $value = !$option[value] ? false : true;

          Show
          Juan Leyva added a comment - Code updated with a new parameter: Options (include or exclude in the backup the following): 'activities' => 'Default to true', 'blocks' => 'Default to true', 'filters' => 'Default to true', I have tested all the possible scenarios and works fine See the code: https://gist.github.com/2397035 Please, check if this: // Strict check for a correct value (allways 1 or 0, true or false). if ($option ['value'] !== 0 and $option ['value'] !== 1) { throw new moodle_exception('invalidextparam', 'webservice', '', $option['name']); } Is ok for checking a backup setting value, or should I force a bool param?: $value = !$option [value] ? false : true;
          Hide
          Jérôme Mouneyrac added a comment -
          • go for two functions, no worries.
          • 0 and 1 are perfect, actually I don't recommend boolean are they still cause issue with one of the server (if I remember it's REST).
          • the case 'xxx' executing the case: 'yyy', I didn't know it would work that way
          • all look ok to me, can add test instructions (like the delete courses, it was perfect) + github branch, I'll submit it.

          Thanks,
          Jerome

          Show
          Jérôme Mouneyrac added a comment - go for two functions, no worries. 0 and 1 are perfect, actually I don't recommend boolean are they still cause issue with one of the server (if I remember it's REST). the case 'xxx' executing the case: 'yyy', I didn't know it would work that way all look ok to me, can add test instructions (like the delete courses, it was perfect) + github branch, I'll submit it. Thanks, Jerome
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Testing it I found out that you put all your code into the deprecated class moodle_course_external

          Show
          Jérôme Mouneyrac added a comment - - edited Testing it I found out that you put all your code into the deprecated class moodle_course_external
          Hide
          Jérôme Mouneyrac added a comment -

          Also you miss the param names into the test instruction, I'm changing them.

          Show
          Jérôme Mouneyrac added a comment - Also you miss the param names into the test instruction, I'm changing them.
          Hide
          Jérôme Mouneyrac added a comment -

          All works fine. I'll submit it once you moved the cod to core_course_external.
          Thanks

          Show
          Jérôme Mouneyrac added a comment - All works fine. I'll submit it once you moved the cod to core_course_external. Thanks
          Hide
          Juan Leyva added a comment -

          Sorry Jerome, I added the code in the correct place, you can submit it for integration

          The parameters name were not necessary because I was testing with the XMLRPC client (not the REST one)

          PD: I requested a peer review of MDL-30084 (core_course_get_categories)

          Show
          Juan Leyva added a comment - Sorry Jerome, I added the code in the correct place, you can submit it for integration The parameters name were not necessary because I was testing with the XMLRPC client (not the REST one) PD: I requested a peer review of MDL-30084 (core_course_get_categories)
          Hide
          Jérôme Mouneyrac added a comment -

          ah you are right, I didn't see you used the XMLRPC one in your test instruction, I'm so used to REST.
          Thank you.

          Show
          Jérôme Mouneyrac added a comment - ah you are right, I didn't see you used the XMLRPC one in your test instruction, I'm so used to REST. Thank you.
          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
          Aparup Banerjee added a comment - - edited

          Hi guys,
          I've tried integrating this as it makes sense. However while integrating, i noted some problems that need to be fixed.

          • The current integration has changes that leads to 'e620e37 Fixing the WS core_course_duplicate_course code ubication at MDL-32233' not having any resulting changes after conflict resolution. This led me to think its better to delay this for just one more week to simply allow rebasing on this week's changes.

          so till next week, cheers!

          Show
          Aparup Banerjee added a comment - - edited Hi guys, I've tried integrating this as it makes sense. However while integrating, i noted some problems that need to be fixed. The commit message formats need to be corrected. Commit messages at the very least must contain MDL-32233 . See http://docs.moodle.org/dev/Commit_cheat_sheet#Provide_clear_commit_messages The current integration has changes that leads to 'e620e37 Fixing the WS core_course_duplicate_course code ubication at MDL-32233 ' not having any resulting changes after conflict resolution. This led me to think its better to delay this for just one more week to simply allow rebasing on this week's changes. so till next week, cheers!
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Juan,
          I can't cherry-pick/rebase with the task becoming a nightmare in git (conflict are due to a big MDL about doc that got integrated and changed a massive amount of code in web service). I'll suppose the best for you is to get the last master and to copy your code into a new branch and redo a unique commit. I think it's going to be pretty easier this way than going through all your commits during a rebase or multiple cherry-pick.

          If you want I can do it for you. I'll keep you as author but you'll be losing your commit counter. it's why I didn't do it yet.
          Cheers,
          Jerome

          Show
          Jérôme Mouneyrac added a comment - Hi Juan, I can't cherry-pick/rebase with the task becoming a nightmare in git (conflict are due to a big MDL about doc that got integrated and changed a massive amount of code in web service). I'll suppose the best for you is to get the last master and to copy your code into a new branch and redo a unique commit. I think it's going to be pretty easier this way than going through all your commits during a rebase or multiple cherry-pick. If you want I can do it for you. I'll keep you as author but you'll be losing your commit counter. it's why I didn't do it yet. Cheers, Jerome
          Hide
          Aparup Banerjee added a comment -

          Hi Jerome, i gave it a shot just now :

          https://github.com/nebgor/moodle/compare/integration_master...integration_MDL-32233

          can you just do a quick check here that it is correct.

          Show
          Aparup Banerjee added a comment - Hi Jerome, i gave it a shot just now : https://github.com/nebgor/moodle/compare/integration_master...integration_MDL-32233 can you just do a quick check here that it is correct.
          Hide
          Jérôme Mouneyrac added a comment -

          Actually there is a mistake Apu, the same that made me stop working on rebasing/cherrypick. You have twice the moodle_course_external class inside your code and the function code is in one of them when it should be in none of them (these functions are deprecated). I guess the best is for Juan to copy his piece of code in a clean updated branch and do one commit.

          Show
          Jérôme Mouneyrac added a comment - Actually there is a mistake Apu, the same that made me stop working on rebasing/cherrypick. You have twice the moodle_course_external class inside your code and the function code is in one of them when it should be in none of them (these functions are deprecated). I guess the best is for Juan to copy his piece of code in a clean updated branch and do one commit.
          Hide
          Aparup Banerjee added a comment -

          reopening for Juan.

          Show
          Aparup Banerjee added a comment - reopening for Juan.
          Hide
          Juan Leyva added a comment -

          As we talked before:

          Here is the updated branch with the original method
          https://github.com/jleyva/moodle/compare/master...MDL-32233-core_course_duplicate_course

          Here is the updated branch with the original method and the users data support:
          https://github.com/jleyva/moodle/compare/master...MDL-32233-core_course_duplicate_course-usersdata

          This a diff between the last two branches:
          https://github.com/jleyva/moodle/commit/9aa84e9168c9c52a9f4b28683e6ae43563d71c7f

          Waiting for Jerome validation

          Show
          Juan Leyva added a comment - As we talked before: Here is the updated branch with the original method https://github.com/jleyva/moodle/compare/master...MDL-32233-core_course_duplicate_course Here is the updated branch with the original method and the users data support: https://github.com/jleyva/moodle/compare/master...MDL-32233-core_course_duplicate_course-usersdata This a diff between the last two branches: https://github.com/jleyva/moodle/commit/9aa84e9168c9c52a9f4b28683e6ae43563d71c7f Waiting for Jerome validation
          Hide
          Fábio Souto added a comment -

          Really cool function Juan. I was thinking of something like this a few days ago and voilá!
          Here it is.

          Fabio

          Show
          Fábio Souto added a comment - Really cool function Juan. I was thinking of something like this a few days ago and voilá! Here it is. Fabio
          Hide
          Juan Leyva added a comment -

          Thanks Fabio, waiting for Jerome to sent it again for integration review

          Jerome, please, can you review also this function related?

          http://tracker.moodle.org/browse/MDL-32919

          core_course_import_course

          I've finished the code, tested and works fine, do you think that we are in time for 2.3? Parts of the code (the backup and restore functions) are pretty close to the ones in core_course_duplicate

          Regards

          Show
          Juan Leyva added a comment - Thanks Fabio, waiting for Jerome to sent it again for integration review Jerome, please, can you review also this function related? http://tracker.moodle.org/browse/MDL-32919 core_course_import_course I've finished the code, tested and works fine, do you think that we are in time for 2.3? Parts of the code (the backup and restore functions) are pretty close to the ones in core_course_duplicate Regards
          Hide
          Jérôme Mouneyrac added a comment -

          Integrators: sorry guys, the code changed (about 100 lines) so it need rereview (you can have a look to https://github.com/jleyva/moodle/commit/9aa84e9168c9c52a9f4b28683e6ae43563d71c7f to focus on the last change).
          The changes cause no problem on web service level, submitting for integration.

          Show
          Jérôme Mouneyrac added a comment - Integrators: sorry guys, the code changed (about 100 lines) so it need rereview (you can have a look to https://github.com/jleyva/moodle/commit/9aa84e9168c9c52a9f4b28683e6ae43563d71c7f to focus on the last change). The changes cause no problem on web service level, submitting for integration.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Juan,

          everything looks perfect. But here there are two tiny details related with backup/temp/xxx files handling.

          1) Your code does:

          https://github.com/jleyva/moodle/compare/master...MDL-32233-core_course_duplicate_course-usersdata#L0R813

          just based in the users setting, and reusing the backupid directory... you decide to unzip the backup file while there are chances of the contents being already there (if the mode is IMPORT or if the user has the 'keepbackups" CFG setting enabled). IMO it would be easier to, always, simply check if backup/temp/$backupid/moodle_backup.xml exists and done. That would prevent some "silly" complete-overwrite already existing files under some present and future situations. If the backup files are already there... let's start restore with them.

          2) Also, depending of the mode/settings... you can end with some $file stored and it's never deleted... I think the ws should be responsible to delete that stored $file if present.

          Nice one! Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Juan, everything looks perfect. But here there are two tiny details related with backup/temp/xxx files handling. 1) Your code does: https://github.com/jleyva/moodle/compare/master...MDL-32233-core_course_duplicate_course-usersdata#L0R813 just based in the users setting, and reusing the backupid directory... you decide to unzip the backup file while there are chances of the contents being already there (if the mode is IMPORT or if the user has the 'keepbackups" CFG setting enabled). IMO it would be easier to, always, simply check if backup/temp/$backupid/moodle_backup.xml exists and done. That would prevent some "silly" complete-overwrite already existing files under some present and future situations. If the backup files are already there... let's start restore with them. 2) Also, depending of the mode/settings... you can end with some $file stored and it's never deleted... I think the ws should be responsible to delete that stored $file if present. Nice one! Ciao
          Hide
          Juan Leyva added a comment -

          Fixed, diff here https://github.com/jleyva/moodle/commit/6c7d3e3100f515317d6fbe410e5498e411d5780f#course/externallib.php

          Please, note that I'm not sure if $results['backup_destination'] allways return a file object (I think that in MODE IMPORT it doesn't) so I've added further checkings

          Show
          Juan Leyva added a comment - Fixed, diff here https://github.com/jleyva/moodle/commit/6c7d3e3100f515317d6fbe410e5498e411d5780f#course/externallib.php Please, note that I'm not sure if $results ['backup_destination'] allways return a file object (I think that in MODE IMPORT it doesn't) so I've added further checkings
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Some more interesting changes are coming here after chatting with Juan about this...

          Show
          Eloy Lafuente (stronk7) added a comment - Some more interesting changes are coming here after chatting with Juan about this...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          (sorry Apu, I've stolen this from you)

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks! (sorry Apu, I've stolen this from you)
          Hide
          Juan Leyva added a comment -

          This is the diff that shows what we talk about

          https://github.com/jleyva/moodle/commit/d7f465d4976cdb6d0d1fd4d57604d766550dc17d

          Now, the WS forces to use allways MODE_SAMESITE instead using MODE_IMPORT or MODE_SAMESITE depending on ws parameters.

          This change will prevent problems in sites where export and import are disabled/forbidden

          Show
          Juan Leyva added a comment - This is the diff that shows what we talk about https://github.com/jleyva/moodle/commit/d7f465d4976cdb6d0d1fd4d57604d766550dc17d Now, the WS forces to use allways MODE_SAMESITE instead using MODE_IMPORT or MODE_SAMESITE depending on ws parameters. This change will prevent problems in sites where export and import are disabled/forbidden
          Hide
          Ankit Agarwal added a comment -

          works as expected!
          Passing
          Thanks

          Show
          Ankit Agarwal added a comment - works as expected! Passing Thanks
          Hide
          Eloy Lafuente (stronk7) added a comment -

          U P S T R E A M I Z E D !

          Many thanks for the hard work, closing this as fixed.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - U P S T R E A M I Z E D ! Many thanks for the hard work, closing this as fixed. Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: