Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong way to generate user name if AD id is empty #12

Open
andriyun opened this issue May 3, 2018 · 3 comments
Open

Wrong way to generate user name if AD id is empty #12

andriyun opened this issue May 3, 2018 · 3 comments
Labels
bug Something isn't working

Comments

@andriyun
Copy link
Contributor

andriyun commented May 3, 2018

During the user import process it's possible that AD id is empty
We can have it in employee_id as activation method mode.
With enabled os2intra_generate_username checkbox (see settings form) the code generate new ad_id in line

$mapped_row['ad_id'] = os2intra_user_import_generate_username($mapped_row);

and pass it to user name afterwards
$fields['name'] = $user['ad_id'];

Users generated in this way also got wrong authorization credentials

} elseif (!empty($user['ad_id'])) {

Proposed solution

For employee_id as activation method mode:

Replace generation of new AD id to drupal user name

For ad_id as activation method mode:

Skip user from import scope with proper log message

@andriyun andriyun added the bug Something isn't working label May 3, 2018
@andriyun
Copy link
Contributor Author

andriyun commented May 3, 2018

@stanbellcom, @skifter FYI
@juuliabellcom do you have something to add?

@juuliabellcom
Copy link
Contributor

@andriyun About your fix
Maybe instead of/additionally to this line https://github.com/bellcom/os2intra_user_import/blob/fix-user_name-generation/os2intra_user_import.module#L135

to add
os2intra_user_import_save_log("User activation mode is not setup. Please check user import configuration /admin/config/os2intra/user_import/settings"); return;
It will save error into log and stop import.

Also some comment about the proposed solution
how we will check for user duplictaes for employee_id as activation method mode. I mean these lines

if (array_search($user['ad_id'], $duplicate_ids) !== false) {

will use `$user["employee_id"] to search user duplicats?

And this line

$fields['name'] = $user['ad_id'];
. What we use instead that? $fields['name'] = $user['name']; ? and in case of ad_id activation method $user['name']= $user['ad_id'];

@andriyun
Copy link
Contributor Author

andriyun commented May 4, 2018

os2intra_user_import_save_log("User activation mode is not setup. Please check user import configuration /admin/config/os2intra/user_import/settings"); return;

Added

will use `$user["employee_id"] to search user duplicats?

Added additional check

What we use instead that? $fields['name'] = $user['name']; ? and in case of ad_id activation method $user['name']= $user['ad_id'];

See PR #13

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants