Skip to content

Conversation

PieterCappelle
Copy link
Contributor

@PieterCappelle PieterCappelle commented Jul 11, 2017

When you add values to some attribute it's possible you have created duplicated values. If you want to save that attribute at Stores > Attributes > Products you will see the error messagee "The value of Admin must be unique." but that error message is not telling you which value is not unique.

This PR shows which values are duplicated so you easily update/remove the values so you can pass the error. Let me know if I need to modify something because this is first PR.

Reproduce

  1. Create attribute (select)
  2. Add values
  3. Add value Blue
  4. Add second value Blue
  5. Submit.

Contribution checklist

  • All commits are accompanied by meaningful commit messages
  • Pull request has a meaningful description of its purpose
  • 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)

schermafbeelding 2017-07-11 om 21 42 27

@ishakhsuvarov ishakhsuvarov self-assigned this Jul 12, 2017
return isValid;
},

_config.message
messager
Copy link
Contributor

Choose a reason for hiding this comment

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

@PieterCappelle Why you can't use msg variable? Why you declare messager function?

Copy link
Contributor Author

@PieterCappelle PieterCappelle Jul 13, 2017

Choose a reason for hiding this comment

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

I can't use the msg variable because its not in the scope of the function inside the jQuery.validator.addMethod.

Copy link
Contributor

Choose a reason for hiding this comment

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

@PieterCappelle msg variable declared in the scope of main function at the same scope declared messager function. You can use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@omiroshnichenko No, you will be in a javascript closure - don't know the exact javascript scope name but I think it's a closure - . You have to use custom function to display the msg variable. I tried multiple times like code below and it's not working.

/**
 * Copyright © Magento, Inc. All rights reserved.
 * See COPYING.txt for license details.
 */

define([
    'jquery',
    'mage/backend/validation'
], function (jQuery) {
    'use strict';

    return function (config) {
        var msg = '',
            _config = jQuery.extend({
                element: null,
                message: '',
                uniqueClass: 'required-unique'
            }, config);

        if (typeof _config.element === 'string') {
            jQuery.validator.addMethod(
                _config.element,

                function (value, element) {
                    var inputs = jQuery(element)
                            .closest('table')
                            .find('.' + _config.uniqueClass + ':visible'),
                        valuesHash = {},
                        isValid = true,
                        duplicates = [];

                    inputs.each(function (el) {
                        var inputValue = inputs[el].value;

                        if (typeof valuesHash[inputValue] !== 'undefined') {
                            isValid = false;
                            duplicates.push(inputValue);
                        }
                        valuesHash[inputValue] = el;
                    });

                    if (!isValid) {
                        msg = _config.message.replace('%1', duplicates.join(', '));
                    }

                    return isValid;
                },

                msg
            );
        }
    };
});

@@ -10,11 +10,17 @@ define([
'use strict';

return function (config) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@PieterCappelle It would be nice if you will cover this changes with JS tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have any guide to create this, never did this before. Sorry.

Copy link
Contributor

Choose a reason for hiding this comment

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

@PieterCappelle Here is a doc and also you can see in dev/tests/js/jasmine/tests already created tests. If you will have some additional question contact me directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@omiroshnichenko I tried this but it's to complex for me at this moment. I don't even get how I need to make the file structure. Do I need to follow the exact path as in the module directory? Other modules use custom paths and are ignore the path of the module.

@PieterCappelle
Copy link
Contributor Author

Replied to all your comments @omiroshnichenko. Let me know if I can help.

@ishakhsuvarov ishakhsuvarov added this to the July 2017 milestone Jul 17, 2017
@magento-team magento-team merged commit 7911d0e into magento:develop Jul 18, 2017
magento-team pushed a commit that referenced this pull request Jul 18, 2017
magento-team pushed a commit that referenced this pull request Jul 18, 2017
[EngCom] Public Pull Requests

 - MAGETWO-70817: remove redundant else and use strict check #10271
 - MAGETWO-70787: update phpserver to support versioned static urls #10250
 - MAGETWO-70764: Fix overwrite default value image/file with NULL #10253
 - MAGETWO-70761: Show values that are duplicate in attribute error msg #10213
 - MAGETWO-70758: Fix for #4530 $product->getRatingSummary() always returns null […] #10248
 - MAGETWO-70706: simplify product lists #2 #9019
 - MAGETWO-70669: Fix fatal errors with deleteOptionsAndSelections #7711
 - MAGETWO-70464: Fix shipping address can use for billing #10130
 - MAGETWO-69913: magento/magento2 #9196 - Products ordered report doesn't show simple (child) products of configurable products #9908
@PieterCappelle PieterCappelle deleted the unique-admin-attribute branch December 21, 2017 20:23
@Ctucker9233
Copy link

@magento-team This PR isn't included in 2.2 Should it be backported?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants