i15-1: display basic feedback from blueapi#44
Conversation
DominicOram
left a comment
DominicOram
left a comment
There was a problem hiding this comment.
Thanks, some comments in code, additionally:
- I can quite easily trigger multiple plans at once by just hitting the button twice. This is probably because of the fact we have to do two calls to BlueAPI to run the plan and we only disable the button on the second call. This should be fixed when BlueAPI moves away from the two calls in DiamondLightSource/blueapi#1326 but can we have an issue that acknowledges this?
- We should move Abort and WorkerState into
blueapi-queryas they seem common. Also update the docs in there so that people know it's available
| return new Promise((res) => setTimeout(res, ms)); | ||
| }; | ||
|
|
||
| const runTask = async (task_id: string) => { |
There was a problem hiding this comment.
Should: Can we have some tests on the logic in this?
| await startTask.mutateAsync(task_id).then(async (response) => { | ||
| if (response) { | ||
| let status = workerState.data; | ||
| while (status !== "IDLE" && status !== "ABORTING") { |
There was a problem hiding this comment.
Could: Can we pull these into constants somewhere?
| }; | ||
| setLoading(true); | ||
| await submitAndRunTask(taskRequest); | ||
| await submitAndRunTask(taskRequest).catch((error) => { |
There was a problem hiding this comment.
Should: Can we have a test on this
DominicOram
left a comment
DominicOram
left a comment
There was a problem hiding this comment.
Thanks, some more comments in code, I think there may have been a bad merge? Also, I still stand by wanting unit tests on some of the logic...
| const theme = useTheme(); | ||
| return ( | ||
| <Box sx={{ ml: 5 }}> | ||
| <Stack direction={"column"} spacing={2}> |
There was a problem hiding this comment.
Must: I think this is a duplicate of the existing card. Maybe a bad merge?
| import { ReadOnlyPv } from "@atlas/pvws-config"; | ||
| import { StatusCard } from "../components/StatusCard"; | ||
| import { BlueapiWorkerState } from "../components/BlueapiWorkerState"; | ||
| // import { AbortPlanButton } from "../components/AbortPlanButton"; |
There was a problem hiding this comment.
Must: This is no longer used? I think we want it here, right?
…response as it's not a rejection
|
Added a few tests but after looking at some documentation, it seems that in order to be able to test the logic I would need to get those functions out of the component (can't test functions nested inside). Tbh, I would rather do that in a separate PR and get this functionality merged... |
3 participants
See https://jira.diamond.ac.uk/browse/I15_1-1539