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.

Sign up for GitHub

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

Jump to bottom

Add check for required fields in BO > Country > Address format #15428

Merged
merged 5 commits into from Oct 14, 2019
Merged

Add check for required fields in BO > Country > Address format #15428

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a7b7408
add check for mandatory fields in address format
matthieu-rolland Sep 5, 2019
513df5c
Add tab name detection to improve error message
matthieu-rolland Sep 30, 2019
f9632ac
change wording above address format form
matthieu-rolland Sep 30, 2019
c525225
change error message
matthieu-rolland Oct 1, 2019
449162e
Add exception
matthieu-rolland Oct 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion admin-dev/themes/default/template/controllers/countries/helpers/form/form.tpl
View file
Open in desktop
Expand Up @@ -32,7 +32,7 @@
<textarea id="ordered_fields" name="address_layout" style="height:150px;">{$input.address_layout}</textarea>
</div>
<div class="col-lg-8">
{l s='Required fields for the address (click for more details):' d='Admin.International.Feature'}
{l s='Some countries require different elements than others. Click on the button below to get the valid default address format for this country.' d='Admin.International.Feature'}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{l s='Some countries require different elements than others. Click on the button below to get the valid default address format for this country.' d='Admin.International.Feature'}
{l s='Some countries require different elements than others. Click the button below to get the valid default address format for this country.' d='Admin.International.Feature'}

{$input.display_valid_fields}
</div>
</div>
Expand Down
44 changes: 44 additions & 0 deletions classes/AddressFormat.php
View file
Open in desktop
Expand Up @@ -236,11 +236,55 @@ public function checkFormatFields()
}
}
}
$this->checkRequiredFields($usedKeyList);
}

return (count($this->_errorFormatList)) ? false : true;
}

/**
* Checks that all required fields exist in a given fields list.
* Fills _errorFormatList array in case of absence of a required field.
*
* @param array $fieldList
*/
protected function checkRequiredFields($fieldList)
{
foreach (self::getFieldsRequired() as $requiredField) {
if (!in_array($requiredField, $fieldList)) {
$this->_errorFormatList[] = $this->trans(
'The %s field (in tab %s) is required.',
array($requiredField, $this->getFieldTabName($requiredField)),
'Admin.Notifications.Error');
matthieu-rolland marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

/**
* Given a field name, get the name of the tab in which the field name can be found.
* For ex: Country:name => the tab is 'Country'.
* There should be only one separator in the string, otherwise return false.
*
* @param string $field
*
* @return bool|string
*/
private function getFieldTabName($field)
{
if (strpos($field, ':') === false) {
// When there is no ':' separator, the field is in the Address tab
return 'Address';
}

$fieldTab = explode(':', $field);
if (count($fieldTab) === 2) {
// The part preceding the ':' separator is the name of the tab in which there is the required field
return $fieldTab[0];
}

return false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jolelievre @PierreRambaud

I have doubts on how to deal with this (when this function returns false).

"Normally" it can never happen, but I wanted the method to be abnormal-proof. Do you think I should throw some kind of exception, in the legacy?

Copy link
Member

Choose a reason for hiding this comment

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

As a general rule, methods should do what they are expected to do, or throw an exception otherwise. Return values should only reflect "correct" and "expected" states in normal execution, all other situations should be considered exceptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your input @eternoendless 👍

}

/**
* Returns the error list.
*/
Expand Down