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

Fix EZP-23170: swapping node with sub items with non-container node. #1032

Merged

Conversation

6 participants
@joaoinacio
Copy link
Contributor

commented Jul 17, 2014

JIRA: https://jira.ez.no/browse/EZP-23170

Problem: swapping a non-container node (class definition) with a node that contains children/sub items will result in an invalid state;
The new node will display the sub item count but will not list any children on the admin interface.

This PR adds an additional check in content/action that prevents the swap if any of the nodes is not a container and the other contains children.

Tests: manual.

Note:
An improvement to this would be to add the check in the interface, before the action is executed (for example when browsing for the "destination" node.

// verify one of the nodes contains children and the other is not a container.
if ( !$node->classIsContainer() && $selectedNode->childrenCount() > 0 )
{
eZDebug::writeError( "Cannot use node $selectedNodeID as the exchanging node for $nodeID, as it contains sub items (node is not container)",

This comment has been minimized.

Copy link
@ghost

ghost Jul 17, 2014

it's the result of a user action that is not allowed. maybe a warning is enough, to avoid unneeded scares?

This comment has been minimized.

Copy link
@joaoinacio

joaoinacio Jul 22, 2014

Author Contributor

@Pbras: indeed.
I based on the checks above, but those are somewhat unexpected conditions (AFAIK, even the lack of permissions is checked earlier/in UI).
I mentioned ideally there should be the same UI check for containers, in which case the error would be appropriate.

}
if ( !$selectedNode->classIsContainer() && $node->childrenCount() > 0 )
{
eZDebug::writeError( "Cannot use node $selectedNodeID as the exchanging node for $nodeID, as it is not container (node contains sub items)",

This comment has been minimized.

Copy link
@ghost
@glye

This comment has been minimized.

Copy link
Member

commented Jul 24, 2014

+1, but agree with @Pbras on using warnings rather than errors. Also agree the UI should stop these cases before they get this far, but that's not strictly necessary for the fix.

@ghost

This comment has been minimized.

Copy link

commented Jul 25, 2014

+1's / comments anyone? ;)

@bdunogier

This comment has been minimized.

Copy link
Member

commented Jul 25, 2014

Is redirecting to view/full/2 really sufficient ? The error message won't even be visible, since we redirect...

@glye

This comment has been minimized.

Copy link
Member

commented Jul 25, 2014

Good point @bdunogier. Working on a way to ensure such nodes can not be selected. (The error logging should stay, in any case.)

@glye

This comment has been minimized.

Copy link
Member

commented Jul 25, 2014

@joaoinacio Attached a blocker for such nodes in the jira issue, can be be added to this PR to have both in one place.

@joaoinacio

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2014

@glye: Thanks!
PR updated and squashed, should the errors still be changed to warnings?

@bdunogier

This comment has been minimized.

Copy link
Member

commented Jul 25, 2014

Is the message that something went happened we should warn the user of, or
that the requested operation was not completed due to an error ?

@joaoinacio

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2014

Given that the interface should no longer allow it, IMHO it is an (unexpected) error and the operation will not complete.
I think its the same as the other error conditions/checks (for example if the user does not have permission).

@crevillo

This comment has been minimized.

Copy link
Collaborator

commented Jul 25, 2014

To me is the second. If the operation cannot be done => error. "you cannot swap with this node"

For me, warnings should be for things like "Well, ez Will proceed as you want But note that..." like when you try to clear caches with root account, for instance.

@bdunogier

This comment has been minimized.

Copy link
Member

commented Jul 25, 2014

Error it is then. And I actually expect a real error, like a kernel error.

@crevillo

This comment has been minimized.

Copy link
Collaborator

commented Jul 25, 2014

Yes, me too.

and( eq( $browse.action_name, 'SwapNode' ),
eq( get_class( $swap_node ), 'ezcontentobjecttreenode' ),
or( and( $swap_node.children_count|gt(0), $Nodes.item.object.content_class.is_container|not ),
and( $swap_node.is_container|not, $Nodes.item.children_count|gt(0) ) ) ) )}

This comment has been minimized.

Copy link
@lolautruche

lolautruche Jul 28, 2014

Contributor

These tests are very complex and thus hard to understand. Could this be fixed by an operator?

This comment has been minimized.

Copy link
@glye

glye Jul 28, 2014

Member

@lolautruche The first line of the test, which was there before, I don't understand either. (Why can't you move non-container nodes?) The rest of it, on multiple lines, seems clear enough (FWIW, I wrote it). An operator would just move the complexity elsewhere imho, though it might make things easier for someone reimplementing this template.

This comment has been minimized.

Copy link
@andrerom

andrerom Jul 28, 2014

Member

Basically you can not move / copy / add nodes to a non container node.

@joaoinacio It would be more readable letting the original "if" be and add this new logic as a separate if.

This comment has been minimized.

Copy link
@glye

glye Jul 29, 2014

Member

@joaoinacio Attached EZP-23170-disable-radiobuttons_v2.diff to the jira. As AR suggests, this leaves the if-statement unmodified and uses a new elseif instead.

@glye

This comment has been minimized.

Copy link
Member

commented Jul 28, 2014

Yep, errors should stay errors now. I tend to agree that an actual kernel error makes sense now, since the situation is now something that Should Never Happen(TM). Either way, +1

@joaoinacio

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2014

Updated with @glye's modified template code.
I could change the errors to KERNEL_* , but what would be appropriate? KERNEL_NOT_AVAILABLE?
What about the already existing checks (canSwap(), etc)?

@glye

This comment has been minimized.

Copy link
Member

commented Jul 29, 2014

KERNEL_NOT_AVAILABLE is close enough imho.

For canSwap() I think KERNEL_ACCESS_DENIED is right, and it would reflect the use on line 380.
https://github.com/joaoinacio/ezpublish-legacy/blob/EZP-23170_swapnode_container_subitems/kernel/content/action.php#L380

"Content node with ID $selectedNodeID does not exist" is an obvious KERNEL_NOT_AVAILABLE

@crevillo

This comment has been minimized.

Copy link
Collaborator

commented Jul 29, 2014

KERNEL_NOT_AVAILABLE works for me too

2014-07-29 8:36 GMT-05:00 Gunnstein Lye notifications@github.com:

KERNEL_NOT_AVAILABLE is close enough imho.

For canSwap() I think KERNEL_ACCESS_DENIED is right, and it would reflect
the use on line 380.

https://github.com/joaoinacio/ezpublish-legacy/blob/EZP-23170_swapnode_container_subitems/kernel/content/action.php#L380

"Content node with ID $selectedNodeID does not exist" is an obvious
KERNEL_NOT_AVAILABLE


Reply to this email directly or view it on GitHub
#1032 (comment)
.

Twitter : @crevillo
Skype ID : carlostanta

Desarrollador en Tanta Comunicación.
eZ Diff Squad Member

@joaoinacio

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2014

All done, thanks!

@glye

This comment has been minimized.

Copy link
Member

commented Jul 29, 2014

+1

@glye

This comment has been minimized.

Copy link
Member

commented Jul 30, 2014

review ping @bdunogier @lolautruche

}
if ( !$selectedNode->canSwap() )
{
eZDebug::writeError( "Cannot use node $selectedNodeID as the exchanging node for $nodeID, the current user does not have edit permission for it",
'content/action' );
return $module->redirectToView( 'view', array( 'full', 2 ) );
return $module->handleError( eZError::KERNEL_ACCESS_DENIED, 'kernel', array() );

This comment has been minimized.

Copy link
@crevillo

crevillo Jul 30, 2014

Collaborator

to me this is a KERNEL_NOT_AVAILABLE too. We shouldn't throw a KERNEL_ACCESS_DENIED to an administrator user imho...

This comment has been minimized.

Copy link
@andrerom

andrerom Jul 30, 2014

Member

+1, either that or SHOP_NOT_A_PRODUCT

This comment has been minimized.

Copy link
@glye

glye Jul 30, 2014

Member

@crevillo Why not? And would you also change the same error on line 380? Access denied seems a natural fit to me.

This comment has been minimized.

Copy link
@crevillo

crevillo Jul 30, 2014

Collaborator

@glye because as by default and admin user have access to do everything, it looks a bit weird to me telling that he/she has no access to do that.
Same for line 380, yes. There's no policy that denies an admin user to swap a node. If such policy existed it would deny to do the swap operation entirely, no matter if the destination node is a container or not...

Just my 2 cents :).

This comment has been minimized.

Copy link
@glye

glye Jul 30, 2014

Member

@crevillo An admin user who has access to everything will not be getting that error - canSwap() will return true for them. It's a test on edit permission and has nothing to do with whether node or target is a container. The container check follows below, line 414. So:

  • No edit permission: Access Denied
  • IsContainer mismatch: Not Available

OK?

And ten points to AR for very obscure joke :)

This comment has been minimized.

Copy link
@crevillo

crevillo Jul 30, 2014

Collaborator

@glye damn, i missread it. Sorry about this. You're totally right.
+1.

@glye

This comment has been minimized.

Copy link
Member

commented Jul 30, 2014

Thanks all for participating. Will merge if no one screams otherwise before tomorrow morning.

glye added a commit that referenced this pull request Jul 30, 2014

Merge pull request #1032 from joaoinacio/EZP-23170_swapnode_container…
…_subitems

Fix EZP-23170: swapping node with sub items with non-container node.

@glye glye merged commit dd3d96a into ezsystems:master Jul 30, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@andrerom

This comment has been minimized.

Copy link
Member

commented Jul 30, 2014

post traumatic +1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.