New setting for identification use #95
Conversation
add userfield option/choose from settings
Fix method name
* fix failing test repository * change quote * switch back to
fix failing test repository (#4)
|
Thank you @gbarat87 for the PR. We'll have a look. :) @PenghaiZhang you're the one who's most recently worked on the moodle module, could you please review this PR? |
There was a problem hiding this comment.
Hi @gbarat87 Thanks for adding this new setting! I have left some questions and suggestions for you.
| global $USER, $CFG; | ||
| if(!isset($USER->equellauser) && isset($USER->username)) { | ||
| $userfield = $CFG->equella_userfield; | ||
| if ($userfield != 'default' && isset($USER->profile[$userfield])) { |
There was a problem hiding this comment.
Can I ask where is $USER->profile[$userfield] set ?
| @@ -38,6 +38,13 @@ function ecs($configoption, $params = null) { | |||
| $settings->add(new admin_setting_configtext('equella_url', ecs('url.title'), ecs('url.desc'), '')); | |||
| $settings->add(new admin_setting_configtext('equella_action', ecs('action.title'), ecs('action.desc'), '')); | |||
|
|
|||
| $userfieldoptions = array(); | |||
| $userfieldoptions = array('default' => ecs('userfield.username')); | |||
There was a problem hiding this comment.
Instead of default, why not use Username as the default option ?
define('EQUELLA_SSO_IDENTIFICATION_FIELD', 'Username');
| foreach($DB->get_records('user_info_field') as $params){ | ||
| $userfieldoptions[$params->shortname] = $params->name; | ||
| } | ||
| $settings->add(new admin_setting_configselect('equella_userfield', ecs('userfield.title'), ecs('userfield.desc'), 'default', $userfieldoptions)); |
There was a problem hiding this comment.
As above, why not use Username as the default option ?
| @@ -70,6 +70,10 @@ | |||
| 1. Use "selectOrAdd" for EQUELLA 6.0 and older, for EQUELLA 6.1 onward please use "structured" | |||
| 2. There should not be a ? or a & at the start or end of the string.'; | |||
|
|
|||
| $string['config.userfield.title'] = 'User field to use'; | |||
| $string['config.userfield.desc'] = 'Choose the user field to be used instead of username'; | |||
There was a problem hiding this comment.
| $string['config.userfield.desc'] = 'Choose the user field to be used instead of username'; | |
| $string['config.userfield.desc'] = 'Choose which User field to be used for SSO identification'; |
| @@ -70,6 +70,10 @@ | |||
| 1. Use "selectOrAdd" for EQUELLA 6.0 and older, for EQUELLA 6.1 onward please use "structured" | |||
| 2. There should not be a ? or a & at the start or end of the string.'; | |||
|
|
|||
| $string['config.userfield.title'] = 'User field to use'; | |||
There was a problem hiding this comment.
| $string['config.userfield.title'] = 'User field to use'; | |
| $string['config.userfield.title'] = 'User field for SSO identification'; |
| foreach($DB->get_records('user_info_field') as $params){ | ||
| $userfieldoptions[$params->shortname] = $params->name; | ||
| } | ||
| $settings->add(new admin_setting_configselect('equella_userfield', ecs('userfield.title'), ecs('userfield.desc'), 'default', $userfieldoptions)); |
There was a problem hiding this comment.
I am not sure if this setting should be under General settings which is more focused on oEQ.
I think it's good to have a separate section named SSO identification at the bottom of this page.
| @@ -15,7 +15,7 @@ | |||
| // along with Moodle. If not, see <http://www.gnu.org/licenses/>. | |||
| defined('MOODLE_INTERNAL') || die(); | |||
|
|
|||
| $plugin->version = 2023070300; | |||
| $plugin->version = 2023070300.3; | |||
There was a problem hiding this comment.
We typically do not have decimal in the version. It can be the date when you made the commit.
| @@ -85,15 +85,15 @@ function equella_getssotoken($course = null) { | |||
| // see if the user has a role that is linked to an equella role | |||
| $shareid = $CFG->{"equella_{$role->shortname}_shareid"}; | |||
| if (!empty($shareid)) { | |||
| return equella_getssotoken_raw($USER->username, $shareid, $CFG->{"equella_{$role->shortname}_sharedsecret"}); | |||
| return equella_getssotoken_raw($USER->equellauser, $shareid, $CFG->{"equella_{$role->shortname}_sharedsecret"}); | |||
There was a problem hiding this comment.
Can you do $CFG -> equellauser ? I just compared this one with other settings and they are all access from $CFG.
| /** | ||
| * Select the corresponding field to allocate the username to $USER | ||
| */ | ||
| function mod_equella_after_config(){ |
There was a problem hiding this comment.
I am not sure if you need this callback. The configured SSO identification field is available in $CFG->equella_userfield.
Checklist
Description of change
New settings added to choose either the username (Default) or any other field listed in the user_info_field table
#94