Skip move logic if the parent is staying the same

[CyberReiter]

Prerequisites

• I have added steps to test this contribution in the description below

Description

Added checks to ContentTypes, Content and DataTypes Move functions to skip all the logic if the parent would stay the same.

How to test

Try moving around ContentTypes, Content and DataTypes and make sure this still works as expected

[github-actions]

Hi there @CyberReiter, thank you for this contribution! 👍

While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

• It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
• The automated tests all pass (see "Checks" tab on this PR)
• The level of security for this contribution is the same or improved
• The level of performance for this contribution is the same or improved
• Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
• If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
• 💡 The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

[mikecp]

Hello @CyberReiter ,

Thanks for this update, it's always better to prevent unnecessary actions 😉

I was wondering if you could also implement this on the Dictionary items move action, which is located in Umbraco.Cms.Web.BackOffice.Controllers.DictionaryController, in the PostMove method.
It would be great if we could have the same behavior there as well 😁

Cheers!

[CyberReiter]

Hey @mikecp

Good catch, I totally forgot to also check Controllers for such logic!
Added some checks and also updated the translations to reflect this changes

[mikecp]

Hi @CyberReiter ,

Thanks for the update, it's good that we cover dictionary items as well 👍

I have one last remark, and I'm very sorry I did not notice it the first time. While testing today, I noticed that when we move media items and dictionary items under the same parent, we get an error message, while we get a success message in the other cases.

I think it would be more consistent to have the same behavior in all cases, and I guess it is then better to align with what already existed, which would then be to display errors in the other situations.
From what I saw, it can be adapted in the content type and data type logic by replacing the return line

return OperationResult.Attempt.Succeed(MoveOperationStatusType.Success, eventMessages);

with something like

return OperationResult.Attempt.Fail(MoveOperationStatusType.FailedNotAllowedByPath, eventMessages);

We then get an error like this, which is similar to what we get on Dictionary:

For the moving of content itself, the logic is quite different and it seems not possible to return an error without creating a full scope, so here I would leave it as you implemented it, and return a success. I'm pinging @nul800sebastiaan here to get his feeling on this.

So, what do you think about all this? Would you be willing to update to the more consistent way 😁?
In that case there is a little catch on the move of content type: there seems to be no translation on the error title somehow

Thanks in advance for the extra help!

Cheers!

[CyberReiter]

Hi @mikecp

I also found it strange that the behaviour was different for the different types but I thought I would leave it as it is for now.
However since I am also a fan of consistency I will take another look at it 😁

For the content part I would leave that out for this PR as I don't see a way of changing this without making a breaking change.

[CyberReiter]

Changed the behaviour to now show an error across the board when moving an item to its parent.

For the missing translations on the content types the problem seems to be that the promise returned by the localizationservice is not able to finish in time and thus will always be an empty object. For that reason I went with the same approach as already implemented on the datatypes and just added a static message - since the notification message will still be localized this should be sufficient.

[mikecp]

I saw your comment about the promise not being returned on time, but what about keeping the promise and having something like

promise ?? 'Failed to move content type'

This way, if the promise ever gets returned on time, we get the foreseen label and otherwise we have a fallback.

What do you think? (same question in mediatype.resource of course😁 )

[CyberReiter]

problem is that the promise is not null but an Promise object so this won't work 🤔

There also does not seem to be an easy way of waiting for the promise to finish as there is no way to check for the state of it.

Only possibility that comes to my mind would be to have a bool which would be set to true if the promise finished in time and then set the message based on in it like so isFinished ? promise : 'Failed to move content type'
However by doing this we would introduce a 3rd way of handling this message as we already have the way it is done on the data types (static message) and another way for media (static message + taking some of the translation from the api response)

Umbraco-CMS/src/Umbraco.Web.UI.Client/src/common/resources/media.resource.js

Line 130 in a0d6732

errorMsg = errorMsg + ": " + data.notifications[0].message;

[mikecp]

Thanks for your comments @CyberReiter !

In that case, I would suggest to keep things as they are. It's already a great improvement, so it should not hold us from merging all this 😁

Thanks again for this update 👍

Cheers!