-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Add test for sibling endpoint returning parent id even when its a folder #20118
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
base: main
Are you sure you want to change the base?
Conversation
…on if not specified on the controller
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Migaroez - looks good and can see if verifies the added functionality.
Only concern is that it feels a little odd this being literally the only test for a management API controller. But I guess so long as it's adding value that's OK, and others could be more easily added in future following the pattern.
tests/Umbraco.Tests.Integration/ManagementApi/Trees/DocumentTypeSiblingControllerTests.cs
Outdated
Show resolved
Hide resolved
tests/Umbraco.Tests.Integration/ManagementApi/Trees/DocumentTypeSiblingControllerTests.cs
Outdated
Show resolved
Hide resolved
using Umbraco.Cms.Core.Services.OperationStatus; | ||
using Umbraco.Cms.Tests.Common.Builders; | ||
|
||
namespace Umbraco.Cms.Tests.Integration.ManagementApi.Trees; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For alignment with the main project, perhaps namespace and folder should be: Umbraco.Cms.Tests.Integration.ManagementApi.Controllers.DocumentType.Trees
…peSiblingControllerTests.cs Co-authored-by: Andy Butland <[email protected]>
…peSiblingControllerTests.cs Co-authored-by: Andy Butland <[email protected]>
Yeah I agree, but it's because the logic there shouldn't be in a controller. When that logic is moved, this test can be moved too and the entrypoint changed while the setup and assert can be more or less the same. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then approving. As you say, maybe it can go away in the future, or it can serve as a basis for other tests on logic in the controllers where it's not straightforward to place or move.
Description
Added a test for the logic introduced in #20083
Updated the GetManagementApiUrl logic to allow for not having to specify the version on the controller/method.