Fix #272: cherry pick api and helpers implemented #273
Conversation
antoineatstariongroup
requested review from
samatstariongroup and
lxatstariongroup
| /// <summary> | ||
| /// Gets or sets a collection of <see cref="Category"/>s shortname to used during the cherry picking request | ||
| /// </summary> | ||
| IEnumerable<string> CategoriesShortName { get; set; } |
There was a problem hiding this comment.
i doubt using shortnames wil work, they cannot be guaranteed to be unique accross the chain of RDLs. Please refactor to use Guids, feel free to use shortguids
| return invalidRequest.WithStatusCode(HttpStatusCode.BadRequest); | ||
| } | ||
|
|
||
| var cherryPickedThings = CherryPickHelper.CherryPick(resourceResponse, this.RequestUtils.QueryParameters.ClassKinds, this.RequestUtils.QueryParameters.CategoriesShortName) |
There was a problem hiding this comment.
this needs to be refactored to use the injected interface and will help when doing unit testing, it can then be mocked
| /// <summary> | ||
| /// Helper for the cherry picking features | ||
| /// </summary> | ||
| public static class CherryPickHelper |
There was a problem hiding this comment.
please refactor to an injectable service.
| /// <summary> | ||
| /// Helper class that provide capabilities on retrieving contained <see cref="Thing"/> or container <see cref="Thing"/> | ||
| /// </summary> | ||
| public static class ContainmentHelper |
There was a problem hiding this comment.
please refactor to injectable service
| { | ||
| Logger.Info(this.ConstructLog($"{requestToken} started")); | ||
| HttpRequestHelper.ValidateSupportedQueryParameter(this.Request, this.RequestUtils, SupportedGetQueryParameters); | ||
|
|
There was a problem hiding this comment.
i think that we should verify that the caller has permissions to request the classkind before we process the request. If the classkind does not have at leas READ, we should exit here and return a 404 not allowed. I want to make sure we do not create a backdoor here.
|
please add unit tests to keep coverage to at least the current value or better. Core product, cannot accept code without coverage |
|
@samatrhea Included your change requests and created unit tests for Services. |
Prerequisites
Description
Fix #272