U4-9310 - Update content, media and member pickers to store UDI instead of integers

Created by Shannon Deminick 22 Dec 2016, 05:33:03 Updated by Shannon Deminick 29 May 2017, 18:15:23

Relates to: U4-9312

Relates to: U4-9497

Relates to: U4-9498

Relates to: deploy-226

Relates to: COU-434

Subtask of: UAASSCRUM-790

Currently storing an integer value for pickers makes deployments for things like Deploy and Courier difficult since they need to be translated, also due to the way deployments works means there's a chicken/egg problem, see COU-434.

1 Attachments

Comments

Asbjørn Riis-Knudsen 27 Dec 2016, 10:37:48

Won't this be a rather big breaking change to do in a minor update? This breaks a lot of assumptions in existing code.


Shannon Deminick 28 Dec 2016, 03:15:19

We are aware of this and are working towards testing what can be done to mitigate the breaking changes among packages, we'll report back later on when things are tested and we have an idea of the impact.


Shannon Deminick 05 Jan 2017, 12:14:17

in 7.6 we could ship with 'new' Property Editors for all pickers with a new alias, we would not enable/ship with the old Integer based ones for new installs, for upgrades those property editors would be marked as deprecated in the list. Then we could have a health check or similar (if required) to perform the migration to the new ones with the UDI values. This would mean no breaking changes at all.


Shannon Deminick 01 Feb 2017, 04:35:57

PR: https://github.com/umbraco/Umbraco-CMS/pull/1721

What's been done:

  • I've changed the pickers that are relevant to UDI: Media picker, Content picker, MNTP, Member picker
  • I did not change: Member Group Picker (this stores the member group name), User Picker - I don't think these are very relevant for our needs for storing UDI - however if required we can make that happen
  • A property editor now has a new attribute that can be applied: "IsDeprecated" which means it will not show up by default in the Property Editor list unless either: ** It is already the selected on for the Data Type OR ** The <showDeprecatedPropertyEditors>true</showDeprecatedPropertyEditors> config option in umbracoSettings.config is set to true (inside of the content section)
  • The old (INT) pickers are still there, however they've been renamed and prefixed with "(Obsolete)" and they've been marked as IsDeprecated
  • The new (UDI) pickers still use the same JS/Html except they now inject a "idType='udi'" pre-value which is how the pickers determine what to store, the old ones will have "idType='int'"
  • The new (UDI) pickers have new aliases, since there's nothing fancy that can be made the aliases are suffixed with "2", like "Umbraco.ContentPicker2"
  • The underying BasicEntity that is the base model for most editor models now has a Udi property, all model mappers have been updated to map this
  • The Examine indexers have been updated to ensure that the GUID id is included in the index - this wasn't actually happening before!
  • Updated the EntityController GetByIds, GetById to support all Id types: INT, GUID, UDI and it will magically pick the correct action
  • Updated the EntityController GetByIds to support both GET and POST since requesting many UDIs might be too long of a GET request, updated the angular entityResource to use POST by default for this

To Test:

  • Create data types of the Obsolete pickers: media picker single, media picker (multiple), content picker single, MNTP, member picker
  • Create data types of the new pickers: media picker (there is only one now), content picker single, MNTP, member picker
  • Create a doc type with all of these
  • Create a content item and pick things for all properties, save it
  • reload the editor, ensure everything looks normal
  • replace all picked items, save it, reload it, ensure everything is normal
  • remove all picked items, save it, reload it, ensure everything is normal
  • pick new items, save it, reload it, ensure everything is normal
  • check database cmsPropertyValue and make sure the values are saved as you would expect with the correct formats


Warren Buckley 02 Feb 2017, 11:57:33

Updated PR with notes, questions & some trivial changes to be made @Shandem Will test this out based on your notes above, but in the meantime will watch your demo-day video.


Warren Buckley 02 Feb 2017, 14:58:10

OK marking this as Re-opened after using the build from my local copy & an AppVeyor build. I am unable to create a new datatype in order for me to complete this test (I have an empty clean build with no doctypes from Fanoe SK).

I get the following error from the response http://localhost:40550/umbraco/backoffice/UmbracoApi/ContentType/GetEmpty?parentId=-1

{
  "Message": "An error has occurred.",
  "ExceptionMessage": "\r\n\r\nMapping types:\r\nContentType -> Udi\r\nUmbraco.Core.Models.ContentType -> Umbraco.Core.Udi\r\n\r\nDestination path:\r\nDocumentTypeDisplay.Udi.Udi\r\n\r\nSource value:\r\nUmbraco.Core.Models.ContentType",
  "ExceptionType": "AutoMapper.AutoMapperMappingException",
  "StackTrace": "   at Umbraco.Web.Editors.ContentTypeController.GetEmpty(Int32 parentId)\r\n   at lambda_method(Closure , Object , Object[] )\r\n   at System.Web.Http.Controllers.ReflectedHttpActionDescriptor.ActionExecutor.<>c__DisplayClass10.<GetExecutor>b__9(Object instance, Object[] methodParameters)\r\n   at System.Web.Http.Controllers.ReflectedHttpActionDescriptor.ActionExecutor.Execute(Object instance, Object[] arguments)\r\n   at System.Web.Http.Controllers.ReflectedHttpActionDescriptor.ExecuteAsync(HttpControllerContext controllerContext, IDictionary`2 arguments, CancellationToken cancellationToken)\r\n--- End of stack trace from previous location where exception was thrown ---\r\n   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)\r\n   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)\r\n   at System.Web.Http.Controllers.ApiControllerActionInvoker.<InvokeActionAsyncCore>d__0.MoveNext()\r\n--- End of stack trace from previous location where exception was thrown ---\r\n   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)\r\n   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)\r\n   at System.Web.Http.Filters.ActionFilterAttribute.<CallOnActionExecutedAsync>d__5.MoveNext()\r\n--- End of stack trace from previous location where exception was thrown ---\r\n   at System.Web.Http.Filters.ActionFilterAttribute.<CallOnActionExecutedAsync>d__5.MoveNext()\r\n--- End of stack trace from previous location where exception was thrown ---\r\n   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)\r\n   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)\r\n   at System.Web.Http.Filters.ActionFilterAttribute.<ExecuteActionFilterAsyncCore>d__0.MoveNext()\r\n--- End of stack trace from previous location where exception was thrown ---\r\n   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)\r\n   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)\r\n   at System.Web.Http.Filters.ActionFilterAttribute.<CallOnActionExecutedAsync>d__5.MoveNext()\r\n--- End of stack trace from previous location where exception was thrown ---\r\n   at System.Web.Http.Filters.ActionFilterAttribute.<CallOnActionExecutedAsync>d__5.MoveNext()\r\n--- End of stack trace from previous location where exception was thrown ---\r\n   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)\r\n   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)\r\n   at System.Web.Http.Filters.ActionFilterAttribute.<ExecuteActionFilterAsyncCore>d__0.MoveNext()\r\n--- End of stack trace from previous location where exception was thrown ---\r\n   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)\r\n   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)\r\n   at System.Web.Http.Filters.ActionFilterAttribute.<CallOnActionExecutedAsync>d__5.MoveNext()\r\n--- End of stack trace from previous location where exception was thrown ---\r\n   at System.Web.Http.Filters.ActionFilterAttribute.<CallOnActionExecutedAsync>d__5.MoveNext()\r\n--- End of stack trace from previous location where exception was thrown ---\r\n   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)\r\n   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)\r\n   at System.Web.Http.Filters.ActionFilterAttribute.<ExecuteActionFilterAsyncCore>d__0.MoveNext()\r\n--- End of stack trace from previous location where exception was thrown ---\r\n   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)\r\n   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)\r\n   at System.Web.Http.Controllers.ActionFilterResult.<ExecuteAsync>d__2.MoveNext()\r\n--- End of stack trace from previous location where exception was thrown ---\r\n   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)\r\n   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)\r\n   at System.Web.Http.Filters.AuthorizationFilterAttribute.<ExecuteAuthorizationFilterAsyncCore>d__2.MoveNext()\r\n--- End of stack trace from previous location where exception was thrown ---\r\n   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)\r\n   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)\r\n   at System.Web.Http.Filters.AuthorizationFilterAttribute.<ExecuteAuthorizationFilterAsyncCore>d__2.MoveNext()\r\n--- End of stack trace from previous location where exception was thrown ---\r\n   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)\r\n   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)\r\n   at System.Web.Http.Filters.AuthorizationFilterAttribute.<ExecuteAuthorizationFilterAsyncCore>d__2.MoveNext()\r\n--- End of stack trace from previous location where exception was thrown ---\r\n   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)\r\n   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)\r\n   at System.Web.Http.Dispatcher.HttpControllerDispatcher.<SendAsync>d__1.MoveNext()",
  "InnerException": {
    "Message": "An error has occurred.",
    "ExceptionMessage": "Object reference not set to an instance of an object.",
    "ExceptionType": "System.NullReferenceException",
    "StackTrace": "   at Umbraco.Core.Udi.<.cctor>b__0()\r\n   at System.Lazy`1.CreateValue()\r\n--- End of stack trace from previous location where exception was thrown ---\r\n   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()\r\n   at System.Lazy`1.get_Value()\r\n   at Umbraco.Core.Udi.Create(String entityType, Guid id)\r\n   at Umbraco.Web.Models.Mapping.ContentTypeUdiResolver.ResolveCore(IContentTypeComposition source)\r\n   at AutoMapper.ValueResolver`2.Resolve(ResolutionResult source)\r\n   at AutoMapper.PropertyMap.<ResolveValue>b__6(ResolutionResult current, IValueResolver resolver)\r\n   at System.Linq.Enumerable.Aggregate[TSource,TAccumulate](IEnumerable`1 source, TAccumulate seed, Func`3 func)\r\n   at AutoMapper.PropertyMap.ResolveValue(ResolutionContext context)\r\n   at AutoMapper.Mappers.TypeMapObjectMapperRegistry.PropertyMapMappingStrategy.MapPropertyValue(ResolutionContext context, IMappingEngineRunner mapper, Object mappedObject, PropertyMap propertyMap)"
  }
}


Shannon Deminick 03 Feb 2017, 00:51:45

Hey @warren.buckley I've updated a bunch of stuff with regards to your review comments. I've added a null check to the ContentTypeUdiResolver which is where your error is coming from above, however i cannot replicate this issue whatsoever so can you please try this by: Getting latest changes from Git on this branch, open the VS project and run from there (instead of running the build locally and testing that ... if you do that everytime for testing, that's a lot of extra work!). Otherwise if you have more steps to reproduce please let me know.


Warren Buckley 06 Feb 2017, 14:27:32

OK cant repro the DocType creation error @Shandem So it's all good, generally don't test by using the build script output unless I experience any unexpected errors or weird stuff.

The new pickers save UDI values all OK. The one minor problem I have and cant understand why it happens is that for the Content Picker & the Member Picker the new UDI picker still uses the old obsoleted property editor under the hood (as it displays the old alias, without the 2 suffix).

I can manually change these built in DataTypes that get setup out of the box from a clean install & rectify the problem, but other users are gonna assume that the non obsolete one is in fact using the non-obsolete UDI version.

I am bit puzzled as to why this happens. Does the initial DB creation insert some old values & reset it back or something?

My test was with a clean SQL CE DB using the RevertToEmptyInstall batch script. Any thoughts or would you like me to re-test again @Shandem ?


Shannon Deminick 07 Feb 2017, 01:06:06

@warren.buckley great reviewing mate and good that you found this! I totally forgot to update the base data creation. All is good now (i think!) and I've fixed up the unit tests too, please make sure that they are working too (I've tested them)


Per Ploug 07 Feb 2017, 11:08:40

What about prevalues - does these support UDI - and also, what about the link/url picker (it can pick internal links) does it support UDI?


Warren Buckley 07 Feb 2017, 11:16:45

I will check for you shortly @pploug as I am testing this currently.


Warren Buckley 07 Feb 2017, 13:43:14

@Shandem I can confirm the changes you made now fix up the pickers to use the new UDI versions when the default datatypes are created out of the box.

I have added a note on the GitHub PR itself about a failing test that needs fixing up please, I wasn't 100% confident on the issue to help rectify it, so if you can look at it that would be great.

I have marked the Macro Picker aka Macro Container datatype as deprecated as suggested from your Slack message Shan, but we need to consider then if we need to make a UDI version of this picker as well, looking at what the current one stores is a formatted string of macro syntax like so, would seem like we don't.

<?UMBRACO_MACRO macroAlias="Quote" /><?UMBRACO_MACRO macroAlias="AnotherMacro" />

But if we are marking this as obsolete/deprecated then with your new config value it would not show up in new/clean installs at all. Is this because we feel this is not used much and needs to be retired?

In regards to Per's notes I have checked and yes we will need to add/do the following:

  • Add a new UDI picker version of the Link Picker - Umbraco.RelatedLinks
  • Fixup PreValues to use UDI values/pickers - Example the new UDI Media Picker stores a UDI for the picked items, but when configuring the property editor we can set a start node which is currently an int & not a UDI to a picked media start node

Based on these notes - marking as re-opened. Third time lucky :)


Shannon Deminick 08 Feb 2017, 01:54:56

@pploug good feedback, I will have to create new tasks for the link/url picker and having to deal with pre-values - this doesn't take those into account.


Shannon Deminick 08 Feb 2017, 02:59:49

I'll update these conent,media and member property editors that have pre-values as INT to use UDI but other property editors that store ids will need to be updated separately: http://issues.umbraco.org/issue/U4-9498, then Deploy (and courier) would need to be updated to support all of this: http://issues.umbraco.org/issue/deploy-226


Shannon Deminick 08 Feb 2017, 05:19:15

@warren.buckley Ok, few more changes here. The tests all pass now (except JS tests but I've asked Mads about that, i don't think it's my changes that broke those)

  • These new UDI pickers also support storing UDIs for pre-values for their start node option
  • This has a knock on effect in that we need to be able to inject some server side config for pre-value editors (otherwise we'd have to make diff udi prevalue editors but that isn't very re-usable), so have implemented that and then updated all of the logic to deal with the udi idType

For testing:

  • follow the same tests listed above
  • then change the start nodes in pre-values for the pickers and verify that this functionality works (i.e. the tree picker will only show children from the node selected)
  • verify that the pre-values stored are UDIs in the [cmsDataTypePreValues] table


Warren Buckley 09 Feb 2017, 10:37:27

Getting another error when doing testing @Shandem When creating a new document type in a clean install and choosing the content picker (UDI new version) I get the following error:

Multiple actions were found that match the request: GetByIds on type Umbraco.Web.Editors.EntityController GetByIds on type Umbraco.Web.Editors.EntityController GetByIds on type Umbraco.Web.Editors.EntityController

   at System.Web.Http.Controllers.ApiControllerActionSelector.ActionSelectorCacheItem.SelectAction(HttpControllerContext controllerContext)
   at System.Web.Http.Controllers.ApiControllerActionSelector.SelectAction(HttpControllerContext controllerContext)
   at Umbraco.Web.Editors.ParameterSwapControllerActionSelector.SelectAction(HttpControllerContext controllerContext) in C:\Code\Umbraco-CMS\src\Umbraco.Web\Editors\ParameterSwapControllerActionSelector.cs:line 93
   at System.Web.Http.ApiController.ExecuteAsync(HttpControllerContext controllerContext, CancellationToken cancellationToken)
   at System.Web.Http.Dispatcher.HttpControllerDispatcher.<SendAsync>d__1.MoveNext()

Marking as re-opened.


Shannon Deminick 09 Feb 2017, 23:49:10

@warren that is all fixed up now, cheers


Warren Buckley 10 Feb 2017, 12:12:57

@Shandem OK we are painfully close now, but still ran into a problem (after a few retests to be 100% certain)

Media Picker with a prevalue Start Node saves as a UDI and resolved correctly when used on content node, but I can't say the same for the Content Picker.

When trying to pick a content node for the start node, the dialog is entirely empty, no 404s or 500's. It seems that it does not even make a request to get content items to populate the dialog.

Where the media picker for the start node prevalue is making requests to populate it. Will do a little more digging for a bit to see if I can spot it & just update it, as I want to get this merged in.

If I can't find it after time-boxing this for a bit, expect this to come back again Shan.


Warren Buckley 10 Feb 2017, 13:36:21

OK trying to go down the rabbit hole of the PreValue TreePicker calling the Dialog TreePicker which calls the Tree component/directive and not seeing anything screamingly obvious as to why I am not getting any results for content nodes when trying to setup the content picker.

As I have spent a bit of time on this, I am going to put this back in re-open so hopefully you may find the issue a-lot quicker than me. If I can set a content picker start node then I would say then this is most likely then all fixed up & ready to go @Shandem


Shannon Deminick 13 Feb 2017, 09:07:51

Ok @warren.buckley i think we're good :) ( hope so!), the issue was due to a mistake I made when cleaning up some code, the key used to display the 'section' i thought was the same name when merging the config sources, but it wasn't. Have pushed the fix.


Warren Buckley 14 Feb 2017, 11:39:13

Yay!!! All tested & working and will merge this in.

Had to fix up Media & Member picker being inserted into the DB with the wrong names at the init db creation of DataTypes we ship with.


Shannon Deminick 15 Feb 2017, 04:02:47

@warren.buckley amazing work mate, much appreciated :)


Priority: Normal

Type: Feature (request)

State: Fixed

Assignee:

Difficulty:

Category:

Backwards Compatible: False

Fix Submitted:

Affected versions:

Due in version: 7.6.0

Sprint: Sprint 52

Story Points: 4

Cycle: