Moodle
  1. Moodle
  2. MDL-16982

Adding data mapping for custom user fields

    Details

    • Testing Instructions:
      Hide

      Pre-requisite:

      1. CAS and LDAP server configured to be used with moodle
      2. Prevent account creation when authenticating" (Site administration ► Plugins ► Authentication ► Manage authentication) is not selected.

      TEST

      1. Log in as admin and create custom user profile field
      2. Map CAS and LDAP fields to be retrieved from SERVER (Site administration ► Plugins ► Authentication ► CAS server (SSO)). Make sure custom profile field is visible and you map it to valid field from LDAP
      3. Logout and try log in as user who is not in moodle, but in CAS/LDAP
      4. Make sure all mapped fields are filled, especially custom field
      5. Log out and change custom field value in LDAP.
      6. Login again with same user and check if field is updated in moodle.
      Show
      Pre-requisite: CAS and LDAP server configured to be used with moodle Prevent account creation when authenticating" (Site administration ► Plugins ► Authentication ► Manage authentication) is not selected. TEST Log in as admin and create custom user profile field Map CAS and LDAP fields to be retrieved from SERVER (Site administration ► Plugins ► Authentication ► CAS server (SSO)). Make sure custom profile field is visible and you map it to valid field from LDAP Logout and try log in as user who is not in moodle, but in CAS/LDAP Make sure all mapped fields are filled, especially custom field Log out and change custom field value in LDAP. Login again with same user and check if field is updated in moodle.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull Master Branch:
      wip-mdl-16982
    • Story Points:
      40
    • Rank:
      44478
    • Sprint:
      BACKEND Sprint 1

      Description

      I'd like having the possibility to add data mapping for custom user fields when I use LDAP or DB auth modules

      1. auth_config.php
        10 kB
        Kyle Egan
      2. auth.php
        86 kB
        Kyle Egan
      3. config.html
        19 kB
        Kyle Egan
      4. customfields.patch
        981 kB
        Gilles-Philippe Leblanc
      5. moodlelib.php
        356 kB
        Kyle Egan
      6. patch2.patch
        13 kB
        Olav Jordan
      1. customprofilefield.png
        51 kB

        Issue Links

          Activity

          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sounds logical for me. Assigning to Martin for his consideration. Addressing for Moodle 2.0, perhaps as part of the auth/enrol rework?. Adding some specialists in custom profile fields & authentication/enrolments.

          Thanks for report. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Sounds logical for me. Assigning to Martin for his consideration. Addressing for Moodle 2.0, perhaps as part of the auth/enrol rework?. Adding some specialists in custom profile fields & authentication/enrolments. Thanks for report. Ciao
          Hide
          Martin Dougiamas added a comment -

          Yes, absolutely.

          This would be part of the general process of converting most of of the fixed user profile fields into custom user fields in Moodle 2.0, which solves a lot of things. There is a bug for this somewhere ..... Shane?

          Show
          Martin Dougiamas added a comment - Yes, absolutely. This would be part of the general process of converting most of of the fixed user profile fields into custom user fields in Moodle 2.0, which solves a lot of things. There is a bug for this somewhere ..... Shane?
          Hide
          Domenico Pontari added a comment -

          We already developed this feature for Moodle 1.8
          If you think it could be useful for Moodle's developers I can post our hack.
          However if you are planning a rework for auth component, it's better if you don't think about the code you wrote before.

          Show
          Domenico Pontari added a comment - We already developed this feature for Moodle 1.8 If you think it could be useful for Moodle's developers I can post our hack. However if you are planning a rework for auth component, it's better if you don't think about the code you wrote before.
          Hide
          Shane Elliott added a comment -

          Related issue is: MDL-10908

          Show
          Shane Elliott added a comment - Related issue is: MDL-10908
          Hide
          Fernando Martin added a comment -

          Hi there!

          Finally I found a post related to my requirement. (happy for that)

          Look, I'm connecting Moodle to an LDAP previously authenticated against a CAS server.
          Among the fields in the LDAP tree I'm storing the userRoleID, which is quite suggesting of what is its purpose, don't you think?
          Well... Now I need to find a way to bind that roleID to Moodle's roleIDs.

          The problem here is that our client is a bit in a hurry and maybe he couldn't wait till v2.0

          Is there any way / help / code you could give me in order to solve this?
          Sorry, but I'm no familiar to Moodle's code even though I'm PHP dev.

          Any guidelines will be very appreciated!

          Thnx for reading.
          Fernando Martin (aka "Pampa")

          Show
          Fernando Martin added a comment - Hi there! Finally I found a post related to my requirement. (happy for that) Look, I'm connecting Moodle to an LDAP previously authenticated against a CAS server. Among the fields in the LDAP tree I'm storing the userRoleID, which is quite suggesting of what is its purpose, don't you think? Well... Now I need to find a way to bind that roleID to Moodle's roleIDs. The problem here is that our client is a bit in a hurry and maybe he couldn't wait till v2.0 Is there any way / help / code you could give me in order to solve this? Sorry, but I'm no familiar to Moodle's code even though I'm PHP dev. Any guidelines will be very appreciated! Thnx for reading. Fernando Martin (aka "Pampa")
          Hide
          Petr Škoda added a comment -

          Please use moodle.org forums for general support, this might not be the best place

          Show
          Petr Škoda added a comment - Please use moodle.org forums for general support, this might not be the best place
          Hide
          Fernando Martin added a comment -

          Ok Petr.. sorry for my bad.

          I came here Googling
          I'll re-post my message.

          Kind regards!
          Pampa

          Show
          Fernando Martin added a comment - Ok Petr.. sorry for my bad. I came here Googling I'll re-post my message. Kind regards! Pampa
          Hide
          Olav Jordan added a comment -

          Just wanted to say I made a patch for this issues in moodle 1.9

          Show
          Olav Jordan added a comment - Just wanted to say I made a patch for this issues in moodle 1.9
          Hide
          Olav Jordan added a comment -

          patch for custom fields mapping to LDAP in moodle 1.9

          Show
          Olav Jordan added a comment - patch for custom fields mapping to LDAP in moodle 1.9
          Hide
          Michael Hughes added a comment -

          Olav, I have got your patch working, kinda. The trouble is, unless there are already entries in user_info_data for the relevant custom fields, they are never updated on login. Should there be a bit somewhere that says 'if entries not present then create them before updating them from active directory fields'?

          Show
          Michael Hughes added a comment - Olav, I have got your patch working, kinda. The trouble is, unless there are already entries in user_info_data for the relevant custom fields, they are never updated on login. Should there be a bit somewhere that says 'if entries not present then create them before updating them from active directory fields'?
          Hide
          Pawel Suwinski added a comment -

          Here is my refactored patch suited for 1.9.5 version: MDL-24252

          Show
          Pawel Suwinski added a comment - Here is my refactored patch suited for 1.9.5 version: MDL-24252
          Hide
          tommy do added a comment -

          Hi everyone,

          can anyone tell me if this patch works on moodle 2.0? I tried to apply this patch manually ( not run through GnuWin32 by adding the "+ row" and deactivating the "- row" which are described in the patch. Is that the right way to apply patches? Besides i would be appreciate if someone who has successfully done that could show me some screenshots of data mapping after adding fields.

          Show
          tommy do added a comment - Hi everyone, can anyone tell me if this patch works on moodle 2.0? I tried to apply this patch manually ( not run through GnuWin32 by adding the "+ row" and deactivating the "- row" which are described in the patch. Is that the right way to apply patches? Besides i would be appreciate if someone who has successfully done that could show me some screenshots of data mapping after adding fields.
          Hide
          Howard Miller added a comment -

          It appears to be a patch for 1.9. It won't work in 2.0 (2.0 handles these fields differently AFAIK)

          Show
          Howard Miller added a comment - It appears to be a patch for 1.9. It won't work in 2.0 (2.0 handles these fields differently AFAIK)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          NOTE: This issue was assigned to the STABLE backlog without complete triaging process. Marking it as triaged, but with this note for future reference.

          Show
          Eloy Lafuente (stronk7) added a comment - NOTE: This issue was assigned to the STABLE backlog without complete triaging process. Marking it as triaged, but with this note for future reference.
          Hide
          Gilles-Philippe Leblanc added a comment -

          Here is the patch for Moodle 2

          Show
          Gilles-Philippe Leblanc added a comment - Here is the patch for Moodle 2
          Hide
          Steve Bubb added a comment -

          Hi all
          I would like to try use this patch. Do I just use the linux patch command in the moodle directory?
          Should this patch also work with moodle 2.1?
          I only have very basic php code knowledge.
          Thanks
          Steve

          Show
          Steve Bubb added a comment - Hi all I would like to try use this patch. Do I just use the linux patch command in the moodle directory? Should this patch also work with moodle 2.1? I only have very basic php code knowledge. Thanks Steve
          Hide
          Christine Stange added a comment -

          Steve - I was able to apply customfields.patch to 2.1.3 and it worked fine, as far as I can tell. I did a careful comparison though between the files before deciding what to merge. There were some differences between 2.0 and 2.1.3. If you only apply the custom fields related code (they're pretty obvious), then you should be okay. There were some files that were much more straightforward than others (ex. the 2 HTML files).

          Show
          Christine Stange added a comment - Steve - I was able to apply customfields.patch to 2.1.3 and it worked fine, as far as I can tell. I did a careful comparison though between the files before deciding what to merge. There were some differences between 2.0 and 2.1.3. If you only apply the custom fields related code (they're pretty obvious), then you should be okay. There were some files that were much more straightforward than others (ex. the 2 HTML files).
          Hide
          Kyle Egan added a comment -

          Hi I have just used the above code to update my 2.2 install. See the attached files you need.

          Show
          Kyle Egan added a comment - Hi I have just used the above code to update my 2.2 install. See the attached files you need.
          Hide
          Kyle Egan added a comment - - edited

          ..\moodle\admin\auth_config.php
          ..\moodle\auth\ldap\auth.php
          ..\moodle\auth\ldap\config.html
          ..\moodle\lib\moodlelib.php

          Show
          Kyle Egan added a comment - - edited ..\moodle\admin\auth_config.php ..\moodle\auth\ldap\auth.php ..\moodle\auth\ldap\config.html ..\moodle\lib\moodlelib.php
          Hide
          Marko Vidberg added a comment -

          I can't get the supplied patches here to apply to any version and I am trying to get a working patch for Moodle 2.1.5. Figuring out the changes for auth_config.php, auth.php and config.html is easy but not so easy for moodlelib.php as that file changes a lot. If anybody has got a working patch file for 2.1.5, I would be appreciated.

          Show
          Marko Vidberg added a comment - I can't get the supplied patches here to apply to any version and I am trying to get a working patch for Moodle 2.1.5. Figuring out the changes for auth_config.php, auth.php and config.html is easy but not so easy for moodlelib.php as that file changes a lot. If anybody has got a working patch file for 2.1.5, I would be appreciated.
          Hide
          paul added a comment -

          Some help would be good on this one, using moodle 2.2 and not sure what to do with these files

          I suppose careful comparison between the files is needed
          Cheers

          Show
          paul added a comment - Some help would be good on this one, using moodle 2.2 and not sure what to do with these files I suppose careful comparison between the files is needed Cheers
          Hide
          Gilles-Philippe Leblanc added a comment -

          I putted GitHub links for this task for Master and for Moodle22_Stable.
          If someone could test this task, it should be appreciated.

          Show
          Gilles-Philippe Leblanc added a comment - I putted GitHub links for this task for Master and for Moodle22_Stable. If someone could test this task, it should be appreciated.
          Hide
          Dan Poltawski added a comment -

          Removing Eloy as peer reviewer on this so someone else can take up the gauntlet

          Show
          Dan Poltawski added a comment - Removing Eloy as peer reviewer on this so someone else can take up the gauntlet
          Hide
          Gilles-Philippe Leblanc added a comment -

          Thanks Dan.

          As a new developer in the community, I didn't be able to put this task on the "Waiting for peer review" status. I was only able to assign someone as peer reviewer... Maybe I missed something

          Show
          Gilles-Philippe Leblanc added a comment - Thanks Dan. As a new developer in the community, I didn't be able to put this task on the "Waiting for peer review" status. I was only able to assign someone as peer reviewer... Maybe I missed something
          Hide
          Rajesh Taneja added a comment -

          Thanks for the patch Gilles,

          I am reviewing this patch, in the mean while can you please update testing instructions.

          Show
          Rajesh Taneja added a comment - Thanks for the patch Gilles, I am reviewing this patch, in the mean while can you please update testing instructions.
          Hide
          Rajesh Taneja added a comment -

          Hello Gilles,

          Code looks good so far, I am still looking at this. But there are few things you might want to consider for now.

          1. Code alignment is not correct at few places (https://github.com/StudiUM/moodle/compare/master...MDL-16982_mapping_user_customfields#L0R222)
          2. Coding style is not followed (https://github.com/StudiUM/moodle/compare/master...MDL-16982_mapping_user_customfields#L0R178), space between if and opening bracket is required
          3. Custom_fields has been added to config_form function, for LDAP and SAS. But this is overloaded function of auth_plugin_base which is not changed. You have to add custom_fields to all definitions of config_form (In all auth modules)
          4. It might be nice to introduce $custom_fields variable in auth_pulgin_base class and add value in respective auth module. As checking for custom_field at https://github.com/StudiUM/moodle/compare/master...MDL-16982_mapping_user_customfields#L5R3760 for another plugin will show undefined notice.
          5. It might be nice to add some comments (documentation), especially in moodlelib.php, as this is overriding input data, it might be nice to know why this is done.
          6. IMO https://github.com/StudiUM/moodle/compare/master...MDL-16982_mapping_user_customfields#L3R265 should use &&, if the value is not set then why compare?
          7. It might be nice to have more brackets on https://github.com/StudiUM/moodle/compare/master...MDL-16982_mapping_user_customfields#L5R3761 for readability

          Not sure how this will work for bulk user upload. Do we need to change custom_fields while uploading bulk user data?

          Show
          Rajesh Taneja added a comment - Hello Gilles, Code looks good so far, I am still looking at this. But there are few things you might want to consider for now. Code alignment is not correct at few places ( https://github.com/StudiUM/moodle/compare/master...MDL-16982_mapping_user_customfields#L0R222 ) Coding style is not followed ( https://github.com/StudiUM/moodle/compare/master...MDL-16982_mapping_user_customfields#L0R178 ), space between if and opening bracket is required Custom_fields has been added to config_form function, for LDAP and SAS. But this is overloaded function of auth_plugin_base which is not changed. You have to add custom_fields to all definitions of config_form (In all auth modules) It might be nice to introduce $custom_fields variable in auth_pulgin_base class and add value in respective auth module. As checking for custom_field at https://github.com/StudiUM/moodle/compare/master...MDL-16982_mapping_user_customfields#L5R3760 for another plugin will show undefined notice. It might be nice to add some comments (documentation), especially in moodlelib.php, as this is overriding input data, it might be nice to know why this is done. IMO https://github.com/StudiUM/moodle/compare/master...MDL-16982_mapping_user_customfields#L3R265 should use &&, if the value is not set then why compare? It might be nice to have more brackets on https://github.com/StudiUM/moodle/compare/master...MDL-16982_mapping_user_customfields#L5R3761 for readability Not sure how this will work for bulk user upload. Do we need to change custom_fields while uploading bulk user data?
          Hide
          Gilles-Philippe Leblanc added a comment -

          Hello,

          In the first place. thank you for all the feedback and tested for the task.
          Unfortunately, I do not have more time to devote to this task so I would ask someone to take over.

          Thanks.

          Show
          Gilles-Philippe Leblanc added a comment - Hello, In the first place. thank you for all the feedback and tested for the task. Unfortunately, I do not have more time to devote to this task so I would ask someone to take over. Thanks.
          Hide
          Rajesh Taneja added a comment -

          Thanks for all the efforts on this, Gilles.

          I will take this forward from here.

          Show
          Rajesh Taneja added a comment - Thanks for all the efforts on this, Gilles. I will take this forward from here.
          Hide
          Rajesh Taneja added a comment -

          I revisited this and it seems we need to prefix profile_field to custom profile field to have consistency.
          Initial work is here: https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-16982. Will test this and push for review.

          Show
          Rajesh Taneja added a comment - I revisited this and it seems we need to prefix profile_field to custom profile field to have consistency. Initial work is here: https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-16982 . Will test this and push for review.
          Show
          Rajesh Taneja added a comment - Original Branch from Gilles. Pull repo: https://github.com/organizations/StudiUM Master branch: https://github.com/StudiUM/moodle/tree/MDL-16982_mapping_user_customfields Diff branch: https://github.com/StudiUM/moodle/compare/master...MDL-16982_mapping_user_customfields
          Hide
          Luis de Vasconcelos added a comment - - edited

          Rajesh

          Your testing instructions include:

          3. Logout and try log in as user who is not in moodle, but in CAS/LDAP
          4. Make sure all mapped fields are filled, especially custom field

          Does that mean that the CAS/LDAP user should be created in Moodle the first time they login to Moodle? What happens if the "Prevent account creation when authenticating" (authpreventaccountcreation) option under Site administration / Plugins / Authentication / Manage authentication IS SELECTED? If it IS selected then the user should never be created in Moodle. Do you agree?

          Show
          Luis de Vasconcelos added a comment - - edited Rajesh Your testing instructions include: 3. Logout and try log in as user who is not in moodle, but in CAS/LDAP 4. Make sure all mapped fields are filled, especially custom field Does that mean that the CAS/LDAP user should be created in Moodle the first time they login to Moodle? What happens if the "Prevent account creation when authenticating" (authpreventaccountcreation) option under Site administration / Plugins / Authentication / Manage authentication IS SELECTED? If it IS selected then the user should never be created in Moodle. Do you agree?
          Hide
          Rajesh Taneja added a comment - - edited

          Thanks for pointing this Luis,

          I agree with you, I should have added this as prerequisite.
          As we are trying to sync mapped data, I assumed that it should not be set.

          Show
          Rajesh Taneja added a comment - - edited Thanks for pointing this Luis, I agree with you, I should have added this as prerequisite. As we are trying to sync mapped data, I assumed that it should not be set.
          Hide
          Jason Fowler added a comment -

          [Y] Syntax
          [Y] Output
          [Y] Whitespace
          [Y] Language
          [NA] Databases
          [Y] Testing
          [NA] Security
          [Y] Documentation
          [Y] Git
          [N] Sanity check

          There is that one section I pointed out to you Raj where some insert changes come between a comment and it's code. Once that is fixed it should all be good.

          Show
          Jason Fowler added a comment - [Y] Syntax [Y] Output [Y] Whitespace [Y] Language [NA] Databases [Y] Testing [NA] Security [Y] Documentation [Y] Git [N] Sanity check There is that one section I pointed out to you Raj where some insert changes come between a comment and it's code. Once that is fixed it should all be good.
          Hide
          Rajesh Taneja added a comment -

          Thanks Jason, I have fixed that comment. Pushing it for integration review.

          Show
          Rajesh Taneja added a comment - Thanks Jason, I have fixed that comment. Pushing it for integration review.
          Hide
          Rajesh Taneja added a comment -

          Removing last commit as strict warning will be fixed by MDL-36138.

          Show
          Rajesh Taneja added a comment - Removing last commit as strict warning will be fixed by MDL-36138 .
          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 -

          Hi Gilles/Raj,

          Iñaki Arenaza looks after and maintains our LDAP/CAS plugins (for the most part). As he has not been involved in this issue and was not a watcher on it, I am going to reopen it to wait for feedback from Iñaki.

          I would not feel confident adding these changes to the ldap plugins without Iñaki's support.

          thanks!
          Dan

          Show
          Dan Poltawski added a comment - Hi Gilles/Raj, Iñaki Arenaza looks after and maintains our LDAP/CAS plugins (for the most part). As he has not been involved in this issue and was not a watcher on it, I am going to reopen it to wait for feedback from Iñaki. I would not feel confident adding these changes to the ldap plugins without Iñaki's support. thanks! Dan
          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
          Rajesh Taneja added a comment -

          Hello Iñaki,

          Can you please provide some feedback on attached patch.

          Show
          Rajesh Taneja added a comment - Hello Iñaki, Can you please provide some feedback on attached patch.
          Hide
          Iñaki Arenaza added a comment -

          Hi Rajesh,

          I'm rather busy right now and won't probably have a chance to have a look at the patch in a serious way before the week-end at least (and can't promise it either, as I'm in the middle of the worst month of the work year).

          Saludos.
          Iñaki.

          Show
          Iñaki Arenaza added a comment - Hi Rajesh, I'm rather busy right now and won't probably have a chance to have a look at the patch in a serious way before the week-end at least (and can't promise it either, as I'm in the middle of the worst month of the work year). Saludos. Iñaki.
          Hide
          Rajesh Taneja added a comment -

          Thanks for update Iñaki,

          Will wait for your response on this patch

          Show
          Rajesh Taneja added a comment - Thanks for update Iñaki, Will wait for your response on this patch
          Hide
          Rajesh Taneja added a comment -

          Moving this to next Sprint and will wait for Iñaki's feedback.

          Show
          Rajesh Taneja added a comment - Moving this to next Sprint and will wait for Iñaki's feedback.
          Hide
          Iñaki Arenaza added a comment -

          Hi Rajesh,

          I finally had a look at the code, and given that I couldn't add comments to the diff page at Github, I'm attaching my comments here. A short note about the syntax of the comments: #LnRmmmm refers to the HTML anchor to add to "https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-16982" link to show the relevant line(s) of code.

          Here are the comments:

          • #L2R123 Remove "global $DB;" it's not needed at all.
          • #L2R266 See discussion about this in MDL-33406 (https://tracker.moodle.org/browse/MDL-33406?focusedCommentId=192902&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-192902)
          • #L2R1123 Shouldn't we set $profile_field to '' before each loop iteration? otherwise we could reuse a value from the previous iteration if we don't execute any of the if/else if branches.
          • #L2R1150 I don't see why we do this.
          • #L2R1419 Why don't you simply merge $this->userfields and $customfields arrays (like you do in #L0R117) and process the merged array? This avoids code duplication.
          • #L4R522 Typo: retrived -> retrieved
          • #L4R529 Why don't we work with $this->customfields directly?
          • #L5R3879 Use single quotes for table name
          • #L5R3942 I think the comment is missing some text after the 'then' part
          • #L5R3944 Don't call get_field() inside the loop. The user is always the same (and you should specify mnethostid in the select conditions, as usernames may not unique). In any case, this is redundant as we already have the user id in $olduser->id.
          • #L5R3953 Use single quotes for table name.
          • #L5R3952 Shouldn't this be "$row->data = $value;"? Also we should do "$data = $value;" as we are going to test $data below (but I'm not 100% sure about this).
          • #L5R3948 to #L5R3955 Why don't we check that the new value is a valid one, like we do in #L5R3958 to #L5R3970? I'd say we can refactor that part of the code a bit.

          Saludos.
          Iñaki.

          Show
          Iñaki Arenaza added a comment - Hi Rajesh, I finally had a look at the code, and given that I couldn't add comments to the diff page at Github, I'm attaching my comments here. A short note about the syntax of the comments: #LnRmmmm refers to the HTML anchor to add to "https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-16982" link to show the relevant line(s) of code. Here are the comments: #L2R123 Remove "global $DB;" it's not needed at all. #L2R266 See discussion about this in MDL-33406 ( https://tracker.moodle.org/browse/MDL-33406?focusedCommentId=192902&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-192902 ) #L2R1123 Shouldn't we set $profile_field to '' before each loop iteration? otherwise we could reuse a value from the previous iteration if we don't execute any of the if/else if branches. #L2R1150 I don't see why we do this. #L2R1419 Why don't you simply merge $this->userfields and $customfields arrays (like you do in #L0R117) and process the merged array? This avoids code duplication. #L4R522 Typo: retrived -> retrieved #L4R529 Why don't we work with $this->customfields directly? #L5R3879 Use single quotes for table name #L5R3942 I think the comment is missing some text after the 'then' part #L5R3944 Don't call get_field() inside the loop. The user is always the same (and you should specify mnethostid in the select conditions, as usernames may not unique). In any case, this is redundant as we already have the user id in $olduser->id. #L5R3953 Use single quotes for table name. #L5R3952 Shouldn't this be "$row->data = $value;"? Also we should do "$data = $value;" as we are going to test $data below (but I'm not 100% sure about this). #L5R3948 to #L5R3955 Why don't we check that the new value is a valid one, like we do in #L5R3958 to #L5R3970? I'd say we can refactor that part of the code a bit. Saludos. Iñaki.
          Hide
          Rajesh Taneja added a comment -

          Thanks for the detailed feedback Iñaki,

          I have integrated all your suggestion. Pushing it back for your review.

          FYI: Added another commit, so you can scan though changes. Will squash them once you are happy with the patch.

          Show
          Rajesh Taneja added a comment - Thanks for the detailed feedback Iñaki, I have integrated all your suggestion. Pushing it back for your review. FYI: Added another commit, so you can scan though changes. Will squash them once you are happy with the patch.
          Hide
          Rajesh Taneja added a comment -

          Hello Iñaki,,

          Can you please review this again.

          Show
          Rajesh Taneja added a comment - Hello Iñaki,, Can you please review this again.
          Hide
          Iñaki Arenaza added a comment -

          Hi Rajesh,

          I'm afraid I'm still in the middle of a storm. I'll try to have a look at it during the weekend.

          Saludos.
          Iñaki.

          Show
          Iñaki Arenaza added a comment - Hi Rajesh, I'm afraid I'm still in the middle of a storm. I'll try to have a look at it during the weekend. Saludos. Iñaki.
          Hide
          Rajesh Taneja added a comment -

          Thanks Iñaki.

          Show
          Rajesh Taneja added a comment - Thanks Iñaki.
          Hide
          Michael de Raadt added a comment -

          Shifting to the next sprint.

          Show
          Michael de Raadt added a comment - Shifting to the next sprint.
          Hide
          Rajesh Taneja added a comment -

          Hello Iñaki,

          Wondering if you have time to look at this.

          Thanks.

          Show
          Rajesh Taneja added a comment - Hello Iñaki, Wondering if you have time to look at this. Thanks.
          Hide
          Pavlik Morozov added a comment -

          Hi Rajesh,

          See #L4R543, there is a duplicated $this->customfield assignment.

          Show
          Pavlik Morozov added a comment - Hi Rajesh, See #L4R543, there is a duplicated $this->customfield assignment.
          Hide
          Rajesh Taneja added a comment -

          Thanks Pavlik,

          Duplicated line removed.

          Show
          Rajesh Taneja added a comment - Thanks Pavlik, Duplicated line removed.
          Hide
          Dave Arnold added a comment -

          This is exactly what I need for a 2.3 installation. The links to github appear to be outdated. Where can I get the latest version of this work?

          Show
          Dave Arnold added a comment - This is exactly what I need for a 2.3 installation. The links to github appear to be outdated. Where can I get the latest version of this work?
          Hide
          Rajesh Taneja added a comment -

          Hello Dave,

          I have re-based branch on top of master. Let me know if you need any help.

          Show
          Rajesh Taneja added a comment - Hello Dave, I have re-based branch on top of master. Let me know if you need any help.
          Hide
          Iñaki Arenaza added a comment -

          Hi Rajesh,

          sorry for being so late, but I've had a look at your updated patch and I'd say it's in good shape to be integrated.

          So +1 from me.

          Show
          Iñaki Arenaza added a comment - Hi Rajesh, sorry for being so late, but I've had a look at your updated patch and I'd say it's in good shape to be integrated. So +1 from me.
          Hide
          Dave Arnold added a comment -

          Thanks Rajesh,

          I grabbed it, gonna work on integrating it with the external db auth module.

          Show
          Dave Arnold added a comment - Thanks Rajesh, I grabbed it, gonna work on integrating it with the external db auth module.
          Hide
          Rajesh Taneja added a comment -

          Thanks Iñaki,

          Pushing for integration.

          Show
          Rajesh Taneja added a comment - Thanks Iñaki, Pushing for integration.
          Hide
          Dan Poltawski added a comment -

          Adding integration_held, as this is a master only new feature we will only add to 2.6 once the on-sync period ends.

          Show
          Dan Poltawski added a comment - Adding integration_held, as this is a master only new feature we will only add to 2.6 once the on-sync period ends.
          Hide
          Dave Arnold added a comment -

          Do the locking options actually work on the custom fields. Since custom fields are handled differently than standard user fields can the unlock if empty option even work?

          Show
          Dave Arnold added a comment - Do the locking options actually work on the custom fields. Since custom fields are handled differently than standard user fields can the unlock if empty option even work?
          Hide
          Sam Hemelryk added a comment -

          Hi Raj,

          The change looks pretty good, there are however a couple of performance qualms I have with these changes.

          1. https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-16982#L5R3856 (create_user_record)
            Do we need to be calling this within the foreach loop? Couldn't it be easily moved outside of the foreach or fetched the first time within the loop if required and then re-used?
          2. https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-16982#L5R3859 (create_user_record)
            This seems pretty wasteful if there are several custom fields provided by auth->get_userinfo. Surely we could iterate and bulk fetch, perhaps even as simple as array_intersect(array_keys($newinfo), $customfields)?
          3. https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-16982#L5R3964 (update_user_record)
            Looks like these two queries could be combined into a single query really easily.
          4. https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-16982#L4R546 (auth_plugin_base::get_custom_user_profile_fields)
            I wonder whether the return array should be changed to use the shortname as the key. I can't think what use a numeric order key and given that in a couple of places we are str_replace'ing to get the shortname from the combined I think that probably makes good sense towards saving a few operations.

          Other things:

          1. https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-16982#L5R3976 (update_user_record)
            $info_field doesn't appear to be defined (I havn't executed but so says my editor )
          2. Should this be using the profile fields API when inserting/updating? I'm a little sceptical presently because its not. This seems like a flaw as profile fields may be doing processing of one form or another. Of course those API's look like they are really focused around interacting with forms and this data isn't coming from a form as that code expects.

          Re-opening for the time being to get these things either discussed and if required addressed.
          Just noting as well there is a big queue in integration this week in order to progress I stopped my review after the above points as there seems plenty to clarify/work on now.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Raj, The change looks pretty good, there are however a couple of performance qualms I have with these changes. https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-16982#L5R3856 (create_user_record) Do we need to be calling this within the foreach loop? Couldn't it be easily moved outside of the foreach or fetched the first time within the loop if required and then re-used? https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-16982#L5R3859 (create_user_record) This seems pretty wasteful if there are several custom fields provided by auth->get_userinfo. Surely we could iterate and bulk fetch, perhaps even as simple as array_intersect(array_keys($newinfo), $customfields)? https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-16982#L5R3964 (update_user_record) Looks like these two queries could be combined into a single query really easily. https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-16982#L4R546 (auth_plugin_base::get_custom_user_profile_fields) I wonder whether the return array should be changed to use the shortname as the key. I can't think what use a numeric order key and given that in a couple of places we are str_replace'ing to get the shortname from the combined I think that probably makes good sense towards saving a few operations. Other things: https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-16982#L5R3976 (update_user_record) $info_field doesn't appear to be defined (I havn't executed but so says my editor ) Should this be using the profile fields API when inserting/updating? I'm a little sceptical presently because its not. This seems like a flaw as profile fields may be doing processing of one form or another. Of course those API's look like they are really focused around interacting with forms and this data isn't coming from a form as that code expects. Re-opening for the time being to get these things either discussed and if required addressed. Just noting as well there is a big queue in integration this week in order to progress I stopped my review after the above points as there seems plenty to clarify/work on now. Many thanks Sam
          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
          Rajesh Taneja added a comment -

          Thanks for the feedback Sam,

          I have added one more commit, integrating 1 and 2.

          As per rest of the feedback:
          Point 3: Not sure how two queries can be combined. Can you please help me here.
          Point 4: Shortname can't be used as key, as it might mix us with user fields like email and profile_field_email. (Refered profile field api, load_data)
          Other things:
          Point 1: $info_field is set by $infofield = $DB->get_record('user_info_field', array('shortname' => $shortname)); from point 3.
          Point 2: Well I thought of using profile API, but they doesn't seem right because they are more intended towards form and will add more overhead of loading field.

          Show
          Rajesh Taneja added a comment - Thanks for the feedback Sam, I have added one more commit, integrating 1 and 2. As per rest of the feedback: Point 3: Not sure how two queries can be combined. Can you please help me here. Point 4: Shortname can't be used as key, as it might mix us with user fields like email and profile_field_email. (Refered profile field api, load_data) Other things: Point 1: $info_field is set by $infofield = $DB->get_record('user_info_field', array('shortname' => $shortname)); from point 3. Point 2: Well I thought of using profile API, but they doesn't seem right because they are more intended towards form and will add more overhead of loading field.
          Hide
          Rajesh Taneja added a comment -

          Hello Sam,

          As discussed I have added another commit to use Profile API. I have not modified prefix for field key as it is expected to be there for profile api's to work.

          Hope it's good to go...

          Show
          Rajesh Taneja added a comment - Hello Sam, As discussed I have added another commit to use Profile API. I have not modified prefix for field key as it is expected to be there for profile api's to work. Hope it's good to go...
          Hide
          Sam Hemelryk added a comment -

          Hi Raj,

          Thanks for tidying things up, it looks much better now.

          I am however sending this back sorry.
          As this was reopened yesterday to get in today I would have to have been confident that changes made were thoroughly tested.
          After finding two unused variables in create_user_record ($customfields and $customprofilefields) I am going to assume this wasn't thoroughly tested.
          I am sending this back so that that can happen and the above noted can be fixed.

          My apologies if I've got it wrong.

          Regards
          Sam

          Show
          Sam Hemelryk added a comment - Hi Raj, Thanks for tidying things up, it looks much better now. I am however sending this back sorry. As this was reopened yesterday to get in today I would have to have been confident that changes made were thoroughly tested. After finding two unused variables in create_user_record ($customfields and $customprofilefields) I am going to assume this wasn't thoroughly tested. I am sending this back so that that can happen and the above noted can be fixed. My apologies if I've got it wrong. Regards Sam
          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
          Rajesh Taneja added a comment -

          Thanks Sam,

          I did test it, but it forgot to push the latest branch.

          Pushing it back for your review.

          Show
          Rajesh Taneja added a comment - Thanks Sam, I did test it, but it forgot to push the latest branch. Pushing it back for your review.
          Hide
          Sam Hemelryk added a comment -

          Hi Raj,

          This is looking so promising now.
          However something is definitely not quite right yet.
          If I run phpunit tests before and after this patch they pass before and I get a fail after.
          vendor/bin/phpunit auth_db_testcase auth/db/tests/db_test.php

          1) auth_db_testcase::test_plugin
          Failed asserting that two strings are identical.
          — Expected
          +++ Actual
          @@ @@
          -u4b@example.com
          +u4@example.com

          /var/www/integration/auth/db/tests/db_test.php:353
          /var/www/integration/lib/phpunit/classes/advanced_testcase.php:76

          Sorry - reopening.
          Sam

          Show
          Sam Hemelryk added a comment - Hi Raj, This is looking so promising now. However something is definitely not quite right yet. If I run phpunit tests before and after this patch they pass before and I get a fail after. vendor/bin/phpunit auth_db_testcase auth/db/tests/db_test.php 1) auth_db_testcase::test_plugin Failed asserting that two strings are identical. — Expected +++ Actual @@ @@ -u4b@example.com +u4@example.com /var/www/integration/auth/db/tests/db_test.php:353 /var/www/integration/lib/phpunit/classes/advanced_testcase.php:76 Sorry - reopening. Sam
          Hide
          Rajesh Taneja added a comment -

          Thanks Sam,

          I will check why this is failing.

          Show
          Rajesh Taneja added a comment - Thanks Sam, I will check why this is failing.
          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
          Rajesh Taneja added a comment -

          Thanks Sam,

          I have fixed unit test problem, and also removed query from forloop.

          Show
          Rajesh Taneja added a comment - Thanks Sam, I have fixed unit test problem, and also removed query from forloop.
          Hide
          Sam Hemelryk added a comment -

          Yay, thanks Raj this has been integrated now.

          Show
          Sam Hemelryk added a comment - Yay, thanks Raj this has been integrated now.
          Hide
          Adrian Greeve added a comment -

          Tested on the master integration branch.
          I initially created a custom user field with some uppercase characters in the short name, but after talking with Rajesh about it, it was discovered that this was stopping the information from being mapped from the LDAP server. We deleted that custom field and created a new one, all lower case.
          Everything worked fine.
          Test passed.

          Show
          Adrian Greeve added a comment - Tested on the master integration branch. I initially created a custom user field with some uppercase characters in the short name, but after talking with Rajesh about it, it was discovered that this was stopping the information from being mapped from the LDAP server. We deleted that custom field and created a new one, all lower case. Everything worked fine. Test passed.
          Hide
          Dan Poltawski added a comment -

          Thanks for your contributions!

          _main:
          @ BB#0:
                  push    {r7, lr}
                  mov     r7, sp
                  sub     sp, #4
                  movw    r0, :lower16:(L_.str-(LPC0_0+4))
                  movt    r0, :upper16:(L_.str-(LPC0_0+4))
          LPC0_0:
                  add     r0, pc
                  bl      _printf
                  movs    r1, #0
                  movt    r1, #0
                  str     r0, [sp]                @ 4-byte Spill
                  mov     r0, r1
                  add     sp, #4
                  pop     {r7, pc}
          
                  .section        __TEXT,__cstring,cstring_literals
          L_.str:                                 @ @.str
                  .asciz   "This code is now upstream!"
          
          Show
          Dan Poltawski added a comment - Thanks for your contributions! _main: @ BB#0: push {r7, lr} mov r7, sp sub sp, #4 movw r0, :lower16:(L_.str-(LPC0_0+4)) movt r0, :upper16:(L_.str-(LPC0_0+4)) LPC0_0: add r0, pc bl _printf movs r1, #0 movt r1, #0 str r0, [sp] @ 4- byte Spill mov r0, r1 add sp, #4 pop {r7, pc} .section __TEXT,__cstring,cstring_literals L_.str: @ @.str .asciz "This code is now upstream!"
          Hide
          James Henestofel added a comment - - edited

          I was testing this on Moodle 2.6dev (Build: 20130920). I've attached an image customprofilefield.png

          It seems that if you enter a NUMBER at the end of the shortname for any of the field types there is a string error on the LDAP and CAS mapping section. I'm not sure if numbers aren't allowed because it's not in the Docs. It only happens when you enter a number at the end of the shortname.

          Show
          James Henestofel added a comment - - edited I was testing this on Moodle 2.6dev (Build: 20130920). I've attached an image customprofilefield.png It seems that if you enter a NUMBER at the end of the shortname for any of the field types there is a string error on the LDAP and CAS mapping section. I'm not sure if numbers aren't allowed because it's not in the Docs. It only happens when you enter a number at the end of the shortname.
          Hide
          Rajesh Taneja added a comment -

          Thanks James,

          This seems to be an issue, with print_auth_lock_options(), where field is checked with regular expression "(preg_match('/^(.?)(\d)$/', $fieldname, $matches))".
          Please feel free to open a bug for same.

          Show
          Rajesh Taneja added a comment - Thanks James, This seems to be an issue, with print_auth_lock_options(), where field is checked with regular expression "(preg_match('/^(. ?)(\d )$/', $fieldname, $matches))". Please feel free to open a bug for same.
          Hide
          Lael... added a comment -

          Hi - I've started playing around with this to use with enrolling students into courses by their custom profile fields. However, the fields don't seem to be be filling in from AD? The fields pull to a standard field without any problems. Do the custom user fields need to be all lowercase? special chars ok?/ not ok?

          Thanks!

          Show
          Lael... added a comment - Hi - I've started playing around with this to use with enrolling students into courses by their custom profile fields. However, the fields don't seem to be be filling in from AD? The fields pull to a standard field without any problems. Do the custom user fields need to be all lowercase? special chars ok?/ not ok? Thanks!
          Hide
          Roman Pavlacka added a comment -

          Hi! What needs to be done if I want to map custom fields in external db auth configuration? I can only find this under CAS auth, but the description of this issue mentions db auth as well. I'm using Moodle 2.6.1. Thanks!

          Show
          Roman Pavlacka added a comment - Hi! What needs to be done if I want to map custom fields in external db auth configuration? I can only find this under CAS auth, but the description of this issue mentions db auth as well. I'm using Moodle 2.6.1. Thanks!

            People

            • Votes:
              24 Vote for this issue
              Watchers:
              41 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Agile