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

Prevent user import if activation method is not selected #13

Merged
merged 5 commits into from
Aug 8, 2018

Conversation

andriyun
Copy link
Contributor

@andriyun andriyun commented May 3, 2018

Fixes related to #12

Copy link
Contributor

@juuliabellcom juuliabellcom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, tested on 3d installations: faxe, fredericia, odsherred

@andriyun andriyun mentioned this pull request May 9, 2018
@@ -103,8 +103,8 @@ function os2intra_user_import_activate_form_validate($form, &$form_state) {
}

// Check if it matches the users birthday field
$birthday_field = field_get_items('user', $user, 'field_os2intra_birthdate');
if ($input_birthday !== $birthday_field[0]['value']) {
$birthday_field = field_get_items('user', $user, 'field_os2intra_birthday');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andriyun
odsherred has this field called as field_os2intra_birthday, but fk and faxe have it as field_os2intra_birthdate.

which would mean that after this change it will only work on odsherred.
Is that the desired change?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i can see we already have a mapping for that field in the settings with they key 'os2intra_user_import_birthday_field', how about using that instead of hardcoding the field?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!
I'll fix fix

@andriyun
Copy link
Contributor Author

@stanbellcom fixed
Could you look at it again?
Thanks

@dsbellcom dsbellcom merged commit 52af9c8 into master Aug 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants