EZP-28055: As a v2 Editor I want to search for content items by entering a search keyword #33

mikadamczyk · 2017-10-30T14:23:55Z

https://jira.ez.no/browse/EZP-28055

This PR implements a user search for content item in the content repository. He can initiate a search by entering a search keyword and the application will retrieve a list of all the content items that have the same keyword in any of their searchable fields. This search feature use a PHP API along with a dedicated Symfony controller. The results is sorted by modified. As paginator pagerfanta/pagerfanta was used.

webhdx · 2017-10-31T06:43:40Z

src/lib/Form/Type/Search/SimpleSearchType.php

+     */
+    public function getBlockPrefix()
+    {
+        return null;


Why do you set it to null? Block prefix is crucial for form theming. We do not have any strong convention but I started using underscored class name with type replaced with form word: simple_search_form.

I used it because I want simple parameters in the URL. I don't think so the block prefix is crucial for form theming, rather id or class added in twig template. I think form builder is not a good place for styling. What do you think?

Hmm, I'm not sure if you understand my point. All forms should have unique block name, this allows you to do overrides in twig:

{% block simple_search_form_widget %} {% set attr = attr|merge({'class': 'custom-class'}) %} {{ block('form_widget') }} {% endblock %}

Which is what we do in order to separate styling information (like class HTML attribute) from PHP code.

webhdx · 2017-10-31T06:45:05Z

src/lib/Form/Type/Search/SimpleSearchType.php

+     */
+    public function buildForm(FormBuilderInterface $builder, array $options)
+    {
+        $builder->setMethod('GET')


I'm not sure if we should force method in form builder. This kills reusability. You can always change this on form creation.

webhdx · 2017-10-31T06:45:19Z

src/lib/Form/Type/Search/SimpleSearchType.php

+    {
+        $resolver->setDefaults([
+            'data_class' => SimpleSearchData::class,
+            'csrf_protection' => false,


See my comment above.

webhdx · 2017-10-31T06:48:12Z

src/bundle/Resources/views/admin/search/search_form.html.twig

@@ -0,0 +1,9 @@
+{{ form_start(form) }}
+<div class="input-group">
+    {{ form_widget(form.query, {'attr': {'style': 'width: 200px', 'class': 'form-control'} }) }}


We have to move inline style to CSS. @dew326 can propose you best solution.

Inline style is not needed anymore

webhdx · 2017-10-31T06:48:56Z

src/bundle/Resources/views/admin/search/search.html.twig

+{% block pageTitle %}{% endblock %}
+
+{% block content %}
+    <div id="react-udw"></div>


Should UDW be a part of search page?

This is something we should change I think. We load UDW module in every page (in layout.html.twig) but the container is added in some subpage. I think we should move this container to the layout.html.twig and remove it from every subpage.

This is good point @dew326 . We should make a follow-up task on this UDW load.

webhdx · 2017-10-31T06:51:40Z

src/bundle/Controller/SearchController.php

+            $pagerfanta->setMaxPerPage($limit);
+            $pagerfanta->setCurrentPage($page);
+
+            $urlGenerator = $this->get('router');


Please inject in the constructor. We tend to not use container directly in the controllers.

webhdx · 2017-10-31T06:54:37Z

src/bundle/Controller/SearchController.php

+
+            $pagination = (new TwitterBootstrap4View())->render($pagerfanta, $routeGenerator);
+
+            return $this->render('@EzPlatformAdminUi/admin/search/list.html.twig', [


Can't we share the same template file for search? By looking at the twig files I can see there is a lot of code duplication going on.

Yes we can, but the grid template will be added, so I prefer separate templates to avoid mixing. What do you think?

dew326 · 2017-10-31T07:04:11Z

src/bundle/Resources/views/admin/search/search.html.twig

+
+{% block javascripts %}
+    {% javascripts
+    'bundles/ezplatformadminui/js/scripts/udw/browse.js' %}


This JS is always needed with the sidebar, if we have sidebar in layout.html.twig this JS should be loaded in layout.html.twig

dew326 · 2017-10-31T07:18:51Z

src/bundle/Resources/views/layout.html.twig

+                            <svg class="ez-icon ez-icon-browse">
+                                <use xlink:href="{{ asset('bundles/ezplatformadminui/img/ez-icons.svg') }}#browse"></use>
+                            </svg>
+                            Browse


Translation?

It's going to be fixed in #28 , @mikadamczyk can leave this as-is since he only moved the code.

dew326 · 2017-10-31T07:22:48Z

src/bundle/Resources/views/admin/search/search_form.html.twig

+    {{ form_widget(form.query, {'attr': {'style': 'width: 200px', 'class': 'form-control'} }) }}
+
+    <span class="input-group-btn">
+        <input type="submit" class="btn btn-primary" value="Search"/>


I think it should be <button>, and I think the icon is missing in this input.

alongosz

🙃

alongosz · 2017-10-31T10:39:51Z

src/bundle/Controller/SearchController.php

+            $query->filter = new Criterion\LogicalAnd(
+                [
+                    new Criterion\Visibility(Criterion\Visibility::VISIBLE),
+                    new Criterion\FullText($data->getQuery()),


Please validate if query is string and if it's not empty (to avoid making unnecessary calls to external SE handler).

alongosz · 2017-10-31T10:44:52Z

src/bundle/Controller/SearchController.php

+                ]
+            );
+            $query->sortClauses[] = new SortClause\DateModified(Query::SORT_ASC);
+


Please actually use $limit for query ($query->limit). Otherwise it will return default number of records (AFAICS now it is 25).
Also there's a need for $query->offset.

I don't know if and how this works with Pagerfanta, but offset and limits are needed to avoid killing external SE handler (e.g. can be Solr on a separate server)

alongosz · 2017-10-31T10:49:16Z

composer.json

@@ -24,9 +24,6 @@
    "require-dev": {
        "phpunit/phpunit": "^6.4"
    },
-    "require-dev": {
-        "phpunit/phpunit": "^6.4"
-    },


Changes out of scope should be in separate commits (yeah, we usually squash, but sometimes we don't).

alongosz · 2017-10-31T11:01:10Z

src/lib/Form/Data/Search/SearchData.php

+    private $page;
+
+    /** @var string */
+    private $query;


I never quite understood the need of encapsulation of data structure properties just to write getters and setters getting and setting them directly. This make sense to me only if structure is supposed to be immutable.

But not requesting changes, just asking why this is better. The flame war about this started around 2003 and never ended :)

Accessing properties directly looks kinda lame. $something->getProperty() looks sexier. 😂

I think the only reason is that other classes are the same :(

lserwatka · 2017-10-31T15:58:45Z

@webhdx @alongosz are you OK now?

alongosz · 2017-10-31T16:05:56Z

Nitpicks:

still would prefer change of composer.json which is out of scope to be in a separate commit, but if it's gonna be squashed, then there's no point :)
also commit message (after expanding) looks a little bit weird, but this can be handled on merge if squashed

webhdx · 2017-11-02T07:18:02Z

composer.json

@@ -22,9 +22,6 @@
        "ezsystems/ez-support-tools": "^0.1@dev",
        "knplabs/knp-menu-bundle": "^2.1"   
    },
-    "require-dev": {


@mikadamczyk Why does this PR remove phpunit? Seems unrelated/bad?

Weird, actually when I reviewed this first it seemed like cleanup of redundant block, but maybe I misread something

webhdx · 2017-11-02T07:19:39Z

src/bundle/Controller/SearchController.php

+class SearchController extends Controller
+{
+    /** @var SearchService */
+    private $searchService;


I would define all properties either private or protected. private seems to be a more popular choice with recent PRs in order to disallow relying someone's code on our controllers.

webhdx · 2017-11-02T07:22:08Z

src/lib/Form/Type/Search/SearchType.php

+    /**
+     * {@inheritdoc}
+     */
+    public function buildForm(FormBuilderInterface $builder, array $options)


Please override configureOptions() with default data_class set to your SearchData::class

Unjustified removal of phpunit

adamwojs · 2017-11-02T08:26:08Z

src/bundle/Controller/SearchController.php

+                );
+
+                $query->limit = $limit;
+                $query->offset = ($page - 1) * $limit;


You don't need to set limit and offset manually, pagerfanta should take care of this.

Ok, so apparently this already works with ContentSearchAdapter from kernel (which I didn't know or even expect to be implemented there, sorry). Not 100% convinced this is clean, as being PAPI developer I got confused, but seems like a intended way to go.

Ok, so I will remove limit and offset for query

yep, sorry for the mess.

barbaragr · 2017-11-03T12:04:26Z

Performing Search on the second page of search results.
Preconditions: perform search action to receive more than 10 results. I've got 12 elements on a list and 2 pages.

Steps:

Go to the second page.
Type a word in a search input.
Perform search action - "Page 2 does not exist. The currentPage must be inferior to 1" alert appears and there are no search results. It should automatically go to the first page and show results.

barbaragr · 2017-11-03T12:23:09Z

Searching with phrase containing '%' results with "The expression 'OR' expected at least 1 argument but none provided". Nothing is shown in search result.

The same for "?", "!", "-", "$"

adamwojs · 2017-11-03T12:25:54Z

Searching with phrase containing '%' results with "The expression 'OR' expected at least 1 argument but none provided". Nothing is shown in search result.

@barbaragr This is already known issue. See ezsystems/ezpublish-kernel#2029 for more details.

barbaragr

I've found a bug so please my comment above.

barbaragr

I've found a bug so please my comment above.

mikadamczyk · 2017-11-06T09:51:36Z

I fixed PR, can you take a look at it? @barbaragr @adamwojs @webhdx

Nattfarinn · 2017-10-30T14:29:11Z

src/bundle/Controller/SearchController.php

+     */
+    public function searchAction(Request $request): Response
+    {
+        $limit = $request->query->getInt('limit', 10);


It would be best to expose limit as a configuration option instead of hardcoded 10.

Nattfarinn · 2017-11-06T13:14:40Z

@webhdx Please confirm if you're fine with changes to your code review.

webhdx · 2017-11-06T13:26:56Z

src/bundle/Resources/views/admin/search/search_form.html.twig

+{{ form_start(form) }}
+<div class="input-group">
+    {{ form_widget(form.query, {'attr': {'class': 'form-control'} }) }}
+    {{ form_widget(form.page, {'attr': {'value': 1 }}) }}


~~We shouldn't overwrite the value in the view. Value can be passed in the controller by changing corresponding data object.~~

Actually with current implementation we cannot avoid doing that. It will need some refactoring in the future.

…ing a search keyword

…tintegration Merge IBX-1070 added fix for contentintegration

mikadamczyk requested review from adamwojs, Nattfarinn, alongosz and webhdx October 30, 2017 14:23

mikadamczyk force-pushed the ezp-28055-search-for-content branch from 46daeee to 0f0bfcb Compare October 30, 2017 15:59

ezsystems deleted a comment from ezrobot Oct 30, 2017

webhdx suggested changes Oct 31, 2017

View reviewed changes

dew326 reviewed Oct 31, 2017

View reviewed changes

mikadamczyk force-pushed the ezp-28055-search-for-content branch 2 times, most recently from 51286c8 to d4a3c53 Compare October 31, 2017 10:45

ezsystems deleted a comment from ezrobot Oct 31, 2017

mikadamczyk force-pushed the ezp-28055-search-for-content branch from d4a3c53 to d4f5933 Compare October 31, 2017 10:49

alongosz requested changes Oct 31, 2017

View reviewed changes

mikadamczyk force-pushed the ezp-28055-search-for-content branch from d4f5933 to 7d50235 Compare October 31, 2017 15:15

alongosz previously approved these changes Oct 31, 2017

View reviewed changes

webhdx suggested changes Nov 2, 2017

View reviewed changes

adamwojs requested changes Nov 2, 2017

View reviewed changes

micszo requested a review from barbaragr November 2, 2017 10:10

webhdx force-pushed the ezp-28055-search-for-content branch 2 times, most recently from 6aaa572 to 439dbf4 Compare November 3, 2017 09:54

dew326 force-pushed the ezp-28055-search-for-content branch from 439dbf4 to 4193e2a Compare November 3, 2017 10:15

barbaragr requested changes Nov 3, 2017

View reviewed changes

mikadamczyk force-pushed the ezp-28055-search-for-content branch from 4193e2a to c72ca58 Compare November 6, 2017 09:43

adamwojs approved these changes Nov 6, 2017

View reviewed changes

barbaragr approved these changes Nov 6, 2017

View reviewed changes

Nattfarinn approved these changes Nov 6, 2017

View reviewed changes

webhdx suggested changes Nov 6, 2017

View reviewed changes

webhdx force-pushed the ezp-28055-search-for-content branch 2 times, most recently from 2d85fdc to dbb6f2a Compare November 6, 2017 14:30

EZP-28055: As a v2 Editor I want to search for content items by enter…

6fdcd3a

…ing a search keyword

webhdx force-pushed the ezp-28055-search-for-content branch from 448e9c7 to 6fdcd3a Compare November 6, 2017 15:10

webhdx approved these changes Nov 6, 2017

View reviewed changes

ezsystems deleted a comment from ezrobot Nov 6, 2017

lserwatka merged commit 4e5337a into ezsystems:master Nov 6, 2017

tischsoic pushed a commit that referenced this pull request Nov 8, 2021

Merge pull request #33 from ibexa/merge-ibx-1070-added-fix-for-conten…

132795a

…tintegration Merge IBX-1070 added fix for contentintegration


		$pagination = (new TwitterBootstrap4View())->render($pagerfanta, $routeGenerator);

		return $this->render('@EzPlatformAdminUi/admin/search/list.html.twig', [

EZP-28055: As a v2 Editor I want to search for content items by entering a search keyword #33

EZP-28055: As a v2 Editor I want to search for content items by entering a search keyword #33

Conversation

mikadamczyk commented Oct 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webhdx Oct 31, 2017 • edited Loading

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

webhdx Oct 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alongosz left a 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

lserwatka commented Oct 31, 2017

alongosz commented Oct 31, 2017

webhdx Nov 2, 2017 • edited Loading

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

alongosz Nov 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barbaragr commented Nov 3, 2017

barbaragr commented Nov 3, 2017 • edited Loading

adamwojs commented Nov 3, 2017

barbaragr left a comment

Choose a reason for hiding this comment

barbaragr left a comment

Choose a reason for hiding this comment

mikadamczyk commented Nov 6, 2017

Choose a reason for hiding this comment

Nattfarinn commented Nov 6, 2017

webhdx Nov 6, 2017 • edited Loading

Choose a reason for hiding this comment

webhdx Oct 31, 2017 •

edited

Loading

webhdx Oct 31, 2017 •

edited

Loading

webhdx Nov 2, 2017 •

edited

Loading

alongosz Nov 3, 2017 •

edited

Loading

barbaragr commented Nov 3, 2017 •

edited

Loading

webhdx Nov 6, 2017 •

edited

Loading