Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure both content and media support fetching entities by ids array of Guid and Udi #9465

Closed

Conversation

bjarnef
Copy link
Contributor

@bjarnef bjarnef commented Nov 28, 2020

Prerequisites

  • I have added steps to test this contribution in the description below

If there's an existing issue for this PR then this fixes #9460

Description

This PR makes it possible for contentResource and mediaResource to fetch entities by int[], Guid[] and Udi[] like entityResource.

I noticed we have a GetByIds(this IContentService contentService, IEnumerable<Udi> ids) in ContentServiceExtensions to extend ContentService with GetByIds(IEnumerable<Udi> ids) without changing the interface as it would a breaking change. Similar method exists for media, but it wasn't included in the project. It is now.

I have changed getByIds in contentResource and mediaResource to POST request similar to getByIds in entityResource.

Here is an example fetching entities using Guid[] and getByIds in the three different services.

image

In have added the dashboard as a zip-file to test with:
MyDashboard.zip

@bjarnef
Copy link
Contributor Author

bjarnef commented Nov 28, 2020

It would also be great to support this in getByIds in memberGroupResource, but it seems to require some changes in IMemberGroupService and IMemberGroupRepository as well to allow fetching member group nodes by Guid.

@bjarnef
Copy link
Contributor Author

bjarnef commented Jun 28, 2021

@nul800sebastiaan I just tried based on an Umbraco v8.13.1 project to include Umbraco.Core.Services.
Unfortunately MediaServiceExtensions.cs hasn't been included in the project, so the extension GetByIds(Udi[] udis) isn't available.

image

@nul800sebastiaan
Copy link
Member

Am I missing something? This PR is still open so yeah, it won't have the changes you made. :)

@bjarnef
Copy link
Contributor Author

bjarnef commented Jun 28, 2021

@nul800sebastiaan yeah this PR enhancd a bit around GetByIds via Udi's ... however this file should be safe to include :)
https://github.com/umbraco/Umbraco-CMS/pull/9465/files#diff-241a5f45f2f7a63af9c68058564c5c2f7b78e414ac6be46aebde98cd8e3d106eR194

From what I can find this was added a long time ago
d546580#diff-0703ef93a614d4373794998ca03f46e6c6703b6abe0cdfaecf5642f36f0ea30e but never included in the project. (which also introduces the GetByIds(IEnumerable<Udi> ids) and CreateContent(string name, Udi parentId, string mediaTypeAlias) in ContentServiceExtensions.cs.

It is however very similar to the ContentService extensions here:
https://github.com/umbraco/Umbraco-CMS/blob/v8/contrib/src/Umbraco.Core/Services/ContentServiceExtensions.cs

@bjarnef
Copy link
Contributor Author

bjarnef commented Aug 21, 2021

@nul800sebastiaan I think this is one of the fra missing parts to access by Guid and Udi.. and furthermore it would allow to access more content and media.

@bjarnef
Copy link
Contributor Author

bjarnef commented Sep 22, 2021

@nul800sebastiaan I appreciate many new PR's are reviewed and merged, but it seems many older PR's are a bit forgotten?

There are still a handful PR from last year hacktoberfest that hasn't been reviewed. Maybe some ser not relevant now and could be closed, there's a better way now or it makes sense to merge?

Maybe PR's should have a label based on the complexity or priority? E.g. some are a minor typo, localization or styling change, while other are regarding performance, security, new feauture which may contains hundreds or thousands of changes.

@umbrabot
Copy link

umbrabot commented Nov 8, 2021

Hiya @bjarnef!

We wanted to thank you for your work and let you know that we have spent some time reviewing the contribution and have decided that we are going to close this one.

While reviewing this PR, the team left the following additional note:

Unfortunately contains breaking changes, and missing a file from the commit. Would love functionality, but in v9+ instead.

We appreciate that you have worked hard to build and document this contribution to create the changes you want to see but given the status of Umbraco 8, moving into LTS and Umbraco 9 being ready and stable and packed with new features, we are working through the open PRs and deciding which are a good fit for the last version 8 minor and which are not.

If you'd like to know a little more about this process, please check out this blog post, explaining how we came to the decisions we have made here. We'd like to reassure you though that the CMS team, along with some special guests, took the time to assess each PR that is being closed and decided how to proceed - whilst the message you're receiving is automatic, the work behind it was done by the people that make up the CMS team, along with community members.

If there are specifics around this PR that you'd like explained that are not covered by the article, please let us know.

All the best to you, our wonderful contributor.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

@umbrabot umbrabot closed this Nov 8, 2021
@bjarnef
Copy link
Contributor Author

bjarnef commented Nov 8, 2021

Not sure which file is missing from the commit 😄
The MediaServiceExtensions.cs was never included in the project, but has been fixed in the PR instead: #10883

It wouldn't cause breaking change if the resource was used, but probably if developer had requests directly to these controller methods or used angular interceptors is expected a GET request.

It should be simple enough the update this to still use GET, while making GetByIds support both int[], Guid[], and Udi[] ... not just int[], although limited to maximum URL length (where POST would be ideal to request many entities).

@nul800sebastiaan do we still want this (without breaking changes in v8) or only v9?

@nul800sebastiaan
Copy link
Member

v9 is where we'll focus from now on. We're trying to wrap up v8.18 and had to make choices on what would still make it in there. I didn't look at this one specifically, but yes, this would definitely be great for v9!

@bjarnef bjarnef deleted the v8/feature/getbyids-int-guid-udi branch November 9, 2021 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ensure GetByIds in resources support Guid and Udi
3 participants