-
Notifications
You must be signed in to change notification settings - Fork 2
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
Implementing default delivery channel matching #745
Implementing default delivery channel matching #745
Conversation
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.
It might be worth moving the stuff for matching default delivery channels to it's own class for ease of testing - even into IDefaultDeliveryChannelRepository
so that can be more than just a caching layer to DB - may be able to alter that so it doesn't return the whole list and does all matching internally. Once it's in there we can write tests against it in isolation to confirm logic. From there it will make it easy to refactor as long as the tests pass.
Using 3 methods and multiple dictionarys feels slightly overly complex and difficult to follow for how the default matching is done but I could well be overlooking something but if this works and it's encapsulated into a tested class then it's easy to refactor.
src/protagonist/DLCS.Repository/DeliveryChannels/DefaultDeliveryChannelRepository.cs
Outdated
Show resolved
Hide resolved
src/protagonist/DLCS.Repository/DeliveryChannels/DefaultDeliveryChannelRepository.cs
Outdated
Show resolved
Hide resolved
src/protagonist/DLCS.Repository/DeliveryChannels/DefaultDeliveryChannelRepository.cs
Outdated
Show resolved
Hide resolved
ee05064
to
13c8fca
Compare
src/protagonist/DLCS.Repository.Tests/DLCS.Repository.Tests.csproj
Outdated
Show resolved
Hide resolved
src/protagonist/API/Features/DeliveryChannels/DeliveryChannelPolicyRepository.cs
Outdated
Show resolved
Hide resolved
src/protagonist/API.Tests/Features/Images/Ingest/AssetProcessorTest.cs
Outdated
Show resolved
Hide resolved
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.
LGTM - couple of comments but small changes/formatting
src/protagonist/API.Tests/Features/DeliveryChannels/DefaultDeliveryChannelRepositoryTests.cs
Outdated
Show resolved
Hide resolved
src/protagonist/API.Tests/Features/DeliveryChannels/DefaultDeliveryChannelRepositoryTests.cs
Outdated
Show resolved
Hide resolved
src/protagonist/API/Features/DeliveryChannels/DefaultDeliveryChannelRepository.cs
Outdated
Show resolved
Hide resolved
src/protagonist/API/Features/DeliveryChannels/DefaultDeliveryChannelRepository.cs
Outdated
Show resolved
Hide resolved
src/protagonist/API/Features/DeliveryChannels/DeliveryChannelPolicyRepository.cs
Outdated
Show resolved
Hide resolved
src/protagonist/API.Tests/Features/DeliveryChannels/DefaultDeliveryChannelRepositoryTests.cs
Outdated
Show resolved
Hide resolved
e2d75a7
to
f23988c
Compare
Resolves #625
This pull request implements the ability to add
ImageDeliveryChannels
to images based on the media type