EZP-26325: Use query and filter in PlatformUI #712

StephaneDiot · 2016-11-03T08:57:40Z

Jira: https://jira.ez.no/browse/EZP-26325

Description

The goal here was to be able to use query and filter Query properties instead of criteria. By using this users will now be able, while using Solr, to get search results sorted with relevancy.

BTW you need to have a ez-JS-REST-client up to date with ezsystems/ez-js-rest-client#80 (hopefully merged soon).

Tests

unit tests

Todo

Review & fixes
Bump composer.json / * requirements on rest client once that has been merged and new tag is out

StephaneDiot · 2016-11-03T10:23:47Z

ready for review @dpobel @yannickroger

dpobel · 2016-11-03T13:20:26Z

Tests/js/models/assets/ez-contentmodel-tests.js

+                method: 'setFilter',
+                args: [Mock.Value.Object],
+                run: Y.bind(function (arg) {
+                    this.query.body.ViewInput.LocationQuery.Filter = arg;


you should not need that. The point of defining a Mock is too avoid this (reimplementation of the object being mocked)

dpobel · 2016-11-03T13:21:02Z

Tests/js/models/assets/ez-locationmodel-tests.js

+                method: 'setFilter',
+                args: [Mock.Value.Object],
+                run: Y.bind(function (arg) {
+                    this.viewCreateStruct.body.ViewInput.LocationQuery.Filter = arg;


same as https://github.com/ezsystems/PlatformUIBundle/pull/712/files#diff-b1369eadf69b693372a3aedfb871e051

dpobel · 2016-11-03T13:21:49Z

Tests/js/views/services/plugins/assets/ez-searchplugin-tests.js

@@ -22,8 +37,15 @@ YUI.add('ez-searchplugin-tests', function (Y) {
            this.capi = new Mock();
            this.contentService = new Mock();
            this.viewName = 'REST-View-Name';
-            this.query = {body: {ViewInput: {ContentQuery: {}}}};
-
+            this.query = new Y.Mock({body: {ViewInput: {ContentQuery: {}}}});


no need for the parameter

dpobel · 2016-11-03T13:23:41Z

Tests/js/views/services/plugins/assets/ez-searchplugin-tests.js

+            method: 'createView',
+            args: [context.query, Mock.Value.Function],
+            run: Y.bind(function () {
+                Y.Mock.verify(context.query);


you should not do that here, you have no guarantee this will be called. You should do that at the end of each test.

dpobel · 2016-11-03T13:25:11Z

Tests/js/views/services/plugins/assets/ez-searchplugin-tests.js

+            this.query = new Y.Mock({body: {ViewInput: {ContentQuery: {}}}});
+            Mock.expect(this.query, {
+                method: 'setLimitAndOffset',
+                args: [Mock.Value.Any, Mock.Value.Any],


this method is supposed to receive specific values not Any

dpobel · 2016-11-03T13:25:53Z

Tests/js/views/services/plugins/assets/ez-searchplugin-tests.js

+            });
+            Mock.expect(this.query, {
+                method: 'setSortClauses',
+                args: [Mock.Value.Object],


same here, you are expecting a very specific object

dpobel · 2016-11-03T13:27:47Z

Tests/js/views/services/plugins/assets/ez-searchplugin-tests.js

@@ -296,8 +316,16 @@ YUI.add('ez-searchplugin-tests', function (Y) {
            this.capi = new Mock();
            this.contentService = new Mock();
            this.viewName = 'REST-View-Name';
-            this.query = {body: {ViewInput: {LocationQuery: {}}}};
+            this.query = new Mock({body: {ViewInput: {LocationQuery: {}}}});


same remarks as in contentSearchTests

dpobel · 2016-11-03T13:28:44Z

Tests/js/views/services/plugins/assets/ez-searchplugin-tests.js


+            Mock.expect(this.query, {
+                method: 'setLimitAndOffset',
+                args: [Mock.Value.Any, Mock.Value.Any],


and same as in the others tests

dpobel · 2016-11-03T13:29:22Z

Tests/js/views/services/plugins/assets/ez-searchplugin-tests.js

@@ -796,8 +840,8 @@ YUI.add('ez-searchplugin-tests', function (Y) {
            this.capi = new Mock();
            this.contentService = new Mock();
            this.viewName = 'REST-View-Name';
-            this.locationQuery = {body: {ViewInput: {LocationQuery: {}}}};
-            this.contentQuery = {body: {ViewInput: {ContentQuery: {}}}};
+            this.locationQuery = new Mock({body: {ViewInput: {LocationQuery: {}}}});


no need for the parameter

andrerom · 2016-11-03T15:22:27Z

Note: Added todo in description in order to make sure we update composer requirements once the rest client changes are merged and tagged for UI use.

StephaneDiot · 2016-11-07T14:17:09Z

updated @dpobel

dpobel

and about the changes in ezsystems/ez-js-rest-client#80 ?

dpobel · 2016-11-08T13:47:32Z

Resources/public/js/views/services/plugins/ez-searchplugin.js

@@ -69,6 +69,7 @@ YUI.add('ez-searchplugin', function (Y) {
            }
            if (search.criteria) {
                query.setCriteria(search.criteria);
+                console.warn("Criteria query's property is deprecated");


that's not the standard deprecation log format

dpobel · 2016-11-08T14:06:07Z

Tests/js/views/services/plugins/assets/ez-searchplugin-tests.js

+            this.sortClauses = {};
+            Mock.expect(this.query, {
+                method: 'setLimitAndOffset',
+                args: [Mock.Value.Number, Mock.Value.Number],


this is still too weak, you don't expect any number but very specific values for limit and offset

dpobel · 2016-11-08T14:07:01Z

Tests/js/views/services/plugins/assets/ez-searchplugin-tests.js

+            });
+            Mock.expect(this.query, {
+                method: 'setSortClauses',
+                args: [Mock.Value.Object],


instead of having a run function with an assertion, you can directly put this.sortClauses here

dpobel · 2016-11-08T14:08:27Z

Tests/js/views/services/plugins/assets/ez-searchplugin-tests.js

@@ -111,7 +147,8 @@ YUI.add('ez-searchplugin-tests', function (Y) {
            this.view.fire('contentSearch', {
                viewName: this.viewName,
                search: {
-                    criteria: {},
+                    offset: 42,


do you need to add offset and limit ? those parameters are documented as optional, so this should not be needed

ok I understand, with your patch offset and limit become mandatory. This is a BC break (most likely this breaks some parts of PlatformUI) so the solution is not to fix the unit tests but fix the code.

Well no, offset and limit aren't mandatory... But the query mocks now expect 2 args number now for their setLimitAndOffset method (that's why i had Y.Mock.Any before). So now I had to fill the tests with limit and offset.

then it's completely useless and you fix your mock instead of changing each and every test

dpobel · 2016-11-08T14:08:59Z

Tests/js/views/services/plugins/assets/ez-searchplugin-tests.js

+            });
+            Mock.expect(this.query, {
+                method: 'setSortClauses',
+                args: [Mock.Value.Object],


same as previously, this.sortClauses can be used

dpobel · 2016-11-08T14:09:03Z

Tests/js/views/services/plugins/assets/ez-searchplugin-tests.js


+            Mock.expect(this.query, {
+                method: 'setLimitAndOffset',
+                args: [Mock.Value.Number, Mock.Value.Number],


too weak as well

yannickroger

Don't forget to squash your commits.

miguelcleverti · 2016-12-14T09:57:32Z

PR approved by QA

StephaneDiot changed the base branch from master to 1.6 November 3, 2016 08:57

StephaneDiot changed the base branch from 1.6 to master November 3, 2016 08:58

StephaneDiot force-pushed the EZP-26325_PlatformUI_search_sorting branch from a0db9c0 to e8a410b Compare November 3, 2016 09:00

StephaneDiot changed the base branch from master to 1.6 November 3, 2016 09:01

StephaneDiot force-pushed the EZP-26325_PlatformUI_search_sorting branch 2 times, most recently from d170b2d to 5c98737 Compare November 3, 2016 09:57

StephaneDiot changed the title ~~[WIP] EZP-26325: Use query and filter in PlatformUI~~ EZP-26325: Use query and filter in PlatformUI Nov 3, 2016

StephaneDiot force-pushed the EZP-26325_PlatformUI_search_sorting branch from 5c98737 to 34890e5 Compare November 3, 2016 10:05

dpobel suggested changes Nov 3, 2016

View reviewed changes

StephaneDiot force-pushed the EZP-26325_PlatformUI_search_sorting branch from 7c05985 to 28085b7 Compare November 7, 2016 14:16

dpobel suggested changes Nov 8, 2016

View reviewed changes

StephaneDiot force-pushed the EZP-26325_PlatformUI_search_sorting branch from 28085b7 to 3b8d69d Compare November 9, 2016 13:03

dpobel approved these changes Nov 24, 2016

View reviewed changes

andrerom approved these changes Nov 26, 2016

View reviewed changes

yannickroger approved these changes Nov 28, 2016

View reviewed changes

dpobel force-pushed the EZP-26325_PlatformUI_search_sorting branch from 3b8d69d to 97ca9cc Compare December 5, 2016 10:11

dpobel mentioned this pull request Dec 5, 2016

EZP-26325: Content search instead of Location search #758

Merged

3 tasks

StephaneDiot force-pushed the EZP-26325_PlatformUI_search_sorting branch 2 times, most recently from 0c0a098 to 85e5d51 Compare December 13, 2016 10:52

stephaneDiot and others added 8 commits December 13, 2016 11:56

EZP-26325: Use query and filter in PlatformUI

d4e4547

EZP-26325: review fixes

0fb7c25

EZP-26325: Use query and filter in PlatformUI

1de6db9

EZP-26325: review fixes

f5aeb90

EZP-26325: Added a findContent public method to the search plugin

84143dc

WIP loadLocation when doing a Content search

147708a

EZP-26325: search plugin tests

c339f36

EZP-26325: rebase fixes

125ec4a

StephaneDiot force-pushed the EZP-26325_PlatformUI_search_sorting branch from 85e5d51 to 125ec4a Compare December 13, 2016 10:58

EZP-26325: Update PUI assets bundle composer requirements to 3.1

5aca129

StephaneDiot merged commit c487744 into 1.6 Dec 15, 2016

andrerom deleted the EZP-26325_PlatformUI_search_sorting branch January 18, 2017 11:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-26325: Use query and filter in PlatformUI #712

EZP-26325: Use query and filter in PlatformUI #712

StephaneDiot commented Nov 3, 2016 •

edited

Loading

StephaneDiot commented Nov 3, 2016

dpobel Nov 3, 2016

dpobel Nov 3, 2016

dpobel Nov 3, 2016

dpobel Nov 3, 2016

dpobel Nov 3, 2016

dpobel Nov 3, 2016

dpobel Nov 3, 2016

dpobel Nov 3, 2016

dpobel Nov 3, 2016

andrerom commented Nov 3, 2016 •

edited

Loading

StephaneDiot commented Nov 7, 2016

dpobel left a comment

dpobel Nov 8, 2016

dpobel Nov 8, 2016

dpobel Nov 8, 2016

dpobel Nov 8, 2016

dpobel Nov 8, 2016

StephaneDiot Nov 9, 2016

dpobel Nov 9, 2016

dpobel Nov 8, 2016

dpobel Nov 8, 2016

yannickroger left a comment

miguelcleverti commented Dec 14, 2016

EZP-26325: Use query and filter in PlatformUI #712

EZP-26325: Use query and filter in PlatformUI #712

Conversation

StephaneDiot commented Nov 3, 2016 • edited Loading

Description

Tests

Todo

StephaneDiot commented Nov 3, 2016

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

andrerom commented Nov 3, 2016 • edited Loading

StephaneDiot commented Nov 7, 2016

dpobel 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yannickroger left a comment

Choose a reason for hiding this comment

miguelcleverti commented Dec 14, 2016

StephaneDiot commented Nov 3, 2016 •

edited

Loading

andrerom commented Nov 3, 2016 •

edited

Loading