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

Adding additional document collection properties to align model with item response #18309

Merged

Conversation

AndyButland
Copy link
Contributor

@AndyButland AndyButland commented Feb 12, 2025

This PR adds the IsTrashed and IsProtected fields to the document collection result, aligning with the document item result.

IsTrashed was straightforward, we just needed to update the mapping.
IsProtected is a little more tricky, as we have to make a call via the IPublicAccessService to get the result.

I've actually implemented this twice when you look through the commits. Neither I'm totally happy with but I can't find a better way.

First I did the work in the controller (see this commit), but this didn't feel right. Meant having to unpack the IActionResult to get at the object being returned to manipulate it.

So I ended with pushing this up the mapping layer, which is the right place, but requires injecting the IPublicAccessService into DocumentMapDefinition. Maybe that's OK, but I don't see we inject services into the mapping definitions in other cases.

So needs a second opinion.

To test just pull up the management API via Swagger and test the /umbraco/management/api/v1/collection/document/ endpoint to verify it returns these two new properties.

@AndyButland AndyButland marked this pull request as draft February 12, 2025 13:56
@AndyButland AndyButland marked this pull request as ready for review February 13, 2025 13:39
@AndyButland AndyButland changed the title WIP: Adding additional document collection properties to align model with item response. Adding additional document collection properties to align model with item response Feb 13, 2025
@nikolajlauridsen
Copy link
Contributor

nikolajlauridsen commented Feb 14, 2025

Good instinct 😄

With the new management API, we try to keep the mappers for only mapping properties from one object to another, so there is no additional population from various services 😄 This is because mapping should be a "cheap" operation; however, this was not always the case in the old backoffice.

What we do instead now is create a factory when we need to populate values from services, you can see examples of this in the Umbraco.Cms.Api.Management.Factories; namespace. This is a bit complicated in this case because it's all tangled up in inheritance. In my opinion, the best way to resolve this would be to create a DocumentCollectionPresentationFactory and move the logic from the inherited CollectionResult method (minus the creation of PagedViewModel and IActionResult) into there, along with the population of IsProtected, and break that aspect of the inheritance. I'm not too fuzzed about the last part however, so I'll let that be up to you, but I think we should avoid using the services in the mapper 😄

@AndyButland
Copy link
Contributor Author

AndyButland commented Feb 14, 2025

Thanks @nikolajlauridsen - agreed. I could tell it was a code smell but I couldn't see the best way forward. I'll have a look at what you suggest.

@AndyButland
Copy link
Contributor Author

OK @nikolajlauridsen, I've refactored now as per your suggestion.

Copy link
Contributor

@nikolajlauridsen nikolajlauridsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great and tests good 👍

@nikolajlauridsen nikolajlauridsen merged commit 69e251a into v15/dev Feb 17, 2025
21 checks passed
@nikolajlauridsen nikolajlauridsen deleted the v15/task/align-document-item-and-collection-models branch February 17, 2025 11:27
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.

2 participants