#7412: Verified and added several Cloning overrides in many drivers #7592
Conversation
src/Orchard.Web/Modules/Orchard.Autoroute/Drivers/AutoroutePartDriver.cs
Show resolved
Hide resolved
|
Looks good but we need someone to confirm it works. /cc summoning @BenedekFarkas |
|
I'll be happy to do testing on this and the related PRs, but won't be able to do so in the next few days. I'll add myself as a reviewer just in case, so it shows up for me on GitHub. |
…Cleanup # Conflicts: # src/Orchard.Web/Core/Contents/Views/Parts.Contents.Publish.SummaryAdmin.cshtml # src/Orchard.Web/Modules/Orchard.MediaLibrary/Drivers/AudioPartDriver.cs # src/Orchard.Web/Modules/Orchard.MediaLibrary/Drivers/DocumentPartDriver.cs # src/Orchard.Web/Modules/Orchard.MediaLibrary/Drivers/MediaPartDriver.cs # src/Orchard.Web/Modules/Orchard.MediaLibrary/Drivers/OEmbedPartDriver.cs
BenedekFarkas
changed the title
|
@MatteoPiovanelli-Laser please merge from dev, I'm testing/reviewing this now. How does this relate to #7409 BTW? |
| return RedirectToRoute(adminRouteValues); | ||
| if (string.IsNullOrWhiteSpace(returnUrl)) { | ||
| var adminRouteValues = _contentManager.GetItemMetadata(cloneContentItem).AdminRouteValues; | ||
| return RedirectToRoute(adminRouteValues); |
There was a problem hiding this comment.
After you merge from dev, please adjust Core/Contents/Views/Parts.Contents.Clone.SummaryAdmin.cshtml and Parts.Contents.Publish.SummaryAdmin.cshtml (line 29) not to include the ReturnUrl parameter.
| protected override void Cloning(MenuPart originalPart, MenuPart clonePart, CloneContentContext context) { | ||
| clonePart.MenuText = originalPart.MenuText; | ||
| clonePart.MenuPosition = originalPart.MenuPosition; | ||
| clonePart.Menu = context.CloneContentItem; |
There was a problem hiding this comment.
context.CloneContentItem causes the "Show on a menu" to always be checked incorrectly.
| clonePart.Menu = context.CloneContentItem; | |
| clonePart.Menu = originalPart.Menu; |
| protected override void Cloning(ImageProfilePart originalPart, ImageProfilePart clonePart, CloneContentContext context) { | ||
| clonePart.Name = originalPart.Name; | ||
| clonePart.ModifiedUtc = originalPart.ModifiedUtc; | ||
| clonePart.Record.Filters = originalPart.Filters; |
There was a problem hiding this comment.
Throws YSOD:
Don't change the reference to a collection with cascade="all-delete-orphan"
Filters are records (and thus references), so they should be cloned too.
| clonePart.Name = originalPart.Name; | ||
| clonePart.ModifiedUtc = originalPart.ModifiedUtc; | ||
| clonePart.Record.Filters = originalPart.Filters; | ||
| clonePart.Record.FileNames = originalPart.FileNames; |
There was a problem hiding this comment.
Do we really need to clone these? Shouldn't this start empty, since the new profile doesn't have any images generated?
| } | ||
|
|
||
| protected override void Cloned(NavigationQueryPart originalPart, NavigationQueryPart clonePart, CloneContentContext context) { | ||
| clonePart.QueryPartRecord = originalPart.QueryPartRecord; |
There was a problem hiding this comment.
Just curious, why is this in Cloned and not Cloning?
| clonePart.Record.SortCriteria = originalPart.SortCriteria; | ||
| clonePart.Record.FilterGroups = originalPart.FilterGroups; | ||
| clonePart.Record.Layouts = originalPart.Layouts; |
| foreach (var addingRole in origRoleRecords.Where(x => !cloneRoleRecords.Contains(x))) { | ||
| _userRolesRepository.Create(new UserRolesPartRecord { UserId = clonePart.ContentItem.Id, Role = addingRole }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Should we even allow cloning Users? Do you know of a good use-case here?
|
|
||
| protected override void Cloning(SearchSettingsPart originalPart, SearchSettingsPart clonePart, CloneContentContext context) { | ||
| clonePart.SearchFields = originalPart.SearchFields; | ||
| } |
There was a problem hiding this comment.
These are site settings and the Site shouldn't be cloned.
Fixes #7412
I am not sure all the overrides are correct even here.
I added the Cloning override in some of the drivers that did not have it as well.