Content cloning and translation

[TFleury]

Initial PR : #6540
This PR adds new clone handling to replace export/import logic, and also allow translating a clone of the master content item.

It fixes : #5535, #5089, #4983, #4970, #4538, #5031

[Skrypt]

Tested and works. I need to review if we could use ILocalizableAspect in some places.

[Skrypt]

Going forward, I've set every content types to be listable in Orchard to see if it could uncover missing use cases.

Here is what can/cannot be cloned :

• Custom Content Type with all types of fields
• Page
• Layout
• Taxonomies
  • Taxonomies with it's terms
• Blog
  • Description, other configurations
• Blog Post
  • Description, other configurations
• Comment
  • The clone link is there but it opens a blank form
• Query
  • Queries are not listable even if the option is checked
• Projection
  • Show on a menu
  • Show on admin menu
• Widget
  • HTML, Text ... none of them can be cloned. You can set them as listable but they will only display their titles
• Menu
  • Menu with it's content
• Media Profiles
  • Media Profiles with filters
• Workflows

You get the idea... there's probably more but I think this PR replaces what was already there and it does'nt break anything.

[sebastienros]

Merge conflict ... sorry

[TFleury]

Rebased

[sebastienros]

And a squash would be appreciated too.

@Skrypt how did you tests go with this change?

[Skrypt]

Give me some time I'll test this soon after I get my cup of coffee.

[jtkech]

@TFleury

Working on field drivers, i noticed that with the boolean field you can use a neutral option in settings, e.g Maybe. So, if you select Maybe, then !field.Value.HasValue will be true without meaning it's a new content item. This because here null is considered as a valid value.

If, for another reason, you still need to change this test, i think we can find the right one.

Best

[TFleury]

You are right, need to find another way to initialize whith default value without overriding the cloned one.

[Skrypt]

This works "and tested" :

        protected override DriverResult Editor(ContentPart part, BooleanField field, dynamic shapeHelper) {

            string action = HttpContext.Current.Request.RequestContext.RouteData.Values["action"].ToString().ToLowerInvariant();

            // if the content item is new, assign the default value
            if (action == "create") {
                var settings = field.PartFieldDefinition.Settings.GetModel<BooleanFieldSettings>();
                field.Value = settings.DefaultValue;
            }

            return ContentShape("Fields_Boolean_Edit", GetDifferentiator(field, part),
                () => shapeHelper.EditorTemplate(TemplateName: TemplateName, Model: field, Prefix: GetPrefix(field, part)));
        }

[jtkech]

Didn't seen the details of cloning, but, as i understand, we can't use the previous test because we work on a content item that is being cloning, and therefore has a draft or is published...

So, after a 1st look, and because i think this method is called only during a regular web request, your code looks good for me. Maybe we need a null check on the action string?

Best

[Skrypt]

The main problem here is that there is no way to know if the Boolean Field has been assigned a null value "explicitly"... since a null value is a null ...

[jtkech]

You're right, i was talking about the action string, and, indeed, here the null check is useless ;)

[Skrypt]

We need to adapt my code to use IHttpContextAccessor instead of HttpContext wich is pre .NET 3.5

[jtkech]

You're right, by injecting an IHttpContextAccessor and using _httpContextAccessor.Current(). Then, one rule is to also use this mvc extension if (!httpContext.IsBackgroundContext()) when it doesn't make sense to do something, e.g in a background task. But i think that the Editor() method will never be executed in the background (unlike the Display() method). Anyway, in the background an empty (but not null) route data would be returned.

So, not tested but i propose something like the following:
Note: maybe good to create a route extension GetActionName() as already done for GetAreaName().

        ...
        var routeData = _httpContextAccessor.Current().Request.RequestContext.RouteData;

        object action;
        if (routeData.Values.TryGetValue("action", out action)) {
            var actionString = action as string;

            // if the content item is new, assign the default value
            if (!String.IsNullOrEmpty(actionString) && actionString.Equals("create", StringComparison.OrdinalIgnoreCase)) {
                var settings = field.PartFieldDefinition.Settings.GetModel<BooleanFieldSettings>();
                field.Value = settings.DefaultValue;
            }
        }
        ...

[jtkech]

About my line note on BooleanFieldDriver.cs, without disturbing you, if you can tell me why you have to change this test, i could try to find a compromise.

Best.

[Skrypt]

@sebastienros All the tests I've made have the same results as before.

[Skrypt]

It is GTG if we fix the small bug with the boolean field.

[Skrypt]

I think we should also provide unit tests on this one. At least one for making sure that the identityPart generates a new GUID.

[TFleury]

I think there is an issue with all fields (not bound to cloning) :
The default value of the field is set in Editor method of the field's driver.
If you create a content item without editing it, or if you add a field to a ContentType with existing content items, they won't take the default value until they are edited and saved.
Default value should be set in a ContentHandler on Initializing handler using a LazyField. On Loading, the "default value" getter will be replaced with a "stored value" getter (if there is one).

Do you agree with that ?

[Skrypt]

I agree. Though, it is not a ship blocker for this PR. If we need to implement such thing, let's propose it in another issue. Else we are going East and West with this one.

[TFleury]

But I think the PR won't be accepted with a condition on Action name...

[Skrypt]

This is a grey zone. Implement it and let's summon @sebastienros

[sebastienros]

I assume either a default value should be implemented for Editor only, or in a Content Handler when an item is created.

[sebastienros]

Well, it depends what we think of the default value. Like the first value that is shown to the user, but the user could delete it. Or the value to assign when there is no value, on save.

[Skrypt]

Not everyone will want their TextboxField to have forced default values saved when an empty string is submitted by the form. I think that the default values should be only proposed in that scenario.

Though, if it comes from a background task it should force default values. So IMO it's kind of both.

[jtkech]

Indeed, as @sebastienros said there could be different meanings around default values. Initial value on creation, predefined value on editing, default value on submission...

But, in our context, note that currently most of the fields don't use a default value on first editing / creation. As i remember, only the enum and bool fields use it. And the boolean field is a special case where null can be a valid value (if the neutral option is set).

I agree with what @TFleury said about content handler and lazy fields. Or, maybe, by adding a new field driver "event" for this purpose, as done for cloning, through ContentFieldDriverCoordinator...

That said, not tested but isn't there any solution by using, in the BooleanFieldDriver, the new Cloning() / Cloned() "events" to set a private variable that we can check & maybe reset in the 1st Editor() method?

Best.

[Skrypt]

Here is the result of my search on how Drupal does it.
https://www.drupal.org/project/replicate (background API)
https://www.drupal.org/project/node_clone (UI)

[jtkech]

@sebastienros @TFleury @Skrypt

Just tried, great feature! Didn't test everithing because Jasmin did it.

So, if you can check this simple solution in BooleanFieldDriver.cs that seems to work for me:

...
private bool _isCloning = false; // new
....
    protected override DriverResult Editor(ContentPart part...) {
        ....
        // if (!part.HasDraft() && !part.HasPublished()) { // before
        if (!part.HasDraft() && !part.HasPublished() && !_isCloning) { // and now
        ....
    }
    ....
    protected override void Cloning(ContentPart part, BooleanField originalField....) {
        cloneField.Value = originalField.Value;
        _isCloning = true; // new
    }

Best.

[vesko-k]

2 questions

1. When cloning what happens with the owner of the cloned item?
2. When cloning content item, if regeneration of the Identifier of IdentityPart should be done on the source or result item?

[Skrypt]

Right now the clone feature opens up a "Create new" form and sets the owner to the current user. The new content item should have then a new IdentityPart since it has the same logic than "Create". So it is done on the created item (Parts). Though we would need to assert that if this is done by a background task that it also changes the current owner properly.

[TFleury]

@sebastienros which is the best (acceptable) way to solve our problem with default value handling ?

• Condition on action name
• Setting a variable in Driver
• Using ContentHandler and LazyField

[Skrypt]

From a discussion with @sebastienros

This PR depends on the behavior of the "default values". We are not sure about what behavior we want for "default values". So we need to fix the "default values" issue first.

I think Fields should mimic what we can do with Fields in SQL Server. So, my opinion is that default values should only be proposed.

Fields can be attached to a content type even when this content type has content items. If we set a default value to this new field, we don't need to batch update all the content items since the default value can be retrieved by the Field properties. Also, if we export/import those content items, and we decide to ommit this field in the import, I don't think we should then set this field to it's default value.

to continue ...

[jtkech]

Not sure that all we can discuss around default values should block this PR. I thought cloning was not directly concerned by this, and, here, we just needed to fix a specific case with the boolean field editor.

About default values we have to separate contexts where different values can be used or not. So, here, for clarity, just more infos on the current implementation related to default form values for field editing.

1. For most of the fields, the default value (if set) is used on submission if the field is left empty. I think this one is good, consistent with e.g the auto generated autoroute slug if left empty. Note: i think it's good to separate default values for editing and for other contexts.
2. For some fields, the default value (here the same) is also used on 1st edition as a proposed value. I think it has been introduced only to adapt some fields to 1. because some controls always send a value (can't be left empty). So, the need to preset and submit the default if no user action.

• So, when saying we don't want to have forced default values when an empty string is submitted and default values should only be proposed, we would need to remove the main point 1. and implement 2. for all fields. For me it's ok as it is but i've no closed opinion. Maybe we can have 1. and 2. with additional options to use them alone or both.

Anyway, regarding default values, the only problem i see here is with the boolean field because null can be a valid value (Neutral), so, when 2. is used we can't detect if it has no value as in a 1st edition. Maybe we can first find a compromise for this (see above the list suggested by Thierry).

Best.

[TFleury]

Just updated my PR.
It should not conflict with #6720

[sebastienros]

Will merge as someone says it works on dev.

[Skrypt]

This PR waiting on me to re-re-test it ? 😉

[sebastienros]

Let's watch Orchard burning ...

[Skrypt]

Oops ! it did not close the thousands issues related to it 😄

[MatteoPiovanelli-Laser]

In light of #7352 I pulled the latest commit related to cloning into a (local) branch off 1.10.x.
I added a few comments in that same commit asking for clarification on some choices in the code.
I hope some one can answer those. In the meantime, I will verify what is broken by the changes here, and a few modifications to the code from the commit to make it so it breaks nothing.

The commit is 87a3948

/cc @TFleury @Skrypt @sebastienros

[TFleury]

@MatteoPiovanelli-Laser thank you for your comments, I tried to answer all of them.
Cloning introduces some breaking changes, so it should not be in 1.10.x but in a new 1.11 branch.
I will push PR to solve some problems you have highlighted.