Admin Config Search #10335

maximgubar · 2017-07-26T13:18:48Z

Description

This feature will provide a possibility to find the config fields in Magento in a fast and easy way (e.g. live search).

Manual testing scenarios

Open Magento admin in your browser.
Type the keyword in global search input field.
If there is a match in the config elements - results should appear in 'autocomplete results' area.
Each result is clickable and should redirect to you to proper section/group of configuration UI.

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)

- reduced text info in search results - injection of dependency was changed to optional (backward compatibility) - highlighting of found field - defined vertical size of results list

maghamed · 2017-07-26T13:34:01Z

app/code/Magento/Backend/Model/Search/Config.php

+     * @param string $needle
+     * @param string $pathLabel
+     */
+    public function findInStructure(ElementIterator $structureElementIterator, $needle, $pathLabel = '')


why does this method declared as public?
We use it just inside this class

maghamed · 2017-07-26T13:44:46Z

app/code/Magento/Backend/Model/Search/Config.php

+        foreach ($structureElementIterator as $structureElement) {
+            if (!($structureElement instanceof ElementInterface)) {
+                continue;
+            }


why do we have this check?

Magento\Config\Model\Config\Structure\Element\Iterator returns \Magento\Config\Model\Config\Structure\ElementInterface by contract, do we have any reasons not to trust the declared contract and make additional check?

/** * Return the current element * * @return \Magento\Config\Model\Config\Structure\ElementInterface */ public function current() { return $this->_flyweight; }

maghamed · 2017-07-26T14:08:49Z

app/code/Magento/Backend/Model/Search/Config.php

+     * @param string $needle
+     * @param string $pathLabel
+     */
+    public function findInStructure(ElementIterator $structureElementIterator, $needle, $pathLabel = '')


don't think "needle" is a good name.
Unless we call $structureElementIterator as haystack

It's the user's search term, so let's call it this way

maghamed · 2017-07-26T14:26:56Z

app/code/Magento/Backend/Model/Search/Config.php

+            if (!($structureElement instanceof ElementInterface)) {
+                continue;
+            }
+            if (stripos((string)$structureElement->getLabel(), $needle) !== false) {


it's better to use
mb_stripos than stripos
Magento requires "ext-mbstring": "*", to be installed

maghamed · 2017-07-26T14:32:03Z

app/code/Magento/Backend/Model/Search/Config.php

+        if (!$this->hasQuery()) {
+            $this->setResults($this->resultBuilder->getAll());
+            return $this;
+        }


if we will move a check for the empty query into the findInStructure function, we could end-up with

public function load() { $this->findInStructure($this->configStructure->getTabs(), $this->getQuery()); $this->setResults($this->resultBuilder->getAll()); return $this; }

maghamed · 2017-07-26T14:33:52Z

app/code/Magento/Backend/Model/Search/Config/Result/Builder.php

+        self::STRUCTURE_ELEMENT_TYPE_SECTION,
+        self::STRUCTURE_ELEMENT_TYPE_GROUP,
+        self::STRUCTURE_ELEMENT_TYPE_FIELD,
+    ];


it's better not to hard code supported elements, but accept an additional parameter, which is configured through the DI.

So, if some new supported component would be added in future we could easily add this new type with DI configuration.

maghamed · 2017-07-26T14:36:26Z

app/code/Magento/Backend/Model/Search/Config/Result/Builder.php

+                    'section' => $elementPathParts[0],
+                    'group'   => $elementPathParts[1],
+                    'field'   => $structureElement->getId(),
+                ];


it's better to introduce an interface and provide implementation for each of the type.
To have an ability to add new supported type in future.
For now, we hardcoded existing structure elements

maghamed · 2017-07-26T14:38:04Z

app/code/Magento/Config/Block/System/Config/Edit.php

     */
    public function __construct(
        \Magento\Backend\Block\Template\Context $context,
        \Magento\Config\Model\Config\Structure $configStructure,
-        array $data = []
+        array $data = [],
+        Json $jsonSerializer = null


it's better to have a dependency on the interface instead?
Magento\Framework\Serialize\SerializerInterface

As mentioned on #8331 (comment) and referenced on #10342 (comment) , SerializerInterface is not guaranteed to return a JSON serializer in the future. And \Magento\Config\Block\System\Config uses it in a method explicitly named getConfigSearchParamsJson

- changed visibility of findInStructure() method + renamed its argument - removed unnecessary verifications

# Conflicts: # app/code/Magento/Backend/i18n/en_US.csv

- introduced extended interface for \Magento\Config\Model\Config\Structure\ElementInterface - moved structure element types from static dependency to DI

maghamed · 2017-08-22T06:10:37Z

app/code/Magento/Config/Model/Config/Structure/ElementNewInterface.php

+/**
+ * @api
+ */
+interface ElementNewInterface extends ElementInterface


Don't think that naming is very good in this case ElementInterface -> ElementNewInterface

I'll move it up and name it StructureElementInterface, wdyt ?

maghamed · 2017-08-22T12:59:58Z

app/code/Magento/Backend/Model/Search/Config/Result/Builder.php

+{
+    const STRUCTURE_ELEMENT_TYPE_SECTION = 'section';
+    const STRUCTURE_ELEMENT_TYPE_GROUP   = 'group';
+    const STRUCTURE_ELEMENT_TYPE_FIELD   = 'field';


why do we need these constants if they not used

maghamed · 2017-08-22T13:25:31Z

app/code/Magento/Backend/Model/Search/Config.php

+ *
+ * @method string|null getQuery()
+ * @method bool hasQuery()
+ * @method Config setResults(array $results)


it's better to have all these methods as a part of Object contract, not relying on __call usage

maghamed · 2017-08-22T14:01:41Z

app/code/Magento/Backend/Model/Search/Config.php

+ * @method string|null getQuery()
+ * @method bool hasQuery()
+ * @method Config setResults(array $results)
+ * @method array getResults()


it's better to have these methods in class contract, not relying on __call method

# Conflicts: # app/code/Magento/Config/Model/Config/Structure/ElementInterface.php

…-develop

ishakhsuvarov · 2017-08-30T10:54:02Z

Hi @maximgubar
Please check the Magento\Backend\Model\Search\ConfigTest failures. Looks like search result in the test is always empty. Let us know if you need any assistance.
Thank you.

ishakhsuvarov · 2017-09-11T12:02:12Z

@maximgubar Thank you for the update

vrann · 2017-09-20T14:22:03Z

@maximgubar

add description to the pull request
please fix static test:

Magento\Test\Php\XssPhtmlTemplateTest::testXssSensitiveOutput
Passed: 1172, Failed: 1, Incomplete: 0, Skipped: 0.
Data set: /opt/bamboo/agents/agent1/xml-data/build-dir/M2-AT2472-SLE1/magento2/app/code/Magento/Config/view/adminhtml/templates/system/config/edit.phtml
Potentially XSS vulnerability. Please verify that output is escaped at lines 391
Failed asserting that a string is empty.
/opt/bamboo/agents/agent1/xml-data/build-dir/M2-AT2472-SLE1/magento2/dev/tests/static/testsuite/Magento/Test/Php/XssPhtmlTemplateTest.php:48
/opt/bamboo/agents/agent1/xml-data/build-dir/M2-AT2472-SLE1/magento2/lib/internal/Magento/Framework/App/Utility/AggregateInvoker.php:56
/opt/bamboo/agents/agent1/xml-data/build-dir/M2-AT2472-SLE1/magento2/dev/tests/static/testsuite/Magento/Test/Php/XssPhtmlTemplateTest.php:51
(4 more lines...)

vrann · 2017-09-25T21:53:52Z

added changes per UX request:

.highlighted should be added to tr around the input
.highlighted color should be #DFF7FF

- address UX request for config search

maximgubar added 2 commits July 19, 2017 11:43

initial config search feature commit

30c1c5d

PR changes we applied:

94b423b

- reduced text info in search results - injection of dependency was changed to optional (backward compatibility) - highlighting of found field - defined vertical size of results list

ishakhsuvarov assigned vrann Jul 26, 2017

ishakhsuvarov added this to the July 2017 milestone Jul 26, 2017

ishakhsuvarov added the develop label Jul 26, 2017

maghamed suggested changes Jul 26, 2017

View reviewed changes

orlangur mentioned this pull request Jul 26, 2017

Remove zend json from json controller #10342

Merged

4 tasks

okorshenko modified the milestones: July 2017, August 2017 Aug 1, 2017

okorshenko added the Progress: needs update label Aug 2, 2017

maximgubar added 5 commits August 15, 2017 14:29

PR changes we applied:

e2e2048

- changed visibility of findInStructure() method + renamed its argument - removed unnecessary verifications

Merge remote-tracking branch 'upstream/develop' into develop

b9398af

# Conflicts: # app/code/Magento/Backend/i18n/en_US.csv

PR changes:

f5dd204

- introduced extended interface for \Magento\Config\Model\Config\Structure\ElementInterface - moved structure element types from static dependency to DI

Merge remote-tracking branch 'upstream/develop' into develop

a1d0f2d

added unit tests

05de33c

maghamed reviewed Aug 22, 2017

View reviewed changes

maghamed approved these changes Aug 22, 2017

View reviewed changes

maximgubar and others added 7 commits August 22, 2017 17:52

added integration test

3f84bd1

renamed ElementNewInterface to StructureElementInterface

42baff6

fixed static tests

c366ac4

fixed integration tests

74a3b06

updated test config for integration test

a468317

Merge remote-tracking branch 'upstream/develop' into develop

10cbd63

# Conflicts: # app/code/Magento/Config/Model/Config/Structure/ElementInterface.php

Merge branch 'develop' of github.com:magento/magento2 into maximgubar…

95503dd

…-develop

ishakhsuvarov self-assigned this Aug 30, 2017

magento-team added develop labels Aug 30, 2017

maximgubar added 2 commits September 11, 2017 14:00

fixed integration test

aff2aa5

Merge remote-tracking branch 'origin/develop' into develop

93f4245

maximgubar added 3 commits September 11, 2017 14:39

fixed static test

27dfdff

fixed static test

76b11c8

fixed static test

951f41e

magento-team removed the Progress: needs update label Sep 13, 2017

magento-engcom-team modified the milestones: August 2017, September 2017 Sep 18, 2017

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

magento-team added the Progress: needs update label Sep 20, 2017

fixed internal static test

fab44c3

maximgubar changed the title ~~WIP: Admin Config Search~~ Admin Config Search Sep 21, 2017

magento-team removed the Progress: needs update label Sep 25, 2017

magento-team merged commit fab44c3 into magento:develop Sep 27, 2017

magento-team pushed a commit that referenced this pull request Sep 27, 2017

MAGETWO-71048: WIP: Admin Config Search #10335

be8ecdb

- address UX request for config search

magento-team pushed a commit that referenced this pull request Sep 27, 2017

MAGETWO-71048: WIP: Admin Config Search #10335

32c1845

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Admin Config Search #10335

Admin Config Search #10335

maximgubar commented Jul 26, 2017 •

edited

Loading

maghamed Jul 26, 2017

maghamed Jul 26, 2017

maghamed Jul 26, 2017

maghamed Jul 26, 2017

maghamed Jul 26, 2017

maghamed Jul 26, 2017

maghamed Jul 26, 2017

maghamed Jul 26, 2017

korostii Jul 27, 2017

maghamed Aug 22, 2017

maximgubar Aug 22, 2017

maghamed Aug 22, 2017

maghamed Aug 22, 2017

maghamed Aug 22, 2017

ishakhsuvarov commented Aug 30, 2017

ishakhsuvarov commented Sep 11, 2017

vrann commented Sep 20, 2017 •

edited

Loading

vrann commented Sep 25, 2017

Admin Config Search #10335

Admin Config Search #10335

Conversation

maximgubar commented Jul 26, 2017 • edited Loading

Description

Manual testing scenarios

Contribution checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishakhsuvarov commented Aug 30, 2017

ishakhsuvarov commented Sep 11, 2017

vrann commented Sep 20, 2017 • edited Loading

vrann commented Sep 25, 2017

maximgubar commented Jul 26, 2017 •

edited

Loading

vrann commented Sep 20, 2017 •

edited

Loading