EZP-26523: Sorting UI for relation list edit view

[ryanolee]

This PR is a continuation of #771 in relation to this issue https://jira.ez.no/browse/EZP-26523.

This PR has been reopened with a few changes to the original approach. Comments left on the older PR should have been addressed.

Notably, the gallery-debounce module is now used to initialize the Y.Sortable UI element the dom changes too quickly for it to properly load when the field is initially populated with related items. Y.later could be used but conflicts with how events bind to the dom here https://yuilibrary.com/yui/docs/api/files/dd_js_delegate.js.html#l148 making Y.debounce a safer choice.

Tests are missing pending an initial round of code review on this approach.

[andrerom]

Thanks for opening this @ryanolee, we will try to do some reviews and see if we can help you get this into accepted form to merge.

One initial question, in regards to debounce; this is only during initial rendering as you say right? Is there some other place we could hook into to avoid trying to make it sortable before rendering has completed?

Review ping @mateuszbieniek, @adamwojs, ..

[ryanolee]

Hi @andrerom,
I did have a look into this. We might be able to trigger an event from this callback to initialize the sortibility of the relation list. It ended up making the event bindings very messy as we had to wait on this event before binding the relatedContentsChange handler for syncing / reinitializing sortibility when the relation list changed. I was also never able to get it to work using this approach.

I personally found the debounce approach to be the best as it removed any chance for the _makeListSortable method to be called in too quick succession. From what I could gather when working on this; the relatedContentsChange was being called too quickly leading to the event bindings to the dom not being made. This looks to be down the 50ms delay used by the YUI sortable widget for binding the events (See: https://yuilibrary.com/yui/docs/api/files/dd_js_delegate.js.html#l148). (Note I never got deep enough to confirm this was the issue 😞 ) Making the method debounced at a 75ms interval stops any chance of this being a problem.

Thanks!

[mateuszbieniek]

I think that we use debouncing in other places, so I think it should be fine.

[andrerom]

@ryanolee would you be able to rebase this on 1.13? I assume this is where you want this, not on a master that will never be released ;)

[ryanolee]

Hi @andrerom,
Would be more than happy to 👍

[ryanolee]

Hi @andrerom @mateuszbieniek,
I have just run into an issue with this using the eZ combo loader. When including gallery-debounce the admin makes a call out to this domain. http://yui.yahooapis.com/combo?gallery-2014.07.31-18-26/build/gallery-debounce/gallery-debounce.js This leads to issues when trying to access the asset over https from the browser as it is only served as http. The debounce code is ultimately included from this external library https://yuilibrary.com/gallery-archive/gallery/show/debounce.html . Are we good to make a copy of this library and include it in this repo or would it be better for me to build a separate debounce implementation? (given possible licensing restrictions https://github.com/yui/yui3/blob/master/LICENSE.md)

[mateuszbieniek]

Then I was mistaken about debounce. We should be good with simple timeout:

    Y.eZ.RelationListEditView = Y.Base.create('relationListEditView', Y.eZ.FieldEditView, [Y.eZ.AsynchronousView], {
        makeListSortableTimeout: null,
        events: {
            '.ez-relation-discover': {
                'tap': '_runUniversalDiscovery',
            },
            '.ez-relation-remove-content': {
                'tap': '_removeRelation'
            }
        },

        initializer: function () {
            var fieldValue = this.get('field').fieldValue;

            this._fireMethod = this._fireLoadObjectRelations;
            this._handleFieldDescriptionVisibility = false;
            if( fieldValue.destinationContentIds ){
                this._set('destinationContentsIds', fieldValue.destinationContentIds);
            }



            this.after('relatedContentsChange', function (e) {
                this._syncDestinationContentsIds(e);
                if (e.src === "remove") {
                    if (this.get('destinationContentsIds').length !== 0) {
                        this._vanish('tr[data-content-id="' + e.contentId + '"]', false);
                    } else {
                        this._vanish('.ez-relationlist-contents', true);
                    }
                } else {
                    this.render();
                }

                window.clearTimeout(this.makeListSortableTimeout);
                this.makeListSortableTimeout = window.setTimeout(this._makeListSortable(), 50);
            });
        },

[mateuszbieniek]

@ryanolee could you please add this line as well:
makeListSortableTimeout: null, so this.makeListSortableTimeout is defined?

    Y.eZ.RelationListEditView = Y.Base.create('relationListEditView', Y.eZ.FieldEditView, [Y.eZ.AsynchronousView], {
        makeListSortableTimeout: null,
        events: {

[ryanolee]

Looks like I accidentally closed the branch during the rebase. This should now be fixed.

[ryanolee]

Made the change @mateuszbieniek 👍

[mateuszbieniek]

👍 looks good

[andrerom]

I'm sending this to QA, however one of the test failures seems relevant and should be fixed in parallel if it is:

✖ [http://127.0.0.1:7000/Tests/js/views/fields/ez-relationlist-editview.html]: Passed: 0 Failed: 1 Total: 1 (ignored 0)
    Javascript Error
       TypeError: 'undefined' is not a function (evaluating 'this.get('content').get('mainLanguageCode')')
       http://127.0.0.1:7000/Resources/public/js/views/ez-fieldeditview.js:341
       http://127.0.0.1:7000/Resources/public/js/views/fields/ez-relationlist-editview.js:64
       http://127.0.0.1:7000/Resources/public/vendors/yui3/build/oop/oop.js:403
✖ [eZ Relation Edit View tests]: Passed: 24 Failed: 1 Total: 25 (ignored 0) (1.522 seconds)
    Should fill the relation with the universal discovery widget selection
       Unexpected error: 'undefined' is not an object (evaluating 'model.name')

@mateuszbieniek You think you can find time for that this week? Or are you up for it @ryanolee ?

[mateuszbieniek]

@andrerom sure, I will give it a look.

[ryanolee]

Hi @andrerom @mateuszbieniek ,
I was looking into writing tests for this last night but had some issues setting grover up. Thanks for giving it a look @mateuszbieniek. Let me know if you need anything from me in order to get it set up! Thanks!

[mateuszbieniek]

The test is failing due to this line:
fieldDefinitionIdentifier = this.getField().fieldDefinitionIdentifier;
It seems that in tests our debounce with setTimeout is ignored and this._makeListSortable is fired right away, which makes the above statement to fail with

TypeError: 'undefined' is not a function (evaluating 'this.get('content').get('mainLanguageCode')')

here: https://github.com/ezsystems/PlatformUIBundle/blob/master/Resources/public/js/views/ez-fieldeditview.js#L341

Right now I have no better idea than wrapping it up in try catch so the error won't mess up the tests. Maybe someone has a better idea @andrerom @ryanolee ?

We're missing get method for this->get('content') Mock. Implementing it in Tests/js/views/fields/assets/ez-relationlist-editview-tests.js resolves this error:

setUp: function () {
            //...            
            this.mainLanguageCode = 'eng-GB';
            //...
            Y.Mock.expect(this.content, {
                method: 'get',
                args: ["mainLanguageCode"],
                returns: this.mainLanguageCode
            });

But now I'm ending with a different error :
TypeError: 'undefined' is not a constructor (evaluating 'new Y.Sortable')
I'll investigate further.

[mateuszbieniek]

@ryanolee As there was a lot of fixing in tests (adding missing mocks and mocks fields) I created PR for this branch with changes, that should fix all broken tests (at least related to this PR 😉 )

[ryanolee]

Thank you very @mateuszbieniek much for helping to get tests working with this branch! I merged the PR into this feature branch. I will still try to get some tests written for this if I can get Grover set up 👍

[andrerom]

One of the jobs failed with:

3✖ [eZ Relation Edit View tests]: Passed: 24 Failed: 1 Total: 25 (ignored 0) (1.392 seconds)
754    Should fill the relation with the universal discovery widget selection
755       Unexpected error: 'undefined' is not an object (evaluating 'model.name')

Is this maybe random due to timeout vs sometimes slow CI?
I restarted the job, but if it helps to increase the timeout or something maybe we should do that to avoid random failures.

[mateuszbieniek]

@andrerom it's not related to this PR - I can get the same error on 1.13 branch.

[andrerom]

Indeed, so this is awaiting QA then.

[ryanolee]

HI @andrerom @mateuszbieniek ,
I came across a bug where the ordering on items would reset after adding relation through the UDW. I have created a fix for this, but the branch will need another round of CR unfortunately :( .
Thanks!
-Ryan

[ryanolee]

Hi @andrerom @mateuszbieniek ,
I have found another issue with the way we load object relation as we don't maintain the order of the relations we load in given we are using Y.paralell in order to do so. I am currently writing a fix for this but It will not be ready for QA before then. Thanks!
-Ryan

[ryanolee]

Hi @andrerom @mateuszbieniek ,
Looking into this more the issues I ran into were due to the version of YUI I was using being too old to have #973 patched in. Looking at it on newer versions it looks to work as intended given order is maintained while loading as it currently stands. I was able to set up Grover but was unable to come up with a way to reliably test this given it is very difficult to mock the reorder events given in sortable widget. This should be ready for QA again 🎉 .

Will this branch need tests writing for it before it is merged into the 1.13 . If so would either of you know where to begin with trying to write tests for this / how much of this needs testing?
Thanks for the help!
-Ryan

[micszo]

QA Approved on eZ Studio v1.13.5 with diff.
Couldn't reproduce the random order issues with or without the fix.
The new feature with manual reordering works fine.
Only a remark about UX. The mouse cursor could change to e.g. pointer shape to indicate to the user that reordering is possible.

[andrerom]

@mateuszbieniek Can you merge this, and up?

[mateuszbieniek]

@andrerom on it.