Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.5
    • Component/s: Libraries
    • Labels:
    • Testing Instructions:
      Hide

      a) phpunit courselib_testcase ./course/tests/courselib_test.php

      b) In Moodle, create and update every type of module and activity. All should work fine. Try with advanced grading, outcomes, conditional availability.

      Show
      a) phpunit courselib_testcase ./course/tests/courselib_test.php b) In Moodle, create and update every type of module and activity. All should work fine. Try with advanced grading, outcomes, conditional availability.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-37079-master2
    • Rank:
      46633

      Description

      From course/modedit.php create a function create_module() that can be:
      1- used by web service
      2- used in core itself

      The code being massive it could need to be separated in different functions. Need to identify piece of code that could be reusable in other place.

      Don't forget to check in the main form (each module has different form, but I guess there should be some common part) that there is no additional checks.

        Issue Links

          Activity

          Hide
          Jérôme Mouneyrac added a comment -

          I've done most of the work. I'm missing file support:

          • in intro text editor. I currently ignore all files.
          • my functions don't support $mform parameter in MODULENAME_add_instance($data, $mform) and MODULENAME_update_instance($data, $mform). $mform is always send to null. From reading the core modules, the three that use $mform values do it to get files.

          I don't think this issues should be blocking the issue. I talk with Damyon, we'll come back to a solution for file by web services as he's working on assignment web service that also require files.

          Note: the plagiarism_save_form_elements() method will work. The external lib functions will just need to pass the correct atrribut names. I think the external function should catch/verify some couple of key/value (key being return by plagiarism plugin function get_config()) I tested with crot pro, and check turnitin code it causes no problem.

          Show
          Jérôme Mouneyrac added a comment - I've done most of the work. I'm missing file support: in intro text editor. I currently ignore all files. my functions don't support $mform parameter in MODULENAME_add_instance($data, $mform) and MODULENAME_update_instance($data, $mform). $mform is always send to null. From reading the core modules, the three that use $mform values do it to get files. I don't think this issues should be blocking the issue. I talk with Damyon, we'll come back to a solution for file by web services as he's working on assignment web service that also require files. Note: the plagiarism_save_form_elements() method will work. The external lib functions will just need to pass the correct atrribut names. I think the external function should catch/verify some couple of key/value (key being return by plagiarism plugin function get_config()) I tested with crot pro, and check turnitin code it causes no problem.
          Hide
          Jérôme Mouneyrac added a comment -

          Sending it for peer-review.

          I'm confident it doesn't break anything. It's just some refactoring + opening the API. The previous comment only concerns the new function create_module() / update_module() that will be used by externallib.php files or the PHPunit tests.

          Show
          Jérôme Mouneyrac added a comment - Sending it for peer-review. I'm confident it doesn't break anything. It's just some refactoring + opening the API. The previous comment only concerns the new function create_module() / update_module() that will be used by externallib.php files or the PHPunit tests.
          Hide
          Davo Smith added a comment -

          Does this affect the drag and drop upload code in course/dnduploadlib.php? (or should that, at least, be refactored to make use of this new API?)

          Show
          Davo Smith added a comment - Does this affect the drag and drop upload code in course/dnduploadlib.php? (or should that, at least, be refactored to make use of this new API?)
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Hi Davo,
          it will not break drag and drop.

          However about the fact that we could call the new API in this file, yes. I'll have a look to it. Thanks for mentioning it.

          Show
          Jérôme Mouneyrac added a comment - - edited Hi Davo, it will not break drag and drop. However about the fact that we could call the new API in this file, yes. I'll have a look to it. Thanks for mentioning it.
          Hide
          Damyon Wiese added a comment -

          Hi Jerome,

          Some comments:

          1. This looks like it will break advanced grading - you are not passing the boolean $showgradingmanagement back from your function to the modedit page - so if you create an assignment with a rubric, it will not take you to the rubric editing screen like it currently does.

          2. Nitpick: I really don't like no spaces before the assignment operator in your unit test code: "$moduleinfo->completion= '';"

          3. It would be useful to have the unit tests implement these parts:

          +        // Retriev the module
          +
          +        // Compare the module values
          +
          +        // error_log(print_r($forum, true));
          

          Right now they are only really testing that a fatal exception was not thrown.

          4. Can you create a ticket for moving dndupload to use this function?

          Thanks - Damyon

          Show
          Damyon Wiese added a comment - Hi Jerome, Some comments: 1. This looks like it will break advanced grading - you are not passing the boolean $showgradingmanagement back from your function to the modedit page - so if you create an assignment with a rubric, it will not take you to the rubric editing screen like it currently does. 2. Nitpick: I really don't like no spaces before the assignment operator in your unit test code: "$moduleinfo->completion= '';" 3. It would be useful to have the unit tests implement these parts: + // Retriev the module + + // Compare the module values + + // error_log(print_r($forum, true )); Right now they are only really testing that a fatal exception was not thrown. 4. Can you create a ticket for moving dndupload to use this function? Thanks - Damyon
          Hide
          Rajesh Taneja added a comment -

          Thanks Jerome, in addition to Damyon review, few minor things:

          1. use of TODO it should be @todo or inline in code with tracker number. https://github.com/mouneyrac/moodle/compare/moodle:master...MDL-37079-master2#L0R4615
          2. Inline comment should start with Capital letter and end with a dot (.). It is not done at most places. eg. https://github.com/mouneyrac/moodle/compare/moodle:master...MDL-37079-master2#L0R4767
          3. AFAIK we don't use object as datatype in phpdocs. It should be either stdClass or class. https://github.com/mouneyrac/moodle/compare/moodle:master...MDL-37079-master2#L0R4982
          Show
          Rajesh Taneja added a comment - Thanks Jerome, in addition to Damyon review, few minor things: use of TODO it should be @todo or inline in code with tracker number. https://github.com/mouneyrac/moodle/compare/moodle:master...MDL-37079-master2#L0R4615 Inline comment should start with Capital letter and end with a dot (.). It is not done at most places. eg. https://github.com/mouneyrac/moodle/compare/moodle:master...MDL-37079-master2#L0R4767 AFAIK we don't use object as datatype in phpdocs. It should be either stdClass or class. https://github.com/mouneyrac/moodle/compare/moodle:master...MDL-37079-master2#L0R4982
          Hide
          Jérôme Mouneyrac added a comment - - edited

          I'm sending this for integration. To help integration, here what have done:
          a) I basically only moved all the generic code from course/modedit.php into some new course/lib.php lib functions.
          b) I created two extra create/update_module lib functions that can be called by externallib files. These functions are not used by anything except the PHPunit tests.
          c) I fixed a compatibility issue with some assign code.
          d) I added get_configs() to plagiarism base class in order to indicate to the external functions which plagiarism settings are expected.
          e) I created the PHPunit tests and tested all what could be set when updating a module. I hope I'm exhaustive.

          At the end, This change should reproduce the exact Moodle server checks as I basically just moved the server code into lib functions. With very few additional lines.

          Still todo but I think not blocking integration:

          • support for file in create/update_module => I suggest to create an issue.
          • it would be good if we find a way to get rid of this $mform expected by our MODULE_add_instance => I suggest to create an issue.
          • update course/dnduploadlib.php to support => I suggest to create an issue.

          I suppose as it does not break anything we can integrate it, it's just that the new create/update_module (that are currently not used, they have been developped to be used by external functions) does not support file. We are working on file upload: MDL-22236 - so it's why I have been reluctant to work on the file part - some refactor may come along following advancement of MDL-22236.

          Show
          Jérôme Mouneyrac added a comment - - edited I'm sending this for integration. To help integration, here what have done: a) I basically only moved all the generic code from course/modedit.php into some new course/lib.php lib functions. b) I created two extra create/update_module lib functions that can be called by externallib files. These functions are not used by anything except the PHPunit tests. c) I fixed a compatibility issue with some assign code. d) I added get_configs() to plagiarism base class in order to indicate to the external functions which plagiarism settings are expected. e) I created the PHPunit tests and tested all what could be set when updating a module. I hope I'm exhaustive. At the end, This change should reproduce the exact Moodle server checks as I basically just moved the server code into lib functions. With very few additional lines. Still todo but I think not blocking integration: support for file in create/update_module => I suggest to create an issue. it would be good if we find a way to get rid of this $mform expected by our MODULE_add_instance => I suggest to create an issue. update course/dnduploadlib.php to support => I suggest to create an issue. I suppose as it does not break anything we can integrate it, it's just that the new create/update_module (that are currently not used, they have been developped to be used by external functions) does not support file. We are working on file upload: MDL-22236 - so it's why I have been reluctant to work on the file part - some refactor may come along following advancement of MDL-22236 .
          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 -

          Hi:

          • conflicts (apparently trivial).
          • Also it sounds to me that any change to courses / modules should be confirmed with Marina. Or were them courses / categories?

          If you can confirm, it would be great! In the mean time I continue reviewing the patch.

          Show
          Eloy Lafuente (stronk7) added a comment - Hi: conflicts (apparently trivial). Also it sounds to me that any change to courses / modules should be confirmed with Marina. Or were them courses / categories? If you can confirm, it would be great! In the mean time I continue reviewing the patch.
          Hide
          Marina Glancy added a comment -

          I love the idea of moving this kind of code from interface php files to library.
          Also I was proposing creating the course class that would contain such functions http://docs.moodle.org/dev/Course_formats_2#Class_course_.28for_discussion.2C_NOT_for_2.4.29 but it did not happen yet

          anyway, from a brief look through the changes my main is to the function names. These are global functions, visible from everywhere, they should be named properly. First, there is no such entity as "moduleinfo" yet, do we really want to use this word in functions name? Second, functions name should start with the entity followed by action, i.e. instead of update_moduleinfo() should be course_module_update(), etc. At least they should start with 'course'. Actually we have to have a naming convention. At the moment functions names are a mess and we are adding more mess now.

          Third, I would create a new file for those functions - course/lib.php has an enormous size already.

          Show
          Marina Glancy added a comment - I love the idea of moving this kind of code from interface php files to library. Also I was proposing creating the course class that would contain such functions http://docs.moodle.org/dev/Course_formats_2#Class_course_.28for_discussion.2C_NOT_for_2.4.29 but it did not happen yet anyway, from a brief look through the changes my main is to the function names. These are global functions, visible from everywhere, they should be named properly. First, there is no such entity as "moduleinfo" yet, do we really want to use this word in functions name? Second, functions name should start with the entity followed by action, i.e. instead of update_moduleinfo() should be course_module_update(), etc. At least they should start with 'course'. Actually we have to have a naming convention. At the moment functions names are a mess and we are adding more mess now. Third, I would create a new file for those functions - course/lib.php has an enormous size already.
          Hide
          Marina Glancy added a comment -

          Another reason why I think that those function should be in separate library file is because they are now are not ready to be part of global course API. They can not be used by 3rd party plugins:

          • They don't require all necessary files, such as gradeslib.php, conditionlib.php ;
          • They don't contain proper PHPdocs describing all the attributes of object $moduleinfo. At the same time in the function code we suppose that $moduleinfo has all the attributes we expect and never call isset();
          • by the name can_update_moduleinfo() one would expect that it returns boolean but it instead returns an array of entities and throws an exception if user can't update. This function just can not possibly be used anywhere else except on page editing the module. This also applies to all functions that call it;
            ...
          Show
          Marina Glancy added a comment - Another reason why I think that those function should be in separate library file is because they are now are not ready to be part of global course API. They can not be used by 3rd party plugins: They don't require all necessary files, such as gradeslib.php, conditionlib.php ; They don't contain proper PHPdocs describing all the attributes of object $moduleinfo. At the same time in the function code we suppose that $moduleinfo has all the attributes we expect and never call isset(); by the name can_update_moduleinfo() one would expect that it returns boolean but it instead returns an array of entities and throws an exception if user can't update. This function just can not possibly be used anywhere else except on page editing the module. This also applies to all functions that call it; ...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I'm reopening this on behalf of Marina's comments. Let's give it another round for better landing, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - I'm reopening this on behalf of Marina's comments. Let's give it another round for better landing, thanks!
          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
          Jérôme Mouneyrac added a comment - - edited
          The situation for people arriving

          I moved the existing modedit.php code in some new functions. When people create a web service functions, they need to call the exact same code as the front end. So this code needs to be available (i.e. not integrated inb modedit.php) and not duplicated. It's the reason of these functions being created.

          I don't think their name are important yet as they should not be used by anyone else (even thought some have to be public to be call by modedit.php). As Marina mention they don't do the full create/update and existing only for backward compatibility reason (the issue is to create the "external" core functions, not to refactor the all course structure):
          a) add_moduleinfo
          b) update_moduleinfo
          c) edit_module_post_actions
          d) set_moduleinfo_defaults
          e) can_add_moduleinfo
          f) can_update_moduleinfo
          g) include_modulelib

          The two other new functions I created for web service / third party plugin are:
          h) create_module
          i) update_module
          PS: not used by anything yet but these are the real course API functions.

          Marina

          We could rename + add proper phpdoc (I actually wrote these long object description but I removed them because there were too long and redundant with PHPunit tests. But I'll add them back, after thinking it's better if people keep them up to date.):
          h) create_module => course_module_create()
          i) update_module => course_module_update()

          The big mess comes from the other functions.

          First we can make private:
          c) edit_module_post_actions
          d) set_moduleinfo_defaults

          Then we can make public (called by modedit.php):
          a) add_moduleinfo
          b) update_moduleinfo
          e) can_add_moduleinfo
          f) can_update_moduleinfo
          g) include_modulelib

          We can add modedit_ at the beginning of each functions. Otherwise we can refactor the modedit.php too but it's a different task in my opinion. Can you take over in this case? I read the chat talk yesterday about moving everything in lib/courselib.php but you may have different opinion about these mess functions. Let me know what you think about where to move / how to name / should we refactor the all things?

          Show
          Jérôme Mouneyrac added a comment - - edited The situation for people arriving I moved the existing modedit.php code in some new functions. When people create a web service functions, they need to call the exact same code as the front end. So this code needs to be available (i.e. not integrated inb modedit.php) and not duplicated. It's the reason of these functions being created. I don't think their name are important yet as they should not be used by anyone else (even thought some have to be public to be call by modedit.php). As Marina mention they don't do the full create/update and existing only for backward compatibility reason (the issue is to create the "external" core functions, not to refactor the all course structure): a) add_moduleinfo b) update_moduleinfo c) edit_module_post_actions d) set_moduleinfo_defaults e) can_add_moduleinfo f) can_update_moduleinfo g) include_modulelib The two other new functions I created for web service / third party plugin are: h) create_module i) update_module PS: not used by anything yet but these are the real course API functions. Marina We could rename + add proper phpdoc (I actually wrote these long object description but I removed them because there were too long and redundant with PHPunit tests. But I'll add them back, after thinking it's better if people keep them up to date.): h) create_module => course_module_create() i) update_module => course_module_update() The big mess comes from the other functions. First we can make private: c) edit_module_post_actions d) set_moduleinfo_defaults Then we can make public (called by modedit.php): a) add_moduleinfo b) update_moduleinfo e) can_add_moduleinfo f) can_update_moduleinfo g) include_modulelib We can add modedit_ at the beginning of each functions. Otherwise we can refactor the modedit.php too but it's a different task in my opinion. Can you take over in this case? I read the chat talk yesterday about moving everything in lib/courselib.php but you may have different opinion about these mess functions. Let me know what you think about where to move / how to name / should we refactor the all things?
          Hide
          Jérôme Mouneyrac added a comment - - edited

          I added Sam as watcher as Marina and Sam were recently talking about the all course lib structure. From my last information I should move all these functions into a new course/lib.php ? What about the "modedit"/"mess" functions?

          Show
          Jérôme Mouneyrac added a comment - - edited I added Sam as watcher as Marina and Sam were recently talking about the all course lib structure. From my last information I should move all these functions into a new course/lib.php ? What about the "modedit"/"mess" functions?
          Hide
          Jérôme Mouneyrac added a comment -

          Notet: this issue is blocking generic create/update module descriptions => which is blocking all ws create/update_forum, create/update_assign, create/update_xxx.

          Show
          Jérôme Mouneyrac added a comment - Notet: this issue is blocking generic create/update module descriptions => which is blocking all ws create/update_forum, create/update_assign, create/update_xxx.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Ok I just had a conversation with Sam (Sam wants to talk about it with integrators/seniors next week). I think we don't have the resource to refactor the all modedit.php/course API in a reasonable time. But as we need the two news functions course_module_create() and course_module_update() for ws, this is what I suggest:

          1) move the modedit.php code in a modedit_lib.php => this is a library class. Obviously developer will understand it will be called by modedit.php. It contains all the functions I created above (except the two course API ones). These methods will start with modedit_xxx.
          2) from what I understood the course API will be in course/lib.php. So I'll just add course_module_create() and course_module_update() to the new course API class/file. Obviously these functions will also call the modedit_lib.php methods and it's where the refactor will start but I think this is currently the cleaner and realist solution given the time and goals we have.

          Show
          Jérôme Mouneyrac added a comment - - edited Ok I just had a conversation with Sam (Sam wants to talk about it with integrators/seniors next week). I think we don't have the resource to refactor the all modedit.php/course API in a reasonable time. But as we need the two news functions course_module_create() and course_module_update() for ws, this is what I suggest: 1) move the modedit.php code in a modedit_lib.php => this is a library class. Obviously developer will understand it will be called by modedit.php. It contains all the functions I created above (except the two course API ones). These methods will start with modedit_xxx. 2) from what I understood the course API will be in course/lib.php. So I'll just add course_module_create() and course_module_update() to the new course API class/file. Obviously these functions will also call the modedit_lib.php methods and it's where the refactor will start but I think this is currently the cleaner and realist solution given the time and goals we have.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Ping (Sam)

          Show
          Jérôme Mouneyrac added a comment - - edited Ping (Sam)
          Hide
          Jérôme Mouneyrac added a comment -

          ping

          Show
          Jérôme Mouneyrac added a comment - ping
          Hide
          Dan Poltawski added a comment -

          Regarding where to put the functions, rather than block Jerome - I suggest we integrate this into course/lib.php and whoever is refactoring it can do that later (when decision is made).

          Show
          Dan Poltawski added a comment - Regarding where to put the functions, rather than block Jerome - I suggest we integrate this into course/lib.php and whoever is refactoring it can do that later (when decision is made).
          Hide
          Damyon Wiese added a comment -

          -1 for the course/modedit_lib.php

          I think this should be:

          course/modlib.php

          Show
          Damyon Wiese added a comment - -1 for the course/modedit_lib.php I think this should be: course/modlib.php
          Hide
          Damyon Wiese added a comment -

          Agree with Dan on integrating this in course/lib.php and moving it later.

          Show
          Damyon Wiese added a comment - Agree with Dan on integrating this in course/lib.php and moving it later.
          Hide
          Jérôme Mouneyrac added a comment -

          Moved the modedit functions into course/modlib.php

          Show
          Jérôme Mouneyrac added a comment - Moved the modedit functions into course/modlib.php
          Hide
          Damyon Wiese added a comment -

          Hi Jerome,

          I got a merge conflict with course/modedit.php

          Comparing the version of add_module with the old from the merge conflict found that the new function is missing "$DB->set_field('course_modules', 'visibleold', 1, array('id' => $fromform->coursemodule));"

          Which was added by MDL-37430 on 22nd of Jan.

          This concerns me - what other changes haven't been merged properly to your branch?

          Can you rebase and meticulously check that all recent changes to modedit.php have been included in your new branches?

          Thanks, Damyon

          Show
          Damyon Wiese added a comment - Hi Jerome, I got a merge conflict with course/modedit.php Comparing the version of add_module with the old from the merge conflict found that the new function is missing "$DB->set_field('course_modules', 'visibleold', 1, array('id' => $fromform->coursemodule));" Which was added by MDL-37430 on 22nd of Jan. This concerns me - what other changes haven't been merged properly to your branch? Can you rebase and meticulously check that all recent changes to modedit.php have been included in your new branches? Thanks, Damyon
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Thanks Damyon, I checked there were nothing more added since I started excepted your change. Added it, retested, and rebased.

          Show
          Jérôme Mouneyrac added a comment - - edited Thanks Damyon, I checked there were nothing more added since I started excepted your change. Added it, retested, and rebased.
          Hide
          Damyon Wiese added a comment -

          Hi Jerome,

          I made the testing instructions a bit more onerous - as I'm a bit cautious of regressions in this change - but it is a good improvement and worth spending the effort on.

          This is integrated to master now - thanks for rechecking the branch.

          Show
          Damyon Wiese added a comment - Hi Jerome, I made the testing instructions a bit more onerous - as I'm a bit cautious of regressions in this change - but it is a good improvement and worth spending the effort on. This is integrated to master now - thanks for rechecking the branch.
          Hide
          Damyon Wiese added a comment -

          I added 2 commits to cleanup some whitespace and indenting and I removed the "continue 2" from the code.

          Show
          Damyon Wiese added a comment - I added 2 commits to cleanup some whitespace and indenting and I removed the "continue 2" from the code.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Don't hesitate (if possible) to add a link to the additional change/commit you do during integration, it's always worth for me to see what I forgot, what I did wrong

          Show
          Jérôme Mouneyrac added a comment - - edited Don't hesitate (if possible) to add a link to the additional change/commit you do during integration, it's always worth for me to see what I forgot, what I did wrong
          Hide
          Damyon Wiese added a comment -

          If I add extra commits they will all have the issue code so you can find them with "git log --grep=MDL-XXX" they don't go in github so it's a bit harder to generate a link for them.

          Show
          Damyon Wiese added a comment - If I add extra commits they will all have the issue code so you can find them with "git log --grep=MDL-XXX" they don't go in github so it's a bit harder to generate a link for them.
          Hide
          Andrew Davis added a comment -

          This seems to be working fine. Passing.

          Show
          Andrew Davis added a comment - This seems to be working fine. Passing.
          Hide
          Damyon Wiese added a comment -

          This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads).

          Thanks for your contributions!

          Show
          Damyon Wiese added a comment - This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads). Thanks for your contributions!

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: