Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EZP-25478: Restore items whose original location has been deleted #819

Merged
merged 1 commit into from Mar 14, 2017

Conversation

@yannickroger
Copy link
Contributor

yannickroger commented Mar 7, 2017

Link: https://jira.ez.no/browse/EZP-25478

Description

When a subtree was deleted, in the trash, it was not possible to restore a subitem of this subtree because its parent had been deleted. With this patch, you can now select a new parent for these items in the trash.

Screencast: https://drive.google.com/file/d/0B5amepf6EkXbTTduR1BuaHJmWmc/view

Tests

Manual and unit tests

@yannickroger yannickroger requested review from dpobel and StephaneDiot Mar 7, 2017
contentDiscoveredHandler: Y.bind(function (e) {
var trashItems = [];

Y.Array.some(this.get('trashItems'), function (trashItem) {

This comment has been minimized.

Copy link
@dpobel

dpobel Mar 7, 2017

Contributor

I would put that in a dedicated method to avoid duplicating the code here (compared to _restoreTrashItems)

*
* @method restore
* @param {Object} options the required for the update
* @param {Object} options.api (required) the JS REST client instance
* @param {Object} options.destination (optional) locationId under which the item will be restored.

This comment has been minimized.

Copy link
@dpobel

dpobel Mar 7, 2017

Contributor

instead of writing @param {Object} options.destination (optional), write @param {Object} [options.destination]


/**
* Fired to restore the selected items
* @event restoreItems

This comment has been minimized.

Copy link
@dpobel

dpobel Mar 7, 2017

Contributor

please document the event parameters.

var restoreCallback = function () {},
destination = {};

this.options = {api: this.apiMock, destination: destination};

This comment has been minimized.

Copy link
@dpobel

dpobel Mar 7, 2017

Contributor

why not just using a local variable ?

method: "recover",
args: [this.id, destination, Mock.Value.Function],
run: Y.bind(function (id, options, callback) {
Assert.areSame(

This comment has been minimized.

Copy link
@dpobel

dpobel Mar 7, 2017

Contributor

missing assertions on options

This comment has been minimized.

Copy link
@yannickroger

yannickroger Mar 8, 2017

Author Contributor

actually, the mistake here is that options should be called destination and it is already asserted automatically in the args.

@@ -269,6 +270,65 @@ YUI.add('ez-trashview-tests', function (Y) {

this.view.fire('whatever:restoreTrashItemsAction');
},

"Should fire `restoreItems` with destination when restoring a single item": function () {

This comment has been minimized.

Copy link
@dpobel

dpobel Mar 7, 2017

Contributor

this test should be split in 3 tests:

  • run the UDW
  • allow to pick only container
  • fire restoreItems with the expected parameters
@yannickroger

This comment has been minimized.

Copy link
Contributor Author

yannickroger commented Mar 8, 2017

Feedback taken into account @dpobel

* @param {String} trashItemId
* @param {Array} resultArray Item will be added to this array if it is found
*/
_findTrashItems: function (trashItemId, resultArray) {

This comment has been minimized.

Copy link
@dpobel

dpobel Mar 9, 2017

Contributor

it's weird to have the result in a parameter of such method, why not returning the result ?

@yannickroger

This comment has been minimized.

Copy link
Contributor Author

yannickroger commented Mar 13, 2017

Feedback taken into account @dpobel

Copy link
Contributor

StephaneDiot left a comment

+1 after nipticks and damien's requested change

},

/**
* Restores the selected (using UDW) trash items

This comment has been minimized.

Copy link
@StephaneDiot

StephaneDiot Mar 13, 2017

Contributor

'trash item" as your UDW is configured with multiple: false

this._findTrashItems(trashItemId, trashItems);

this._fireRestoreItems(trashItems, e.selection.location.get('id'));

This comment has been minimized.

Copy link
@StephaneDiot

StephaneDiot Mar 13, 2017

Contributor

Not sure you really need this empty line (maybe same above)

@@ -43,6 +43,11 @@
</ul>
{{else}}
<span class="ez-trashview-info-message">{{translate 'trash.ancestors' 'trash'}}</span>
<button
data-trash-item-id="{{item.id}}"
class="ez-trashitem-restore pure-button ez-font-icon ez-button">

This comment has been minimized.

Copy link
@StephaneDiot

StephaneDiot Mar 13, 2017

Contributor

I'm not sure you need ez-font-icon here.

@yannickroger

This comment has been minimized.

Copy link
Contributor Author

yannickroger commented Mar 13, 2017

Feedback by @StephaneDiot taken into account, just waiting for your go @dpobel

@dpobel
dpobel approved these changes Mar 13, 2017
@yannickroger yannickroger force-pushed the ezp-25478-restore_orphans_trash branch from e32467a to fb2a032 Mar 13, 2017
@yannickroger yannickroger merged commit e024466 into master Mar 14, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
ezrobot Code review by ezrobot
Details
@yannickroger yannickroger deleted the ezp-25478-restore_orphans_trash branch Mar 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.