#7241: Always add empty option for prefix and/or suffix if optional #11462

avstudnitz · 2017-10-14T13:48:54Z

Description

This solves part of #7241. If the prefix and/or suffix field is set to "optional" in the configuration, we automatically add a blank first option if it isn't entered in the "Dropdown Options" configuration field.

Fixed Issues (if relevant)

No option to start with blank option for prefix and suffix in checkout. #7241: No option to start with blank option for prefix and suffix in checkout. (partly fixed)

Manual testing scenarios

Go to Admin panel > Stores > Configuration > Customers > Customer Configuration
Change "Show Prefix" and/or "Show Suffix" to "Optional"
Populate "Prefix Dropdown Options" with : "Ms;Miss;Mrs;Mr;Dr;Atty;Hon;Prof;Pres;VP;MA"
Populate "Suffix Dropdown Options" with : "Jr;Sr;Ph.D.;M.D.;R.N.;Ret."
Click Save.
Switch to the front-end, and add something to your cart.
Go to the checkout page or customer registration page
You'll notice that there is an empty first option for the Prefix and Suffix field.

Contribution checklist

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

dmanners · 2017-10-24T12:45:15Z

app/code/Magento/Customer/Model/Options.php

     * @return array|bool
     */
-    protected function _prepareNamePrefixSuffixOptions($options)
+    private function prepareNamePrefixSuffixOptions($options, $isOptional = false)


As part of the BC guidelines you cannot remove a protected method http://devdocs.magento.com/guides/v2.2/contributor-guide/backward-compatible-development/#public-and-protected-method-removal I would suggest that you keep the private method and the internal calls to this private method and then simply add back the protected method that simply calls the private method and mark it as deprecated.

dmanners · 2017-11-10T15:13:56Z

@avstudnitz do you think you will have some time to continue this PR?

avstudnitz · 2017-11-14T09:46:57Z

Yes, I will, probably next week.

This is due to Backwards Compatibility reasons.

avstudnitz · 2017-11-15T10:23:51Z

I re-added the protected method as you said @dmanners.

dmanners · 2017-11-15T10:40:52Z

Thanks @avstudnitz I will take another look over this PR for you.

dmanners · 2017-11-15T13:58:33Z

app/code/Magento/Customer/Model/Options.php

+     * @param bool $isOptional
+     * @return array|bool
+     *
+     * @deprecated See prepareNamePrefixSuffixOptions


You should also be able to use http://docs.phpdoc.org/references/phpdoc/tags/see.html here to link to the new method. Something like the following should work.

@see prepareNamePrefixSuffixOptions()

Changed, makes sense.

dmanners · 2017-11-15T15:46:45Z

app/code/Magento/Config/Model/Config/Source/Nooptreq.php

@@ -11,15 +11,19 @@
 */
 class Nooptreq implements \Magento\Framework\Option\ArrayInterface
 {
+    const VALUE_NO = '';


Sadly while putting this to the 2.2-develop branch and with this class marked as @api we cannot add any new constants. For a PR to 2.3-develop we could add these new constants. For this PR we will have to change this back.

Can you explain why adding new constants is an issue? It doesn't affect backwards compatibility and only improves things, I don't see any disadavantage of that... sounds like a quite arbitrary rule to me, but perhaps it gets clearer if you explain.

Hi @dmanners in general, you are right. But in this particular, we can accept new constants. There are some cases when new constants can break client code, but the chances a very low.
@dmanners @avstudnitz thank you for collaboration

dmanners · 2017-11-15T15:47:11Z

app/code/Magento/Customer/Model/Options.php

-        return $this->_prepareNamePrefixSuffixOptions($this->addressHelper->getConfig('prefix_options', $store));
+        return $this->prepareNamePrefixSuffixOptions(
+            $this->addressHelper->getConfig('prefix_options', $store),
+            $this->addressHelper->getConfig('prefix_show', $store) == NooptreqSource::VALUE_OPTIONAL


Unfortunately we will need to change this since we cannot add the new constants.

dmanners · 2017-11-15T15:47:15Z

app/code/Magento/Customer/Model/Options.php

-        return $this->_prepareNamePrefixSuffixOptions($this->addressHelper->getConfig('suffix_options', $store));
+        return $this->prepareNamePrefixSuffixOptions(
+            $this->addressHelper->getConfig('suffix_options', $store),
+            $this->addressHelper->getConfig('suffix_show', $store) == NooptreqSource::VALUE_OPTIONAL


Unfortunately we will need to change this since we cannot add the new constants.

dmanners · 2017-11-30T13:55:58Z

@avstudnitz have you had anytime to look into this PR further? There is still the part about showing the empty value in the check and also the backwards compatibility changes also. Thanks

See magento#11462 (review)

avstudnitz · 2017-12-01T08:00:29Z

@dmanners I made the requested changes. Would be glad about an explanation of the forbidden constants.
This PR doesn't cover preselecting the empty value in the checkout as I wasn't able to do that (see #7241 (comment)). It still improves UX and makes the core better even if it doesn't solve all of #7241.

dmanners · 2017-12-01T09:08:36Z

@avstudnitz thank you for the update. I will see if I can get someone to help out with the front-end part, it would be nice to have both together.

With regards to the constant addition. I also agree that it is worth having in this case. Generally speaking because constants are public and the class is marked as @api the public face of the class is changing and people need to be able to rely on this. In this case I will talk with the team and see if we can get it approved.

Could you revert the revert commit and then I can ask from some approval here. Sorry for the pain.

See magento#11462 (comment)

avstudnitz · 2017-12-01T09:56:54Z

Thanks @dmanners, I reverted the commit. I agree that removing or changing public elements of the class would be bad, but I don't see the point for newly added functions or constants because they can be ignored. Would be great if you could get approval for that.

dmanners · 2017-12-12T09:05:04Z

@avstudnitz still trying to sort out that front end part for you. I will aim to get around to this PR in the next week. Sorry for the delay.

…fix in checkout. - replace usage of empty string for optional value with space so that the knockout.js picks up the optional values

…x if optional #11462

7241 Always add empty option for prefix and/or suffix if optional

11e38f6

avstudnitz mentioned this pull request Oct 14, 2017

No option to start with blank option for prefix and suffix in checkout. #7241

Closed

dmanners self-assigned this Oct 24, 2017

dmanners added this to the October 2017 milestone Oct 24, 2017

dmanners added Release Line: 2.2 2.2.x bug report Component: Customer Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release labels Oct 24, 2017

dmanners suggested changes Oct 24, 2017

View reviewed changes

magento-team added the Progress: needs update label Oct 24, 2017

okorshenko modified the milestones: October 2017, November 2017 Nov 1, 2017

magento-engcom-team added bugfix Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release labels Nov 7, 2017

okorshenko removed the Reproduced on develop label Nov 7, 2017

7241 Re-add removed protected method and mark it as deprecated

7800e5d

This is due to Backwards Compatibility reasons.

Merge branch '2.2-develop' into 7241

252529f

magento-team removed the Progress: needs update label Nov 15, 2017

dmanners suggested changes Nov 15, 2017

View reviewed changes

magento-team added the Progress: needs update label Nov 15, 2017

okorshenko removed this from the November 2017 milestone Dec 1, 2017

okorshenko added this to the December 2017 milestone Dec 1, 2017

avstudnitz added 2 commits December 1, 2017 08:52

7241 Change syntax of @see annotation for depracated method

519b805

7241 Undo creation of constants due to @api reasons

eb79b38

See magento#11462 (review)

7241 Revert commit eb79b38 and re-add constants

fce3afe

See magento#11462 (comment)

okorshenko removed the 2.2.x label Dec 14, 2017

magento#7241: No option to start with blank option for prefix and suf…

3e38118

…fix in checkout. - replace usage of empty string for optional value with space so that the knockout.js picks up the optional values

magento-team removed the Progress: needs update label Dec 18, 2017

dmanners approved these changes Dec 20, 2017

View reviewed changes

magento-team added the Progress: accept label Dec 20, 2017

magento-engcom-team added the Fixed in 2.2.x The issue has been fixed in 2.2 release line label Dec 22, 2017

magento-team merged commit 3e38118 into magento:2.2-develop Dec 22, 2017

magento-team pushed a commit that referenced this pull request Dec 22, 2017

MAGETWO-82635: #7241: Always add empty option for prefix and/or suffi…

fd567bc

…x if optional #11462

dmanners mentioned this pull request Sep 21, 2018

Customer Name Prefix shows white space when extra separator is addes.… #18077

Closed

4 tasks

#7241: Always add empty option for prefix and/or suffix if optional #11462

#7241: Always add empty option for prefix and/or suffix if optional #11462

Uh oh!

Conversation

avstudnitz commented Oct 14, 2017 • edited by LexKoomen Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Fixed Issues (if relevant)

Manual testing scenarios

Contribution checklist

Uh oh!

dmanners Oct 24, 2017

Choose a reason for hiding this comment

Uh oh!

dmanners commented Nov 10, 2017

Uh oh!

avstudnitz commented Nov 14, 2017

Uh oh!

avstudnitz commented Nov 15, 2017

Uh oh!

dmanners commented Nov 15, 2017

Uh oh!

dmanners Nov 15, 2017

Choose a reason for hiding this comment

Uh oh!

avstudnitz Dec 1, 2017

Choose a reason for hiding this comment

Uh oh!

dmanners Nov 15, 2017

Choose a reason for hiding this comment

Uh oh!

avstudnitz Dec 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

okorshenko Dec 20, 2017

Choose a reason for hiding this comment

Uh oh!

dmanners Nov 15, 2017

Choose a reason for hiding this comment

Uh oh!

dmanners Nov 15, 2017

Choose a reason for hiding this comment

Uh oh!

dmanners commented Nov 30, 2017

Uh oh!

avstudnitz commented Dec 1, 2017

Uh oh!

dmanners commented Dec 1, 2017

Uh oh!

avstudnitz commented Dec 1, 2017

Uh oh!

dmanners commented Dec 12, 2017

Uh oh!

Uh oh!

avstudnitz commented Oct 14, 2017 •

edited by LexKoomen

Loading

avstudnitz Dec 1, 2017 •

edited

Loading