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

[release-10.1] Fix form validation #4227

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions module/VuFind/src/VuFindTest/Integration/MinkTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -829,6 +829,40 @@ protected function hasElementsMatchingText(Element $page, $selector, $text)
return false;
}

/**
* Check that a field content is valid (does not have the :invalid pseudo class).
*
* @param Element $page Page element (not currently used)
* @param string $selector CSS selector
*
* @return void
*/
protected function checkFieldIsValid(Element $page, string $selector): void
{
$session = $this->getMinkSession();
$session->wait(
$this->getDefaultTimeout(),
"document.querySelector('$selector:invalid') === null"
);
}

/**
* Check that a field content is invalid (has the :invalid pseudo class).
*
* @param Element $page Page element (not currently used)
* @param string $selector CSS selector
*
* @return void
*/
protected function checkFieldIsInvalid(Element $page, string $selector): void
{
$session = $this->getMinkSession();
$session->wait(
$this->getDefaultTimeout(),
"document.querySelector('$selector:invalid') !== null"
);
}

/**
* Wait for a callback to return the expected value
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -702,32 +702,30 @@ public function testSMS(): void
// - too empty
$this->findCssAndSetValue($page, '.modal #sms_to', '');
$this->clickCss($page, '.modal-body .btn.btn-primary');
$this->findCss($page, '.modal .sms-error');
$this->checkFieldIsInvalid($page, '.modal #sms_to');
// - too short
$this->findCssAndSetValue($page, '.modal #sms_to', '123');
$this->clickCss($page, '.modal-body .btn.btn-primary');
$this->findCss($page, '.modal .sms-error');
$this->checkFieldIsInvalid($page, '.modal #sms_to');
// - too long
$this->findCssAndSetValue($page, '.modal #sms_to', '12345678912345678912345679');
$this->clickCss($page, '.modal-body .btn.btn-primary');
$this->findCss($page, '.modal .sms-error');
$this->checkFieldIsInvalid($page, '.modal #sms_to');
// - too lettery
$this->findCssAndSetValue($page, '.modal #sms_to', '123abc');
$this->clickCss($page, '.modal-body .btn.btn-primary');
$this->findCss($page, '.modal .sms-error');
$this->checkFieldIsInvalid($page, '.modal #sms_to');
// - just right
$this->findCssAndSetValue($page, '.modal #sms_to', '8005555555');
$this->clickCss($page, '.modal-body .btn.btn-primary');
$this->waitForPageLoad($page); // wait for form submission to catch missing carrier
$this->assertNull($page->find('css', '.modal .sms-error'));
$this->checkFieldIsValid($page, '.modal #sms_to');

$this->unFindCss($page, '.modal .sms-error');
// - pretty just right
$this->findCssAndSetValue($page, '.modal #sms_to', '(800) 555-5555');
$this->clickCss($page, '.modal-body .btn.btn-primary');
$this->waitForPageLoad($page); // wait for form submission to catch missing carrier
$this->assertNull($page->find('css', '.modal .sms-error'));
$this->unFindCss($page, '.modal .sms-error');
$this->checkFieldIsValid($page, '.modal #sms_to');
// Send text to false number
$this->findCssAndSetValue($page, '.modal #sms_to', '(800) 555-5555');
$this->findCss($page, '.modal #sms_provider option');
Expand Down
6 changes: 3 additions & 3 deletions themes/bootprint3/css/compiled.css

Large diffs are not rendered by default.

6 changes: 3 additions & 3 deletions themes/bootstrap3/css/compiled.css

Large diffs are not rendered by default.

13 changes: 8 additions & 5 deletions themes/bootstrap3/js/common.js
Original file line number Diff line number Diff line change
Expand Up @@ -726,7 +726,12 @@ function getUrlRoot(url) {
return urlroot;
}

// Phone number validation
/**
* Phone number validation
* @param {String} numID Phone number field ID
* @param {String} regionCode Region code
* @deprecated See validation.js for replacement
*/
function phoneNumberFormHandler(numID, regionCode) {
var phoneInput = document.getElementById(numID);
var number = phoneInput.value;
Expand All @@ -737,12 +742,10 @@ function phoneNumberFormHandler(numID, regionCode) {
} else {
valid = VuFind.translate('libphonenumber_invalid');
}
$(phoneInput).siblings('.help-block.with-errors').html(valid);
$(phoneInput).closest('.form-group').addClass('sms-error');
phoneInput.setCustomValidity(valid);
demiankatz marked this conversation as resolved.
Show resolved Hide resolved
return false;
} else {
$(phoneInput).closest('.form-group').removeClass('sms-error');
$(phoneInput).siblings('.help-block.with-errors').html('');
phoneInput.setCustomValidity('');
}
}

Expand Down
65 changes: 65 additions & 0 deletions themes/bootstrap3/js/validation.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/* global VuFind, isPhoneNumberValid */

VuFind.register('validation', function Validation() {
/**
* Check field match validity
* @param {Event} ev Event
* @returns {boolean} Validity
*/
function checkMatchValidity(ev) {
const field = ev.target;
const matchSelector = field.dataset.match;
if (!matchSelector) {
return true;
}
const matchEl = field.form.querySelector(matchSelector);
if (matchEl.value !== field.value) {
field.setCustomValidity(field.dataset.matchError || '');
return false;
}
field.setCustomValidity('');
return true;
}

/**
* Check field phone number validity
* @param {Event} ev Event
* @returns {boolean} Validity
*/
function checkPhoneNumberValidity(ev) {
const field = ev.target;
if (field.id && field.type === 'tel' && field.dataset.validatorRegion) {
const valid = isPhoneNumberValid(field.value, field.dataset.validatorRegion);
Copy link
Member

Choose a reason for hiding this comment

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

If I'm following things correctly, I believe that isPhoneNumberValid is being exported by libphonenumber. It looks to me like we're using a version of libphonenumber from 2010 or so, which is probably outdated and inaccurate. It's out of scope for the current PR, but maybe when we update this for the 11.0 release, we should also introduce loading of libphonenumber via NPM (possibly using the lighter-weight libphonenumber-js) and make sure things work with the latest version. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That all would make sense, but I'd handle it separately. Not sure if libphonenumber-js supports usage without a module system, but any update would probably be better than the current one.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, definitely a separate issue, but worth doing before we forget about it. I can open a JIRA ticket if you think that would be the best next step.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it would even make sense to evaluate all of our JS dependencies that are not currently installed via NPM and open tickets to update/replace all of them.

Copy link
Contributor Author

@EreMaijala EreMaijala Feb 3, 2025

Choose a reason for hiding this comment

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

Sounds good, please open a ticket about it!

Copy link
Member

Choose a reason for hiding this comment

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

Done: VUFIND-1751

if (true !== valid) {
field.setCustomValidity(typeof valid === 'string' ? valid : VuFind.translate('libphonenumber_invalid'));
return false;
}
}
field.setCustomValidity('');
return true;
}

/**
* Check field validity
* @param {Event} ev Event
*/
function checkValidity(ev) {
const checks = [checkMatchValidity, checkPhoneNumberValidity];
for (const check of checks) {
if (!check(ev)) {
return;
}
}
}

/**
* Init custom form validation
*/
function init() {
document.addEventListener('input', checkValidity);
}

return {
init: init
};
});
9 changes: 0 additions & 9 deletions themes/bootstrap3/js/vendor/validator.min.js

This file was deleted.

16 changes: 0 additions & 16 deletions themes/bootstrap3/less/bootstrap.less
Original file line number Diff line number Diff line change
Expand Up @@ -225,22 +225,6 @@ footer {
.flex-col { flex: 0 1 100%; }
.flex-none { flex: none; }

/* ------ Form Errors ------ */
.has-error {margin-bottom: 0;}
.sms-error { &:extend(.has-error); }
.help-block.with-errors {
margin: 0;
&:first-child {
padding-top: @padding-base-vertical;
}
padding-bottom: @padding-base-vertical;
&:empty {padding: 0;}

ul.list-unstyled {
margin: 0;
}
}

/* ------ Admin ------ */
.form-admin-maintenance .form-control {
display: inline-block;
Expand Down
16 changes: 0 additions & 16 deletions themes/bootstrap3/scss/bootstrap.scss
Original file line number Diff line number Diff line change
Expand Up @@ -225,22 +225,6 @@ footer {
.flex-col { flex: 0 1 100%; }
.flex-none { flex: none; }

/* ------ Form Errors ------ */
.has-error {margin-bottom: 0;}
.sms-error { @extend .has-error; }
.help-block.with-errors {
margin: 0;
&:first-child {
padding-top: $padding-base-vertical;
}
padding-bottom: $padding-base-vertical;
&:empty {padding: 0;}

ul.list-unstyled {
margin: 0;
}
}

/* ------ Admin ------ */
.form-admin-maintenance .form-control {
display: inline-block;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
<div class="form-group">
<label class="control-label" for="oldpwd"><?=$this->transEsc('old_password') ?>:</label>
<input type="password" name="oldpwd" id="oldpwd" class="form-control" autocomplete="current-password">
<div class="help-block with-errors"></div>
</div>
<?php endif; ?>
<div class="form-group">
Expand All @@ -21,10 +20,8 @@
<?php if ($hint = $this->passwordPolicy['hint'] ?? null): ?>
<div class="help-block"><?=$this->transEsc($hint) ?></div>
<?php endif; ?>
<div class="help-block with-errors"></div>
</div>
<div class="form-group">
<label class="control-label" for="password2"><?=$this->transEsc('confirm_new_password') ?>:</label>
<input type="password" name="password2" id="password2" class="form-control" required aria-required="true" data-match="#password" data-match-error="<?=$this->transEscAttr('Passwords do not match')?>" autocomplete="new-password">
<div class="help-block with-errors"></div>
</div>
4 changes: 0 additions & 4 deletions themes/bootstrap3/templates/Auth/Database/create.phtml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
<div class="form-group">
<label class="control-label" for="account_email"><?=$this->transEsc('Email Address')?>:</label>
<input id="account_email" type="email" name="email" required aria-required="true" value="<?=$this->escapeHtmlAttr($this->request->get('email'))?>" class="form-control" autocomplete="email">
<div class="help-block with-errors"></div>
</div>
<div class="form-group">
<label class="control-label" for="account_username"><?=$this->transEsc('Desired Username')?>:</label>
Expand All @@ -21,7 +20,6 @@
<?php if ($this->usernamePolicy['hint']): ?>
<div class="help-block"><?=$this->transEsc($this->usernamePolicy['hint']) ?></div>
<?php endif; ?>
<div class="help-block with-errors"></div>
</div>
<div class="form-group">
<label class="control-label" for="account_password"><?=$this->transEsc('Password')?>:</label>
Expand All @@ -33,10 +31,8 @@
<?php if ($this->passwordPolicy['hint']): ?>
<div class="help-block"><?=$this->transEsc($this->passwordPolicy['hint']) ?></div>
<?php endif; ?>
<div class="help-block with-errors"></div>
</div>
<div class="form-group">
<label class="control-label" for="account_password2"><?=$this->transEsc('Password Again')?>:</label>
<input id="account_password2" type="password" name="password2" required class="form-control" data-match="#account_password" data-match-error="<?=$this->transEscAttr('Passwords do not match')?>" autocomplete="new-password">
<div class="help-block with-errors"></div>
</div>
2 changes: 1 addition & 1 deletion themes/bootstrap3/templates/myresearch/account.phtml
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
<h2><?=$this->transEsc('User Account')?></h2>
<?=$this->flashmessages()?>

<form method="post" name="accountForm" id="accountForm" class="form-user-create" data-toggle="validator">
<form method="post" name="accountForm" id="accountForm" class="form-user-create">
<?=$this->auth()->getCreateFields()?>
<?=$this->captcha()->html($this->useCaptcha) ?>
<div class="form-group">
Expand Down
2 changes: 1 addition & 1 deletion themes/bootstrap3/templates/myresearch/changeemail.phtml
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
<?php if (!$this->auth()->getManager()->supportsEmailChange($this->auth_method)): ?>
<div class="error"><?=$this->transEsc('change_email_disabled') ?></div>
<?php else: ?>
<form id="newemail" class="form-new-email" action="<?=$this->url('myresearch-changeemail') ?>" method="post" data-toggle="validator">
<form id="newemail" class="form-new-email" action="<?=$this->url('myresearch-changeemail') ?>" method="post">
<input type="hidden" value="<?=$this->escapeHtmlAttr($this->auth()->getManager()->getCsrfHash())?>" name="csrf">
<div class="form-group">
<label class="control-label"><?=$this->transEsc('Email Address') ?>:</label>
Expand Down
2 changes: 1 addition & 1 deletion themes/bootstrap3/templates/myresearch/newpassword.phtml
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
<?php elseif (!isset($this->hash)): ?>
<div class="error"><?=$this->transEsc('recovery_user_not_found') ?></div>
<?php else: ?>
<form id="newpassword" class="form-new-password" action="<?=$this->url('myresearch-newpassword') ?>" method="post" data-toggle="validator">
<form id="newpassword" class="form-new-password" action="<?=$this->url('myresearch-newpassword') ?>" method="post">
<input type="hidden" value="<?=$this->escapeHtmlAttr($this->auth()->getManager()->getCsrfHash())?>" name="csrf">
<input type="hidden" value="<?=$this->escapeHtmlAttr($this->hash) ?>" name="hash">
<input type="hidden" value="<?=$this->escapeHtmlAttr($this->username) ?>" name="username">
Expand Down
12 changes: 2 additions & 10 deletions themes/bootstrap3/templates/record/sms.phtml
Original file line number Diff line number Diff line change
Expand Up @@ -8,23 +8,15 @@
. '<li>' . $this->recordLinker()->getBreadcrumbHtml($this->driver) . '</li> '
. '<li class="active">' . $this->transEsc('Text this') . '</li>';
?>
<?php $validatorCallback = <<<JS
function phoneNumberValidation() {
return phoneNumberFormHandler('sms_to', '{$this->validation}');
}
JS;
?>
<?=$this->inlineScript(\Laminas\View\Helper\HeadScript::SCRIPT, $validatorCallback, 'SET'); ?>
<h2><?=$this->transEsc('Text this') ?>: <span class="title-in-heading"><?=$this->escapeHtml($this->driver->getBreadcrumb())?></span></h2>
<form method="post" name="smsRecord" class="form-record-sms"<?php if (isset($this->validation) && !empty($this->validation)):?> data-lightbox-onsubmit="phoneNumberValidation"<?php endif; ?>>
<form method="post" name="smsRecord" class="form-record-sms">
<?=$this->flashmessages()?>
<input type="hidden" name="id" value="<?=$this->escapeHtmlAttr($this->driver->getUniqueId())?>">
<input type="hidden" name="source" value="<?=$this->escapeHtmlAttr($this->driver->getSourceIdentifier())?>">
<input type="hidden" value="<?=$this->escapeHtmlAttr($this->auth()->getManager()->getCsrfHash())?>" name="csrf">
<div class="form-group">
<label class="control-label" for="sms_to"><?=$this->transEsc('Number')?>:</label>
<input id="sms_to" type="tel" name="to" value="<?=$this->escapeHtmlAttr($this->to)?>" placeholder="<?=$this->transEscAttr('sms_phone_number')?>" class="form-control">
<div class="help-block with-errors"></div>
<input id="sms_to" type="tel" name="to" value="<?=$this->escapeHtmlAttr($this->to)?>" placeholder="<?=$this->transEscAttr('sms_phone_number')?>" class="form-control" required<?=!empty($this->validation) ? ' data-validator-region="' . $this->escapeHtmlAttr($this->validation) . '"' : ''?>>
</div>
<?php if (is_array($this->carriers) && count($this->carriers) > 1): ?>
<div class="form-group">
Expand Down
Loading