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

Enh: Check User Birthday field #89

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

ArchBlood
Copy link
Contributor

@ArchBlood ArchBlood commented Sep 24, 2024

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

@ArchBlood ArchBlood marked this pull request as draft September 24, 2024 00:36
@ArchBlood ArchBlood marked this pull request as ready for review September 24, 2024 01:49
@ArchBlood
Copy link
Contributor Author

@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.

@luke- luke- requested a review from yurabakhtin September 24, 2024 08:09
@luke-
Copy link
Collaborator

luke- commented Sep 24, 2024

@ArchBlood Thanks!

@yurabakhtin Can you please review?

@yurabakhtin
Copy link
Contributor

@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 You must be at least {age} years old. is printed there, and then I see a fast redirect to the login page without any messages, and if I enter my admin login/pass I see this:

admin-disabled

@ArchBlood
Copy link
Contributor Author

@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 You must be at least {age} years old. is printed there, and then I see a fast redirect to the login page without any messages, and if I enter my admin login/pass I see this:

admin-disabled

I'll look into this as soon as possible.

@ArchBlood
Copy link
Contributor Author

@yurabakhtin can you review again?

validators/AgeValidator.php Outdated Show resolved Hide resolved
validators/AgeValidator.php Outdated Show resolved Hide resolved
@ArchBlood
Copy link
Contributor Author

@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)
Copy link
Contributor Author

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');
        }
    }
}

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Collaborator

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?

Copy link
Contributor Author

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. 🤔

https://github.com/yiisoft/yii2/blob/3c75ff1043cdfc3c0c78ad8a4b477b5894223a5a/framework/base/Model.php#L375-L382

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.

Copy link
Contributor Author

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 ...
],

Copy link
Collaborator

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?

Copy link
Contributor Author

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)
Copy link
Collaborator

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?

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.

3 participants