Skip to content

Admin Order - Email is Now Required - Magento 2.3 #22648

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

Closed
wants to merge 3 commits into from
Closed

Admin Order - Email is Now Required - Magento 2.3 #22648

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7e0be95
Admin Order - Email is Now Required - Magento 2.3
solwininfotech May 4, 2019
1243b8a
Added Email Required Setting In Admin
solwininfotech May 8, 2019
b9e7d1d
Removed ScopeInterface
solwininfotech May 8, 2019
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
5 changes: 5 additions & 0 deletions app/code/Magento/Customer/etc/adminhtml/system.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,11 @@
<comment>To show VAT number on Storefront, set Show VAT Number on Storefront option to Yes.</comment>
<source_model>Magento\Config\Model\Config\Source\Yesno</source_model>
</field>
<field id="email_required_create_order" translate="label comment" type="select" sortOrder="58" showInDefault="1" showInWebsite="1" showInStore="1" canRestore="1">
<label>Email is required field for Admin order creation</label>
<comment>If set YES Email field will be required during Admin order creation for new Customer.</comment>
<source_model>Magento\Config\Model\Config\Source\Yesno</source_model>
</field>
<field id="email_domain" translate="label" type="text" sortOrder="60" showInDefault="1" showInWebsite="1" showInStore="1" canRestore="1">
<label>Default Email Domain</label>
</field>
Expand Down
1 change: 1 addition & 0 deletions app/code/Magento/Customer/etc/config.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
<confirm>0</confirm>
<default_group>1</default_group>
<tax_calculation_address_type>billing</tax_calculation_address_type>
<email_required_create_order>0</email_required_create_order>
<email_domain>example.com</email_domain>
<email_identity>general</email_identity>
<email_template>customer_create_account_email_template</email_template>
Expand Down
17 changes: 16 additions & 1 deletion app/code/Magento/Sales/Block/Adminhtml/Order/Create/Form/Account.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ class Account extends AbstractForm
*/
protected $_extensibleDataObjectConverter;

const XML_PATH_EMAIL_REQUIRED_CREATE_ORDER = 'customer/create_account/email_required_create_order';
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, make constant private

Copy link
Contributor

@driskell driskell Aug 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Needs a PHPDOC too


/**
* @param \Magento\Backend\Block\Template\Context $context
* @param \Magento\Backend\Model\Session\Quote $sessionQuote
Expand Down Expand Up @@ -147,7 +149,7 @@ protected function _addAdditionalFormElementData(AbstractElement $element)
{
switch ($element->getId()) {
case 'email':
$element->setRequired(1);
$element->setRequired($this->isEmailRequiredCreateOrder());
$element->setClass('validate-email admin__control-text');
break;
}
Expand Down Expand Up @@ -204,4 +206,17 @@ private function extractValuesFromAttributes(array $attributes): array

return $formValues;
}

/**
* Retrieve email is required field for admin order creation
*
* @return bool
*/
public function isEmailRequiredCreateOrder()
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, make method private

{
return $this->_scopeConfig->getValue(
self::XML_PATH_EMAIL_REQUIRED_CREATE_ORDER,
\Magento\Store\Model\ScopeInterface::SCOPE_STORE
);
}
}
26 changes: 25 additions & 1 deletion app/code/Magento/Sales/Model/AdminOrder/Create.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ class Create extends \Magento\Framework\DataObject implements \Magento\Checkout\
*/
const XML_PATH_DEFAULT_EMAIL_DOMAIN = 'customer/create_account/email_domain';

const XML_PATH_EMAIL_REQUIRED_CREATE_ORDER = 'customer/create_account/email_required_create_order';
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, make constant private

Copy link
Contributor

Choose a reason for hiding this comment

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

Judging by the current code not obeying such practice I would suggest that change be made in a separate PR aimed at resolving this in the entire class.

Copy link
Contributor

Choose a reason for hiding this comment

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

Having said that - I notice the PHPDOC is missing for this constant.

Copy link
Contributor

Choose a reason for hiding this comment

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

@driskell we can't make existing constants as private due to backward compatibility, but the new one should be private.

/**
* Quote session object
*
Expand Down Expand Up @@ -2029,7 +2030,30 @@ protected function _validate()
*/
protected function _getNewCustomerEmail()
{
return $this->getData('account/email');
$email_required = $this->_scopeConfig->getValue(
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, avoid underscore in the variable naming

Copy link
Contributor

Choose a reason for hiding this comment

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

As above, this isn’t obeyed by the remaining code and likely require refactor of unrelated code, and so would increase scope of the PR. I’d suggest to pick this up in a separate PR that resolves this for all properties in the class.

Copy link
Contributor

Choose a reason for hiding this comment

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

@driskell it's not a big deal to rename variables in the modern IDE, it requires an additional commit to make code style better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could it be a problem that it is protected and not private? I worry further change here might delay the PR merge due to unrelated opportunistic change. As it stands I’m happy to help with the other changes but this one feels like it needs a little more thought and a separately considered and reversible changeset so this feature isn’t lost should an issue be raised.

self::XML_PATH_EMAIL_REQUIRED_CREATE_ORDER,
\Magento\Store\Model\ScopeInterface::SCOPE_STORE
);

if($email_required) {
return $this->getData('account/email');
} else {
$email = $this->getData('account/email');
if (empty($email)) {
$host = $this->_scopeConfig->getValue(
self::XML_PATH_DEFAULT_EMAIL_DOMAIN,
\Magento\Store\Model\ScopeInterface::SCOPE_STORE
);
$account = time();
$email = $account . '@' . $host;
$account = $this->getData('account');
$account['email'] = $email;
$this->setData('account', $account);
}

return $email;
}

}

/**
Expand Down