DataPicker context with non-integer "id"s (for use in UIBuilder)

[andrewmckaskill]

DataPicker works great in UIBuilder and the context works on UIBuilder entities with integer keys, but I can't use the ContentmentContext to access data about the current item on entities with Guid keys because DataPicker assumes the id is an integer.

I've had a look and think it's pretty easy, but wanted to run it past you first. Here are the changes that I think are needed (which I can do up in a PR):

In data-picker.js:

            // NOTE: [LK] Adds context of the current page, for potential future data-sources.
            // If the page is new, then it doesn't have an id, so the parentId will be used.
            config.currentPage = $scope.node || editorState.getCurrent();
            config.currentPageId = config.currentPage.id > 0 ? config.currentPage.id : config.currentPage.parentId;
            
           /** ^^^^^ this line has to change as it assumes currentPage.id is an integer **/

            // Support Content not in Content / Media Tree
            if (!config.currentPageId) {
                config.currentPageId = -1;
            }

The test to check the Id assumes the currentPage.id is an integer. For my UIBuilder pages, it's a Guid in string format. (eg. "11111111-1111-1111-1111-111111111111"). On new pages or subpages it's "00000000-0000-0000-0000-000000000000" rather than a 0.

In DataPickerApiController.cs:

public async Task<IActionResult> Search(int id, Guid dataTypeKey, int pageNumber = 1, int pageSize = 12, string query = "")
 {

and

public async Task<IActionResult> GetItems([FromQuery] int id, Guid dataTypeKey, [FromBody] string[] values)

Change the signature for the "id" field from an int to a string. It isn't being used in this method, and the ContentmentContentContext it checks if it's parseable to an int anyway, so it shouldn't be a problem.

Finally, add a new ContentmentOtherContext class and Interface that tries to access the Id element and return it as a string (or a generic if you like). Then the IContentmentOtherContext could be injected instead of the IContentmentContentContext.

I'm pretty sure that's all that's needed. Any thoughts?
(I might go ahead and try this locally anyway - just because it would make a massive difference to the project I'm working on having contextual data pickers)

[andrewmckaskill]

I'll not make a PR yet, but I've put together something that works here:
andrewmckaskill@47bb09f

I've got another branch with sufficient test data (incl. UI builder) to see that it all works nicely, but I'm going to compile and try in my main solution.

[leekelleher]

@andrewmckaskill Initial reply to acknowledge your suggestion, I'm good with the idea. 👍

I'll give it some thought on the implementation, mostly about considering the IContentmentEntityContext concept, (as Contentment wouldn't be using this internally, it'd be mostly for third-party DX).

[leekelleher]

@andrewmckaskill Finally got around to follow up on this. I have a pull request #429 ready with the tweaks that you suggested.

I'm still not convinced about the IContentmentEntityContext concept, as it feels slightly over-engineering (to have a service class), when a data-source could as easily use the IRequestAccessor to grab the "id" from the querystring, or leave it to third-party code to roll their own? I'm happy to chat more about this if I'm missing something.

Hopefully, #429 has the core changed you need.

I'll be aiming to get this in for a v5.1.0 release, just need to do some more testing with the other features I'm planning for it.

[leekelleher]

Closing off this discussion, as the non-integer ID has been implemented in PR #429.