refactor(/admin/groups/): Move parameter check from responder to endpoint #3093
Conversation
mpro7
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3093 +/- ##
===========================================
+ Coverage 11.74% 88.60% +76.86%
===========================================
Files 246 261 +15
Lines 22907 22394 -513
===========================================
+ Hits 2690 19842 +17152
+ Misses 20217 2552 -17665 ☔ View full report in Codecov by Sentry. |
| Left("At least one field must be present.") | ||
| else Right(request), | ||
| identity, | ||
| ) |
There was a problem hiding this comment.
issue: A serializer/deserializer contains validation logic for the value type
IMHO this behavior of a codec is surprising in a bad way. The product type GroupUpdateRequest allows for all combinations of params and that includes all being None. A serializer/deserializer should then also be able to produce a GroupUpdateRequest json reflecting the all None state.
I would rather like to see this done programatically in the rest service or in a smart constructor. A codec to me feels like the last place I would assume such logic exists.
| @@ -186,20 +186,6 @@ class GroupsResponderADMSpec extends CoreSpec { | |||
| s"Group with the name '${groupName.value}' already exists.", | |||
| ) | |||
| } | |||
|
|
|||
| "return 'BadRequest' if nothing would be changed during the update" in { | |||
There was a problem hiding this comment.
suggestion: Maybe move this test to the GroupsADME2ESpec?
|
@seakayone I will address your comments in another (follow-up) PR. |
Pull Request Checklist
Task Description/Number
Small PR which relates to DEV-3292 and two questions.
PR Type
Basic requirements for bug fixes and features
Does this PR introduce a breaking change?
Does this PR change client-test-data?