feat: add informative REST results for TAKE failures (SOFIE-295) #1621
feat: add informative REST results for TAKE failures (SOFIE-295) #1621anteeek wants to merge 1 commit intoSofie-Automation:mainfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
✨ Finishing touches🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
f50e077 to
0b0ef4c
Compare
|
Note: this PR was first created with wrong target in bbc#64 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Julusian
left a comment
There was a problem hiding this comment.
I feel like the openapi definitions should be updated to reflect this addition?
I don't see how the time is being reported for non-200 status codes. Unless the nextAllowedTakeTime inside the UserError is being reported somehow?
0b0ef4c to
4a86d64
Compare
|
@Julusian addressed the comment, thanks! Please take a look when you can |
| */ | ||
| export function responseError(userError: UserError): ClientResponseError { | ||
| return { error: UserError.serialize(userError), errorCode: userError.errorCode } | ||
| const nextAllowedTakeTime = userError.userMessage.args?.nextAllowedTakeTime as number | undefined |
There was a problem hiding this comment.
I'm not keen on this.
If its going to be a concept that we have defined in the client api, then it should be a proper concept in the UserError too.
But I'm not sure if it should be a dedicated value in ClientResponseError either.
I don't have any suggestions on how to do this better though, it just feels smelly to have this value specific to one UserAction be present in what is otherwise very general code
There was a problem hiding this comment.
I suppose I wouldnt mind if on UserError has an explicit property for nextAllowedOperationTime, and that was on ClientResponseError too. (or something named more around being a rate limit)
Then at least it is more generic and could be used by other things too (even though it wont for now, but maybe there should be more cases where we 'rate limit')
In the api it can continue to call it nextAllowedTakeTime, I am just worried about the specific naming of something for a single UserAction on internal types (if every action did that, these types would become a confusing mess
About the Contributor
This PR is made on behalf of the BBC
Type of Contribution
This is a:
Feature
Current Behavior
The TAKE endpoint returns either 200 or 500, without any additional information
New Behavior
HTTP Rest codes 400, 425 and 429 are added as possible failure reasons, and a UTC timestamp for next possible take is added
Testing
Affected areas
the TAKE endpoint
Time Frame
Not urgent, but we would like to get this merged into the in-development release.
Other Information
Status