Added copy functionality for data types

[patrickdemooij9]

This is a feature that I've been wanting for quite some time now. Sometimes we have blocklists that we have to copy over, but we just aren't able to do that. So, I took a look at how this is currently done with document types and managed to get it working for the datatypes:

[umbrabot]

Hi there @patrickdemooij9, 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:

• [x] 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
• [x] The automated tests all pass (see "Checks" tab on this PR)
• [x] The level of security for this contribution is the same or improved
• [x] The level of performance for this contribution is the same or improved
• [x] Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
• [x] If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
• [x] 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 🤖 🙂

[bjarnef]

Would be useful with more complex datatypes like Grid and Block List editor. It will truly be a timesaver in many projects. https://github.com/umbraco/Umbraco-CMS/discussions/11388

[nathanwoulfe]

Hey @patrickdemooij9 this looks pretty slick, but given the change to the interface it constitutes a breaking change so would need to target v10 rather than v9. I can try changing the base branch and see what happens, but might be better for you to do it locally, then create a new PR (changing the base branch in GH can do strange things to histories)

Apologies for making more work for you!

[patrickdemooij9]

@nathanwoulfe, so basically closing this PR and then opening a new one towards the v10 branch? Or should I first merge the v10 towards my own branch?

[nul800sebastiaan]

Just thinking out loud, would it be feasible to make the required changes without updating the interfaces now, we can save the interfaces to v10.

Wouldn't that be okay, the functionality works but not necessarily for anyone who might be implementing the interfaces, they'd need to know to add the extra methods as well, but is that a bad thing? Seems fine to me.. ? The implementers will just not have the copy functionality..

[patrickdemooij9]

@nul800sebastiaan, might be possible but then we'll need to cast the interface to the actual service right? As we won't be able to call the function on the interfaces due to not updating it? Or am I missing something here?

[nathanwoulfe]

Can't remember where, but there are instances where a new interface has been added to avoid modifying the existing one, with the new interface extending the old. Could then do something like:

public interface IDataTypeService2 : IDataTypeService {
    void NewMethod()
}

The new interface is the one registered by Umbraco, leaving the old one unchanged for downstream implementations. Then come v10, it's just a matter of merging the two interfaces.

[nul800sebastiaan]

@patrickdemooij9 with regards to the breaking change - we fixed this in a recent PR by using a default interface: https://github.com/umbraco/Umbraco-CMS/pull/12218#issuecomment-1089981684

It was a new concept for me so that caused some confusion but it works like a charm! Let me know if you need some help with that. Love the feature, so it would be great to finish it up! 👍

[patrickdemooij9]

@nul800sebastiaan, yeah that should help a lot with breaking changes. However, how would I implement it here? There is no old interface code that I should call as I am only adding a method to the interface

[Zeegaan]

@nul800sebastiaan, yeah that should help a lot with breaking changes. However, how would I implement it here? There is no old interface code that I should call as I am only adding a method to the interface

@patrickdemooij9 You would just add the actual functionality to the interface, like this on the IDataType interface:

IDataType DeepCloneWithResetIdentities()
{
    var clone = (DataType)DeepClone();
    clone.Key = Guid.Empty;
    clone.ResetIdentity();
    return clone;
}

[patrickdemooij9]

@nul800sebastiaan, yeah that should help a lot with breaking changes. However, how would I implement it here? There is no old interface code that I should call as I am only adding a method to the interface

@patrickdemooij9 You would just add the actual functionality to the interface, like this on the IDataType interface:

IDataType DeepCloneWithResetIdentities()
{
    var clone = (DataType)DeepClone();
    clone.Key = Guid.Empty;
    clone.ResetIdentity();
    return clone;
}

How would I go about the one in the IDataTypeService as that one is using DI? It also kinda confuses me how this is a breaking change as I am not updating any existing code. Is it because the interface needs to recompile or something like that?

[Zeegaan]

@patrickdemooij9 I'd probably do something like this: Attempt<OperationResult<MoveOperationStatusType, IDataType>?> Copy(IDataType copying, int containerId) => throw new NotImplementedException(); So the default implementation will make it throw an expection, but that seems reasonable, because no one should be referencing a brand new method in their code yet 😅

And yes, adding a method to an interface is a breaking change, because lets say you have a class that implements the IDataTypeService interface, you will get an error saying you haven't implemented a method from the interface when you upgrade to the newest version

[Zeegaan]

Microsoft has a great article about the different types of breaking changes: https://docs.microsoft.com/en-us/dotnet/standard/library-guidance/breaking-changes#types-of-breaking-changes

[patrickdemooij9]

@Zeegaan I totally forgot about people implementing this interface themselves. I added the code to the default interface and that seems to all work well.

Is this all? Ideally you would want to get rid of the default interface logic eventually right? So is there any other step that I should take/add?

[mikecp]

Hey @patrickdemooij9 , Thanks for this PR, it will be a great addition 😉 . I had a look and posted a few minor points/questions as well. There are also a few conflicts due to the recent merge of v10/dev (10.1) into v1/contrib, I hope they aren't too complex 😬 Cheers!

[patrickdemooij9]

Hi @mikecp I'll make sure to fix the feedback points when I get back from vacation, so it might take a bit before an update is done on this

[mikecp]

Hey @patrickdemooij9 , Sure no prob, I don't think we're up to a few days on this one 😂 Enjoy your vacation!! 🎉🏖🌞

[patrickdemooij9]

@mikecp All back from vacation and the PR has been updated along with the merge conflicts solved

[mikecp]

Hey @patrickdemooij9 ,

Hope you had great vacation 😉🏖

Thanks for the update! I have checked it and almost all works fine: I just noticed that the tree does not refresh automatically at the end like in your video, so, currently I have to manually reload in order to view the copied element. Could you maybe check if you also have that issue n now? It might be a local issue on my PC but maybe this could be an unexpected side-effect from the merge? Thanks for checking!

[patrickdemooij9]

Hey @patrickdemooij9 ,

Hope you had great vacation 😉🏖

Thanks for the update! I have checked it and almost all works fine: I just noticed that the tree does not refresh automatically at the end like in your video, so, currently I have to manually reload in order to view the copied element. Could you maybe check if you also have that issue n now? It might be a local issue on my PC but maybe this could be an unexpected side-effect from the merge? Thanks for checking!

Yeah, vacation was great, thank you!

As for the issue that you are having, it is not caused by this PR, but by a different one (it also happens on the contrib branch). I created an issue for it here: https://github.com/umbraco/Umbraco-CMS/issues/12841

[mikecp]

OK, thanks for the quick feedback @patrickdemooij9. Then there is no more reason to wait for merging this😁 Thanks for the nice addition 👍

[nul800sebastiaan]

I do apologize @patrickdemooij9, we were checking the 10.2 RC and it turns out the RC branch was created just before this was merged, so it didn't quite make it into 10.2.0 yet. 😅 I'm going to have to move this one to 10.3.0 instead, so a few more weeks of patience. Sorry about that!

[KevinJump]

Hi, just taking a look, I don't think this process fires any form of notification when a copy happens. (Unless I've missed something in the code)

I think it should fire at least a save notification (for the new copy?) or something new? - either way the copy can't be tracked if something isn't fired

[patrickdemooij9]

Good point @KevinJump I thought I had made sure that it would throw a notification, but apparently not! I've fixed that in this PR: https://github.com/umbraco/Umbraco-CMS/pull/11867