-
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
Open
ArchBlood
wants to merge
11
commits into
humhub:master
Choose a base branch
from
ArchBlood:patch-1
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+115
−0
Open
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
da8d5ed
Update Events.php
ArchBlood 042648e
Create AgeValidator.php
ArchBlood 7f06858
Update config.php
ArchBlood 7a2d5a2
Update LegalCest.php
ArchBlood 805e323
Update AgeValidator.php
ArchBlood b00d783
Fix: Validation for admins
ArchBlood 3691237
Fix: Removed unused class
ArchBlood 6ac1599
Fix: Remove Disabling Account
ArchBlood 1a4c9fb
Update AgeValidator.php
ArchBlood a00867d
Update CHANGELOG.md
ArchBlood 0d67e07
Merge branch 'master' into patch-1
luke- File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -132,4 +132,48 @@ public function testAgeVerification(AcceptanceTester $I) | |
$I->amUser1(true); | ||
$I->dontSee($title, '.panel-heading'); | ||
} | ||
} | ||
|
||
public function testAgeValidation(AcceptanceTester $I) | ||
{ | ||
$I->wantTo('test age validation during registration and profile update'); | ||
$minimumAge = 18; | ||
|
||
$I->amAdmin(); | ||
$I->amGoingTo('enable age verification'); | ||
$I->enableAgeVerification($minimumAge); | ||
|
||
// Test registration with valid age | ||
$I->amGoingTo('test registration with valid age'); | ||
$I->amOnRoute('/user/registration'); | ||
$I->fillField('Registration[username]', 'validAgeUser'); | ||
$I->fillField('Registration[email]', '[email protected]'); | ||
$I->fillField('Registration[password]', 'ValidPassword123'); | ||
$I->fillField('Registration[birthday]', date('Y-m-d', strtotime("-{$minimumAge} years -1 day"))); | ||
$I->click('Register'); | ||
$I->dontSee('You must be at least ' . $minimumAge . ' years old.'); | ||
|
||
// Test registration with invalid age | ||
$I->amGoingTo('test registration with invalid age'); | ||
$I->amOnRoute('/user/registration'); | ||
$I->fillField('Registration[username]', 'invalidAgeUser'); | ||
$I->fillField('Registration[email]', '[email protected]'); | ||
$I->fillField('Registration[password]', 'InvalidPassword123'); | ||
$I->fillField('Registration[birthday]', date('Y-m-d', strtotime("-{$minimumAge} years +1 day"))); | ||
$I->click('Register'); | ||
$I->see('You must be at least ' . $minimumAge . ' years old.'); | ||
|
||
// Test profile update with invalid age | ||
$I->amGoingTo('test profile update with invalid age'); | ||
$I->amUser1(true); | ||
$I->amOnRoute('/user/account/edit'); | ||
$I->fillField('Profile[birthday]', date('Y-m-d', strtotime("-{$minimumAge} years +1 day"))); | ||
$I->click('Save'); | ||
$I->see('You must be at least ' . $minimumAge . ' years old.'); | ||
|
||
// Test profile update with valid age | ||
$I->amGoingTo('test profile update with valid age'); | ||
$I->fillField('Profile[birthday]', date('Y-m-d', strtotime("-{$minimumAge} years -1 day"))); | ||
$I->click('Save'); | ||
$I->dontSee('You must be at least ' . $minimumAge . ' years old.'); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
<?php | ||
|
||
namespace humhub\modules\legal\validators; | ||
|
||
use DateTime; | ||
use Yii; | ||
use yii\validators\Validator; | ||
use humhub\modules\user\models\User; | ||
|
||
/** | ||
* AgeValidator validates that the given value represents an age greater than or equal to a specified minimum age. | ||
*/ | ||
class AgeValidator extends Validator | ||
{ | ||
/** | ||
* @var int The minimum age required | ||
*/ | ||
public $minimumAge; | ||
|
||
/** | ||
* @inheritdoc | ||
*/ | ||
public function init() | ||
{ | ||
parent::init(); | ||
if ($this->minimumAge === null) { | ||
$this->minimumAge = Yii::$app->getModule('legal')->getMinimumAge(); | ||
} | ||
} | ||
|
||
/** | ||
* Validates the age of the user based on the given attribute value. | ||
* | ||
* @param \yii\base\Model $model the data model being validated | ||
* @param string $attribute the name of the attribute to be validated | ||
*/ | ||
public function validateAttribute($model, $attribute) | ||
{ | ||
$value = $model->$attribute; | ||
if (!$value instanceof DateTime) { | ||
try { | ||
$value = new DateTime($value); | ||
} catch (\Exception $e) { | ||
$this->addError($model, $attribute, Yii::t('LegalModule.base', 'Invalid date format.')); | ||
return; | ||
} | ||
} | ||
|
||
$today = new DateTime(); | ||
$age = $today->diff($value)->y; | ||
|
||
if ($age < $this->minimumAge) { | ||
$message = Yii::t('LegalModule.base', 'You must be at least {age} years old.', ['age' => $this->minimumAge]); | ||
$this->addError($model, $attribute, $message); | ||
} | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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 fieldbirthday
exists in DB.But I don't agree that we should update the profile field
birthday
on the eventProfile::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.
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.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
config.php Example
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?