feat(robot server): add a POST method on the analyses endpoint #14828
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SyntaxColoring
approved these changes
Apr 8, 2024
Comment on lines
+580
to
+592
| """Start a new analysis for the given existing protocol. | ||
|
|
||
| Starts a new analysis for the protocol along with the provided run-time parameter | ||
| values (if any), and appends it to the existing analyses. | ||
|
|
||
| If the last analysis in the existing analyses used the same RTP values, then a new | ||
| analysis is not created. | ||
|
|
||
| If `forceAnalyze` is True, this will always start a new analysis. | ||
|
|
||
| Returns: List of analysis summaries available for the protocol, ordered as | ||
| most recently started analysis last. | ||
| """ |
There was a problem hiding this comment.
I'm not sure if this is intentional, but FYI, none of this docstring will show up in the HTTP API docs—it gets overridden by the description in the decorator.
| default={}, | ||
| description="Key-value pairs of run-time parameters defined in a protocol.", | ||
| ) | ||
| forceReAnalyze: bool = Field( |
There was a problem hiding this comment.
FYI this doesn't quite match the name you have in the method docstring. forceReAnalyze vs. forceAnalyze.
Carlos-fernandez
pushed a commit
that referenced
this pull request
May 20, 2024
Closes AUTH-255 # Overview Adds a POST method to the existing `/protocols/{protocolId}/analyses` endpoint in order to post a new analysis for an existing protocol. This endpoint will take a request body with two optional fields: - `runTimeParameterValues` - `forceReAnalyze` The new method can affect the analyses in three ways: 1. When the request is sent with `forceReAnalyze=True`, the server will unconditionally start a new analysis for the protocol using any RTP data sent along with it. It will return a 201 CREATED status and respond with a list of analysis summaries of all the analyses (ordered oldest first), including the newly started analysis. 2. When the request is sent without the `forceReAnalyze` field (or with `forceReAnalyze=False`), then the server will check the last analysis of the protocol - if the RTP values used for it were **different** from the RTP values sent with the current request, then the server will start a new analysis using the new RTP values. It will return a 201 CREATED status and respond with a list of analysis summaries of all the analyses, including the newly started analysis. - if the RTP values used for it were the **same** as the RTP values sent with the current request, then the server will **NOT** start a new analysis. It will return a 200 OK status, and simply return the existing list of analysis summaries. This request requires the last analysis of the protocol to have been completed before handling this request. If the last analysis is pending, it will return a 503 error. # Test Plan Test out the above three cases and anything else you can think might affect the behavior. It's pretty well tested in unit & integration tests and it is also the same logic used for handling analyses from `POST /protocols`, so it is expected to work well when used as tested in integration tests. # Review requests Usual review for code sanity check. # Risk assessment Low. New HTTP API not yet used anywhere.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes AUTH-255
Overview
Adds a POST method to the existing
/protocols/{protocolId}/analysesendpoint in order to post a new analysis for an existing protocol.This endpoint will take a request body with two optional fields:
runTimeParameterValuesforceReAnalyzeThe new method can affect the analyses in three ways:
forceReAnalyze=True, the server will unconditionally start a new analysis for the protocol using any RTP data sent along with it. It will return a 201 CREATED status and respond with a list of analysis summaries of all the analyses (ordered oldest first), including the newly started analysis.forceReAnalyzefield (or withforceReAnalyze=False), then the server will check the last analysis of the protocolThis request requires the last analysis of the protocol to have been completed before handling this request. If the last analysis is pending, it will return a 503 error.
Test Plan
Test out the above three cases and anything else you can think might affect the behavior.
It's pretty well tested in unit & integration tests and it is also the same logic used for handling analyses from
POST /protocols, so it is expected to work well when used as tested in integration tests.Review requests
Usual review for code sanity check.
Risk assessment
Low. New HTTP API not yet used anywhere.