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.

Sign up for GitHub

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

Jump to bottom

Skip move logic if the parent is staying the same #12937

Merged
merged 6 commits into from
Sep 9, 2022
Merged

Skip move logic if the parent is staying the same #12937

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
31eaa7d
v10/feature/skip-move-logic: add logic to skip move logic when parent…
CyberReiter Sep 1, 2022
3552ac9
v10/feature/skip-move-logic: remove unnecessary checks
CyberReiter Sep 1, 2022
6c3fea3
remove check for mediaservice as there is already one in place
CyberReiter Sep 2, 2022
4b7c20a
remove unnecessary using
CyberReiter Sep 2, 2022
4289c81
added parent checks for dictionary items
CyberReiter Sep 5, 2022
5465464
changing behaviour for contenttypes, datatypes and mediatypes to show…
CyberReiter Sep 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Umbraco.Core/EmbeddedResources/Lang/en.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1178,7 +1178,7 @@ To manage your website, simply open the Umbraco backoffice and start adding cont
<key alias="nodeSelected">has been selected as the root of your new content, click 'ok' below.</key>
<key alias="noNodeSelected">No node selected yet, please select a node in the list above before clicking 'ok'</key>
<key alias="notAllowedByContentType">The current node is not allowed under the chosen node because of its type</key>
<key alias="notAllowedByPath">The current node cannot be moved to one of its subpages</key>
<key alias="notAllowedByPath">The current node cannot be moved to one of its subpages neither can the parent and destination be the same</key>
<key alias="notAllowedAtRoot">The current node cannot exist at the root</key>
<key alias="notValid">The action isn't allowed since you have insufficient permissions on 1 or more child
documents.
Expand Down
2 changes: 1 addition & 1 deletion src/Umbraco.Core/EmbeddedResources/Lang/en_us.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1203,7 +1203,7 @@ To manage your website, simply open the Umbraco backoffice and start adding cont
<key alias="nodeSelected">has been selected as the root of your new content, click 'ok' below.</key>
<key alias="noNodeSelected">No node selected yet, please select a node in the list above before clicking 'ok'</key>
<key alias="notAllowedByContentType">The current node is not allowed under the chosen node because of its type</key>
<key alias="notAllowedByPath">The current node cannot be moved to one of its subpages</key>
<key alias="notAllowedByPath">The current node cannot be moved to one of its subpages neither can the parent and destination be the same</key>
<key alias="notAllowedAtRoot">The current node cannot exist at the root</key>
<key alias="notValid">The action isn't allowed since you have insufficient permissions on 1 or more child
documents.
Expand Down
5 changes: 5 additions & 0 deletions src/Umbraco.Core/Services/ContentService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2432,6 +2432,11 @@ public OperationResult MoveToRecycleBin(IContent content, int userId = Constants
/// <param name="userId">Optional Id of the User moving the Content</param>
public void Move(IContent content, int parentId, int userId = Constants.Security.SuperUserId)
{
if(content.ParentId == parentId)
{
return;
}

// if moving to the recycle bin then use the proper method
if (parentId == Constants.System.RecycleBinContent)
{
Expand Down
4 changes: 4 additions & 0 deletions src/Umbraco.Core/Services/ContentTypeServiceBaseOfTRepositoryTItemTService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -845,6 +845,10 @@ public TItem Copy(TItem original, string alias, string name, TItem? parent)
public Attempt<OperationResult<MoveOperationStatusType>?> Move(TItem moving, int containerId)
{
EventMessages eventMessages = EventMessagesFactory.Get();
if(moving.ParentId == containerId)
{
return OperationResult.Attempt.Fail(MoveOperationStatusType.FailedNotAllowedByPath, eventMessages);
}

var moveInfo = new List<MoveEventInfo<TItem>>();
using (ICoreScope scope = ScopeProvider.CreateCoreScope())
Expand Down
5 changes: 5 additions & 0 deletions src/Umbraco.Core/Services/DataTypeService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,11 @@ private void ConvertMissingEditorsOfDataTypesToLabels(IEnumerable<IDataType> dat
public Attempt<OperationResult<MoveOperationStatusType>?> Move(IDataType toMove, int parentId)
{
EventMessages evtMsgs = EventMessagesFactory.Get();
if (toMove.ParentId == parentId)
{
return OperationResult.Attempt.Fail(MoveOperationStatusType.FailedNotAllowedByPath, evtMsgs);
}

var moveInfo = new List<MoveEventInfo<IDataType>>();

using (ICoreScope scope = ScopeProvider.CreateCoreScope())
Expand Down
10 changes: 10 additions & 0 deletions src/Umbraco.Web.BackOffice/Controllers/DictionaryController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,11 @@ public ActionResult<int> Create(int parentId, string key)
return ValidationProblem(_localizedTextService.Localize("dictionary", "itemDoesNotExists"));
}

if(dictionaryItem.ParentId == null && move.ParentId == Constants.System.Root)
{
return ValidationProblem(_localizedTextService.Localize("moveOrCopy", "notAllowedByPath"));
}

IDictionaryItem? parent = _localizationService.GetDictionaryItemById(move.ParentId);
if (parent == null)
{
Expand All @@ -274,6 +279,11 @@ public ActionResult<int> Create(int parentId, string key)
}
else
{
if (dictionaryItem.ParentId == parent.Key)
{
return ValidationProblem(_localizedTextService.Localize("moveOrCopy", "notAllowedByPath"));
}

dictionaryItem.ParentId = parent.Key;
if (dictionaryItem.Key == parent.ParentId)
{
Expand Down
4 changes: 1 addition & 3 deletions src/Umbraco.Web.UI.Client/src/common/resources/contenttype.resource.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,15 +432,13 @@ function contentTypeResource($q, $http, umbRequestHelper, umbDataFormatter, loca
throw "args.id cannot be null";
}

var promise = localizationService.localize("contentType_moveFailed");

return umbRequestHelper.resourcePromise(
$http.post(umbRequestHelper.getApiUrl("contentTypeApiBaseUrl", "PostMove"),
{
parentId: args.parentId,
id: args.id
}, { responseType: 'text' }),
promise);
'Failed to move content type');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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😁 )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

},

/**
Expand Down
4 changes: 1 addition & 3 deletions src/Umbraco.Web.UI.Client/src/common/resources/mediatype.resource.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,15 +208,13 @@ function mediaTypeResource($q, $http, umbRequestHelper, umbDataFormatter, locali
throw "args.id cannot be null";
}

var promise = localizationService.localize("mediaType_moveFailed");

return umbRequestHelper.resourcePromise(
$http.post(umbRequestHelper.getApiUrl("mediaTypeApiBaseUrl", "PostMove"),
{
parentId: args.parentId,
id: args.id
}, { responseType: 'text' }),
promise);
'Failed to move media type');
},

copy: function (args) {
Expand Down