Skip to content
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

refactor(app): load modules to retrieve moduleId for run setup module controls #10451

Merged
merged 11 commits into from
May 27, 2022

Conversation

sakibh
Copy link
Contributor

@sakibh sakibh commented May 25, 2022

Overview

This PR loads the modules in the protocol when the module control tab in the run setup page is visited. It retrieves the moduleId from the protocol analysis and feeds it to the corresponding createCommand for each module action pertaining to the run. When there is a runId present (during an active run) the module commands should POST commands to the /runs/:id/commands end point.

paired with @shlokamin @jerader

Changelog

  • Added useModuleIdFromRun to retrieve the correct moduleId from protocol analysis.
  • Updated ProtocolRunModuleControls to issue the necessary loadModule commands.
  • Updated various module related components and tests to accommodate module actions pertaining to a run.

Review requests

  • Upload a protocol with modules. Then visit the Module Controls tab in the setup page. Using the module dropdown menus and slideouts, issue commands (e.g. set temperature module to 45 degrees, then deactivate it). Check that when sending these request they are sending POST requests to the /runs/:id/commands end point.

  • Remove the protocol above and un-current the run. With a fresh slate, visit the device details page where all the module cards are listed. Perform the same commands/actions performed previously in the run detail page. This time check that the commands are sending POST requests to the /commands end point.

Note: Due to a polling issue currently, anytime you issue a command you will have to refresh the page. You can also open a menu and close it which will re-render the component quickly.

Risk assessment

low, only adds ability to send requests to alternative end point when runId is present

sakibh added 3 commits May 24, 2022 13:56
… controls

This PR loads the modules in the protocol and then retrieves the moduleId from the protocol analysis
and feeds it to the corresponding createCommand for each module action.
@sakibh sakibh requested review from b-cooper, shlokamin and jerader May 25, 2022 17:39
@sakibh sakibh self-assigned this May 25, 2022
@sakibh sakibh requested a review from a team as a code owner May 25, 2022 17:39
@codecov
Copy link

codecov bot commented May 25, 2022

Codecov Report

Merging #10451 (eb88285) into edge (1e97a69) will decrease coverage by 0.16%.
The diff coverage is 60.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #10451      +/-   ##
==========================================
- Coverage   73.81%   73.65%   -0.17%     
==========================================
  Files        2138     2136       -2     
  Lines       57468    57464       -4     
  Branches     5780     5807      +27     
==========================================
- Hits        42422    42325      -97     
- Misses      13836    13919      +83     
- Partials     1210     1220      +10     
Flag Coverage Δ
app 71.29% <60.52%> (-0.03%) ⬇️
components 52.75% <ø> (ø)
labware-library 49.74% <ø> (ø)
protocol-designer 45.58% <ø> (ø)
react-api-client 82.68% <ø> (ø)
step-generation 86.48% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...rganisms/Devices/ModuleCard/ModuleOverflowMenu.tsx 100.00% <ø> (ø)
app/src/organisms/Devices/ModuleCard/index.tsx 71.71% <0.00%> (-1.48%) ⬇️
...src/organisms/Devices/ProtocolRun/SetupModules.tsx 86.20% <ø> (ø)
...organisms/Devices/HeaterShakerWizard/TestShake.tsx 68.42% <38.46%> (-13.73%) ⬇️
app/src/organisms/Devices/ModuleCard/hooks.tsx 83.56% <40.00%> (-3.21%) ⬇️
.../Devices/ProtocolRun/ProtocolRunModuleControls.tsx 60.00% <50.00%> (-15.00%) ⬇️
...organisms/Devices/ModuleCard/TestShakeSlideout.tsx 76.19% <57.14%> (+5.13%) ⬆️
...anisms/Devices/ModuleCard/HeaterShakerSlideout.tsx 78.72% <75.00%> (-0.35%) ⬇️
...organisms/Devices/ModuleCard/useModuleIdFromRun.ts 81.81% <81.81%> (ø)
...src/organisms/Devices/HeaterShakerWizard/index.tsx 82.22% <100.00%> (ø)
... and 28 more

@sakibh sakibh requested review from smb2268 and removed request for a team May 25, 2022 18:46
Copy link
Collaborator

@jerader jerader left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic in here looks great and works as expected. Thanks so much for tackling this monstrous task.

@sakibh sakibh requested a review from a team as a code owner May 25, 2022 20:41
@sakibh sakibh removed the request for review from a team May 25, 2022 20:49
@sakibh sakibh requested a review from y3rsh May 25, 2022 21:01
const filteredModules = attachedModules.filter(
item =>
item.moduleModel === module.moduleModel ||
compatibleWith.includes(item.moduleModel)
Copy link
Member

@shlokamin shlokamin May 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how come we have to check compatibleWith? i.e. if there is a temp module v1 and a temp module v2, they should be treated as two different modules right?

Copy link
Contributor

@b-cooper b-cooper May 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to check compatible with, because we need to have an array of all of the modules that the robot server would use as candidates to match with the specced module. That way we can pair the load index with the usb index. This is a (currently necessary) duplication of logic on the backend.

Comment on lines +31 to +36
const moduleIndex = filteredModules.findIndex(
attachedModule => attachedModule.serialNumber === module.serialNumber
)

const moduleIdFromRun = loadModuleCommands?.[moduleIndex].result.moduleId

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wonder if we should have some null check safety here, in the event someone unplugs a module or something. if for some reason loadModuleCommands?.[moduleIndex] is undefined, this would white screen

Comment on lines 37 to 39
const loadCommands: LoadModuleRunTimeCommand[] = protocolData?.commands.filter(
command => command.commandType === 'loadModule'
) as LoadModuleRunTimeCommand[]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of using an as cast, you can use a type predicate:

const loadCommands: LoadModuleRunTimeCommand[] = protocolData?.commands.filter(
  (command: RunTimeCommand): command is LoadModuleRunTimeCommand =>
    command.commandType === 'loadModule'
)

Copy link
Contributor

@b-cooper b-cooper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on Robot and works as expected. The fix looks good! :atom:

@sakibh sakibh merged commit 3dffaef into edge May 27, 2022
@sakibh sakibh deleted the app_fix-module-controls branch May 27, 2022 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants