Moodle
  1. Moodle
  2. MDL-41778

LDAP Enrolment - Allow update of course fields during sync

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5.2
    • Fix Version/s: 2.6
    • Component/s: Enrolments
    • Labels:
    • Testing Instructions:
      Hide

      1/ configure ldap sync with an LDAP server
      2/ configure mapping for shortname, fullname ,summary and idnumber of courses
      3/ enable course auto creation
      4/ when "update fullname", "update shortname" or "update_summary" are set to be updated they should be updated during sync script
      5/ run cli script enrol/ldap/cli/sync.php
      6/ change one or more of the field values in ldap server (shortname, fullname or idnumber)
      7/ run cli script enrol/ldap/cli/sync.php again
      8/ verify settings made in (4) (fields that were supposed to be updated were updated and those that were not were not updated)

      Show
      1/ configure ldap sync with an LDAP server 2/ configure mapping for shortname, fullname ,summary and idnumber of courses 3/ enable course auto creation 4/ when "update fullname", "update shortname" or "update_summary" are set to be updated they should be updated during sync script 5/ run cli script enrol/ldap/cli/sync.php 6/ change one or more of the field values in ldap server (shortname, fullname or idnumber) 7/ run cli script enrol/ldap/cli/sync.php again 8/ verify settings made in (4) (fields that were supposed to be updated were updated and those that were not were not updated)
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull from Repository:
    • Pull 2.5 Branch:
      MDL-41778_MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-41778_master
    • Rank:
      52928

      Description

      During LDAP sync allow to update course shortname, fullname, summary from LDAP server (optionally).

      Add an "update during sync" option to each of the above course fields in enrol/ldap config.

        Activity

        Hide
        Nadav Kavalerchik added a comment -

        Hi Michael de Raadt, I was going through the code and it seems ok to me.
        Who is the lead maintainer of the LDAP module, that can give a proper code review?

        Show
        Nadav Kavalerchik added a comment - Hi Michael de Raadt , I was going through the code and it seems ok to me. Who is the lead maintainer of the LDAP module, that can give a proper code review?
        Hide
        Frédéric Massart added a comment -

        Hi Nitzan,

        thanks for working on this. I am taking over the peer review of this, not knowing if there is a component lead for LDAP. Here are some comments:

        1. I think the update_course() method should be protected instead of private.
        2. About $this->get_config('course_'.$field.'updateonsync') == '1': It appears that we are usually not comparing a checkbox value == '1', see _ignorehiddencourses.
        3. I don't think there is a need to pass the entire course object to update_course, only the modified fields and the ID should be set there.
        4. I would add a trace() when the course is updated. Though, thinking of it, it might be good to have 3 traces:
          • Cannot update course, or failed course update
          • Update skipped because there is no need for course update
          • Course updated successfully (with fields updated?)
        5. I think I would prefer to have all the settings in a new section called Automatic course update settings or so. In there, I would use selectyesno elements for each of the fields, and also a general setting autoupdate (see below).

        I like the patch, and this is definitely a good feature. I am just a little concerned about the performance hit on this. I am suggesting to create a new setting autoupdate so that we can easily bypass any of the logic checking if the course has to be updated or not. Also, there definitely should be a check to ensure that another course does not exist with the same shortname (similar to MDL-41259) to the risk of an exception being raisd from courselib::update_course().

        Many thanks!
        Fred

        Show
        Frédéric Massart added a comment - Hi Nitzan, thanks for working on this. I am taking over the peer review of this, not knowing if there is a component lead for LDAP. Here are some comments: I think the update_course() method should be protected instead of private . About $this->get_config('course_'.$field.' updateonsync') == '1': It appears that we are usually not comparing a checkbox value == '1', see _ignorehiddencourses . I don't think there is a need to pass the entire course object to update_course, only the modified fields and the ID should be set there. I would add a trace() when the course is updated. Though, thinking of it, it might be good to have 3 traces: Cannot update course, or failed course update Update skipped because there is no need for course update Course updated successfully (with fields updated?) I think I would prefer to have all the settings in a new section called Automatic course update settings or so. In there, I would use selectyesno elements for each of the fields, and also a general setting autoupdate (see below). I like the patch, and this is definitely a good feature. I am just a little concerned about the performance hit on this. I am suggesting to create a new setting autoupdate so that we can easily bypass any of the logic checking if the course has to be updated or not. Also, there definitely should be a check to ensure that another course does not exist with the same shortname (similar to MDL-41259 ) to the risk of an exception being raisd from courselib::update_course(). Many thanks! Fred
        Hide
        Nitzan Bar added a comment -

        Hi Fred,

        Thanks for your comments I agree with all of them.
        One thing though - Don't you think adding another global setting "Autoupdate" could confuse the user and complicate the configuration of the plugin?
        I agree there is a performance hit when no fields are selected for update, so maybe instead we should do a Logical OR on all the fields that are candidates for update and save that in a static variable inside the update_course() method. Then, on subsequent calls to update_course() we can check the value of the static variable and return immediately if no candidates for update exist.

        What do you say?

        Thanks again!

        Nitzan

        Show
        Nitzan Bar added a comment - Hi Fred, Thanks for your comments I agree with all of them. One thing though - Don't you think adding another global setting "Autoupdate" could confuse the user and complicate the configuration of the plugin? I agree there is a performance hit when no fields are selected for update, so maybe instead we should do a Logical OR on all the fields that are candidates for update and save that in a static variable inside the update_course() method. Then, on subsequent calls to update_course() we can check the value of the static variable and return immediately if no candidates for update exist. What do you say? Thanks again! Nitzan
        Hide
        Frédéric Massart added a comment -

        Hi Nitzan,

        I don't know if that would confuse a user, it reflects the setting that you need to set to automatically create courses. Also, it highlights the fact that you are going to update the course, which you might not realise if you just select a single 'Update shortname'. I have no objections, those are my thoughts, feel free to go in one direction or the other. If it's not worth having a "Auto update" setting, then don't add any . As long as it is clear in the user interface that an update is going to happen when selecting one of the fields, then I am fine.

        Many thanks!
        Fred

        Show
        Frédéric Massart added a comment - Hi Nitzan, I don't know if that would confuse a user, it reflects the setting that you need to set to automatically create courses. Also, it highlights the fact that you are going to update the course, which you might not realise if you just select a single 'Update shortname'. I have no objections, those are my thoughts, feel free to go in one direction or the other. If it's not worth having a "Auto update" setting, then don't add any . As long as it is clear in the user interface that an update is going to happen when selecting one of the fields, then I am fine. Many thanks! Fred
        Hide
        Nitzan Bar added a comment -

        Hi Fred,

        I have updated everything.
        Let me know what you think.

        Also I have created a branch for version 2.5, even though this is an improvement because it was easy to backport (simple cherry-pick). Can this go into 2.5 stable? (Version 2.4 is harder to backport because a lot of changes where made to the plugin).

        Thanks,

        Nitzan

        Show
        Nitzan Bar added a comment - Hi Fred, I have updated everything. Let me know what you think. Also I have created a branch for version 2.5, even though this is an improvement because it was easy to backport (simple cherry-pick). Can this go into 2.5 stable? (Version 2.4 is harder to backport because a lot of changes where made to the plugin). Thanks, Nitzan
        Hide
        Frédéric Massart added a comment -

        Hi Nitzan,

        Good work! Just some tiny details:

        1. We usually do not need to escape {\$a->foo} in language strings because we use single quotes. I've seen that this has been done elsewhere in the language file, it is not blocking your issue, but just so you know.
        2. You have 1 extra indentation on lines 1057 and 1065.
        3. As first I thought that the DB query to check if the shortname existed should exclude the updated course, but as $updatedcourse->shortname is only set if != than $externalcourse, it should be OK.

        Many thanks!
        Fred

        Show
        Frédéric Massart added a comment - Hi Nitzan, Good work! Just some tiny details: We usually do not need to escape {\$a->foo} in language strings because we use single quotes. I've seen that this has been done elsewhere in the language file, it is not blocking your issue, but just so you know. You have 1 extra indentation on lines 1057 and 1065. As first I thought that the DB query to check if the shortname existed should exclude the updated course, but as $updatedcourse->shortname is only set if != than $externalcourse, it should be OK. Many thanks! Fred
        Hide
        Nitzan Bar added a comment -

        Hi Fred,

        Fixed the indentations. Sorry for that

        I left the language strings as they were since I used single quotes in the string and I would have to escape them otherwise.

        Thanks,

        Nitzan

        Show
        Nitzan Bar added a comment - Hi Fred, Fixed the indentations. Sorry for that I left the language strings as they were since I used single quotes in the string and I would have to escape them otherwise. Thanks, Nitzan
        Hide
        Frédéric Massart added a comment -

        Thanks, pushing for integration. Cheers!

        Show
        Frédéric Massart added a comment - Thanks, pushing for integration. Cheers!
        Hide
        Marina Glancy added a comment -

        Thanks guys, I integrated this in master only since it is an improvement. I will create an issue to backport it to 2.5, it can be integrated after 1 month and integrators voting.

        Show
        Marina Glancy added a comment - Thanks guys, I integrated this in master only since it is an improvement. I will create an issue to backport it to 2.5, it can be integrated after 1 month and integrators voting.
        Hide
        Damyon Wiese added a comment -

        Tests all passed with flying colours.

        Thanks!

        Show
        Damyon Wiese added a comment - Tests all passed with flying colours. Thanks!
        Hide
        Marina Glancy added a comment -

        And THANK YOU again for making Moodle better every day!

        Another weekly release has been released.

        Show
        Marina Glancy added a comment - And THANK YOU again for making Moodle better every day! Another weekly release has been released.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: