-
Notifications
You must be signed in to change notification settings - Fork 8
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
Enh: Check User Birthday field #89
base: master
Are you sure you want to change the base?
Conversation
@luke- not sure if I did the tests correctly, but I can say that it does work correctly as long as the two fields are required at registration. |
@ArchBlood Thanks! @yurabakhtin Can you please review? |
@luke- @ArchBlood The validator works but I think we should skip the restriction for admin users, because when I save an admin account form with a date less the min age I see the submitted form is blinked for 1 second so I even can't read the error under field, I guess the error |
I'll look into this as soon as possible. |
@yurabakhtin can you review again? |
@luke- @yurabakhtin for this function to truly work as it should, would it be good to have a way that this force enables Required and Show at registration for the Birthday field or have an info box as a reminder of some kind? As I'm not sure that this will work as it should in this case and it would more than likely get reported as a bug instead of by design, although I'm not exactly sure how I'd force enable these two checkboxes. |
@@ -283,6 +283,17 @@ public static function onAccountSettingsMenuInit($event) | |||
} | |||
} | |||
|
|||
public static function onBeforeValidate($event) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luke- @yurabakhtin your thoughts on using the following?
How this version works is if the checkbox Show age verification and the Minimum age field aren't set it will uncheck the required checkboxes in the Birthday field otherwise when the checkbox and field in the Legal module are set it enables these requirements. If this solution isn't like then we'd have to find a way to have a conditional migration which currently I don't have the time to build.
public static function onBeforeValidate($event)
{
$registrationForm = $event->sender;
$minimumAge = Yii::$app->getModule('legal')->getMinimumAge();
// Find the ProfileField for 'birthday'
$profileField = \humhub\modules\user\models\ProfileField::findOne(['internal_name' => 'birthday']);
if ($profileField !== null) {
// Handle when minimum age is set and valid
if ($minimumAge > 0) {
// If minimum age is set, force "Required" and "Show at registration" checkboxes
$profileField->required = 1;
$profileField->show_at_registration = 1;
// Validate the minimum age for the birthday field
$ageValidator = new validators\AgeValidator(['minimumAge' => $minimumAge]);
$ageValidator->validateAttribute($registrationForm, 'birthday');
}
// Handle when minimum age is 0 or negative
if ($minimumAge <= 0) {
// If minimum age is 0 or negative, uncheck the "Required" and "Show at registration" checkboxes
$profileField->required = 0;
$profileField->show_at_registration = 0;
}
// Save the changes to the ProfileField
if (!$profileField->save(false)) {
Yii::error('Failed to update profile field settings for birthday.', 'legal');
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ArchBlood I agree that before apply the AgeValidator
we should check the profile field birthday
exists in DB.
But I don't agree that we should update the profile field birthday
on the event Profile::EVENT_BEFORE_VALIDATE
. I mean it is not a correct place for doing such changes in the profile field, because the event is called when any user updates own profile.
If we really need to do the changes then it would be better to do this at the time when the legal module form is updated, i.e. when the settings "Show age verification X" and "Minimum age" are updated.
But I am not sure that the Legal module should do it, because it looks like a silent updating that may be unexpected for admin. If the updating will be noticed somehow for admin with some warning message, probably under the settings on the legal module config form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yurabakhtin I'm not sure how we should approach this to be honest that is why I suggested the above option, if you have any ideas on the approach we should take I'm all eyes and ears, otherwise I'm left scratching my head on this one, as without the two checkboxes within the Birthday field being checked this P/R won't work and we'd have to start from scratch.
@luke- any ideas on this topic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I have no idea.
But couldn't we just inject an AgeValidator when the registration is displayed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I have no idea.
But couldn't we just inject an AgeValidator when the registration is displayed?
I'm not exactly sure here, as I'm reviewing the code EVENT_BEFORE_VALIDATE
doesn't allow for this, or not to the extent that we need it to or want it to. 🤔
Maybe what we should think about using is EVENT_BEFORE_INSERT
? But again this wouldn't really match up with a validation class as far as I'm seeing it.
https://github.com/yiisoft/yii2/blob/3c75ff1043cdfc3c0c78ad8a4b477b5894223a5a/framework/db/BaseActiveRecord.php#L957-L980
Another option would be updating HumHub's ActiveRecord class with a custom validation event, I already see a EVENT_APPEND_RULES
being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this way we'd do as following;
Events Example
public static function onAppendRegistrationRules($event)
{
/** @var ActiveRecord $model */
$model = $event->sender;
$minimumAge = Yii::$app->getModule('legal')->getMinimumAge();
if ($minimumAge > 0) {
$model->addRule('birthday', AgeValidator::class, ['minimumAge' => $minimumAge]);
}
}
config.php Example
'events' => [
[
'class' => Registration::class,
'event' => ActiveRecord::EVENT_APPEND_RULES,
'callback' => [Events::class, 'onAppendRegistrationRules']
],
// ... other event handlers ...
],
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, seems there is no way to $model->addRule()
. So the current approach is ok.
Is there anything not working as expected? Maybe we can make this validation optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than what @yurabakhtin has pointed out I see no issues, as for optional currently it should be only triggered when Yii::$app->getModule('legal')->getMinimumAge()
is both set and enabled unless I am misunderstanding and you wish for an additional option?
@@ -283,6 +283,17 @@ public static function onAccountSettingsMenuInit($event) | |||
} | |||
} | |||
|
|||
public static function onBeforeValidate($event) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, seems there is no way to $model->addRule()
. So the current approach is ok.
Is there anything not working as expected? Maybe we can make this validation optional?
Currently it works as long as the profile field "Birthday" is on the registration page and is required. I've not looked deeper into making the function force these checkboxes to be set to enabled when the option is enabled.
Alternative to #67