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.

Sign up for GitHub

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

Jump to bottom

fix(app): clone run with RTPs from HistoricalProtocolRun #14959

Merged
merged 8 commits into from
Apr 23, 2024
Merged

fix(app): clone run with RTPs from HistoricalProtocolRun #14959

merged 8 commits into from
Apr 23, 2024

Conversation

ncdiehl11
Copy link
Collaborator

@ncdiehl11 ncdiehl11 commented Apr 19, 2024 •
edited
Loading

closes RQA-2601

Overview

Here, I extend the useCloneRun hook to optionally create a new analysis for a protocol with RTP
override values that will be referenced at protocol setup. This provides functionality for cloning a
HistoricalProtocolRun with its RTP values preserved.

Test Plan

  1. Run an RTP protocol with any parameter set
  2. Rerun that same protocol with different parameters on the same robot
  3. Navigate to device details run history
  4. Click the overflow menu > "Rerun protocol now"
  5. Verify that the Parameters tab shows the RTP for the selected run and not the most recent run
  6. Repeat 4 and 5, but clicking the overflow menu > "View protocol run record"
  7. Repeat 4 and 5, but clicking the date of the run record rather than "Rerun now"

Changelog

  • update useCloneRun to optionally trigger a new protocol analysis, properly invalidate queries to /runs and /protocols
  • use run record rather than most recent analysis able on Parameters and RunPreview
  • add and export constant for terminal run statuses
  • fix runTimeParameters key on Run type
  • add ProtocolRun > Parameters InfoScreen for run canceled before beginning

Review requests

auth, @shlokamin per pairing

Risk assessment

medium

@ncdiehl
fix(app): clone run with RTPs from HistoricalProtocolRun
7e1ccb8 
Here, I extend the useCloneRun hook to optionally create a new analysis for a protocol with RTP
override values that will be referenced at protocol setup. This provides functionality for cloning a
HistoricalProtocolRun with its RTP values preserved.
@ncdiehl11 ncdiehl11 marked this pull request as ready for review April 19, 2024 16:27
@ncdiehl11 ncdiehl11 requested a review from a team as a code owner April 19, 2024 16:27
mjhuff
mjhuff reviewed Apr 19, 2024
View reviewed changes
Copy link
Contributor

@mjhuff mjhuff left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I like the new terminal constant!

I'd probably spend some time grepping for the terminal run statuses in the app individually, because there are a few places we use them with slightly different names. Ex, in ProtocolRunHeader we have RUN_OVER_STATUSES. These should be refactored over.

shlokamin
shlokamin reviewed Apr 22, 2024
View reviewed changes
Copy link
Member

@shlokamin shlokamin left a comment

Choose a reason for hiding this comment

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

looks good to me, just a few small things

app/src/organisms/Devices/ProtocolRun/ProtocolRunRunTimeParameters.tsx
Comment on lines +47 to +51
const run = useNotifyRunQuery(runId).data
const runTimeParameters =
(isRunTerminal
? run?.data?.runTimeParameters
: mostRecentAnalysis?.runTimeParameters) ?? []
Copy link
Member

Choose a reason for hiding this comment

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

i'd add a little comment above here explaining why we're doing this — i can see myself forgetting a year from now 😛

app/src/organisms/Devices/ProtocolRun/ProtocolRunRunTimeParameters.tsx Outdated
<InfoScreen contentType="parameters" />
<InfoScreen
contentType={
runStatus === RUN_STATUS_STOPPED ? 'runNotStarted' : 'parameters'
Copy link
Member

Choose a reason for hiding this comment

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

this isn't quite right — a run could be stopped half way through a run. i think what we want to check here is whether a play action has been dispatched. so we'd do something like this:

  const runActions = runRecord?.data.actions
  const hasRunStarted = runActions?.some(
    action => action.actionType === RUN_ACTION_TYPE_PLAY
  )

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That makes more sense— thanks for the catch

app/src/organisms/RunPreview/index.tsx Outdated
Comment on lines 51 to 54
const commandsFromQuery = useAllCommandsQuery(runId, null, {
staleTime: Infinity,
cacheTime: Infinity,
}).data?.data
Copy link
Member

Choose a reason for hiding this comment

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

id add a comment here explaining that we're doing this because we only ever want one request done because its so heavy. i'd also disable this query whenever the run is NOT terminal so we don't unnecessarily take the performance hit

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

makes sense to me as well

jerader
jerader reviewed Apr 23, 2024
View reviewed changes
app/src/organisms/RunTimeControl/hooks.ts
RUN_STATUS_SUCCEEDED,
RUN_STATUS_STOPPED,
] as RunStatus[]).includes(lastRunStatus.current),
!(RUN_STATUSES_TERMINAL as RunStatus[]).includes(lastRunStatus.current),
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be able to remove this typing when RUN_STATUSES_TERMINAL is typed

jerader
jerader reviewed Apr 23, 2024
View reviewed changes
components/src/molecules/ParametersTable/InfoScreen.tsx Outdated
Comment on lines 17 to 24
case 'parameters':
bodyText = 'No parameters specified in this protocol'
break
case 'moduleControls':
bodyText = 'Connect modules to see controls'
break
case 'runNotStarted':
bodyText = 'Run was never started'
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we add these to i18n? you'll have to pass it down as a prop since components doesn't have i18n

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can, but am I wrong in thinking we removed that translation object prop somewhere along the way? Do we remember the reason for this?

Copy link
Member

Choose a reason for hiding this comment

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

yeah lets use i18n here — you can use the useTranslation hook directly from this component

jerader
jerader approved these changes Apr 23, 2024
View reviewed changes
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.

lgtm, i don't wanna block this from merging so all the comments i left can be addressed in a follow up

shlokamin
shlokamin approved these changes Apr 23, 2024
View reviewed changes
app/src/organisms/Devices/ProtocolRun/ProtocolRunRunTimeParameters.tsx Outdated
<InfoScreen contentType="parameters" />
<InfoScreen
contentType={
!hasRunStarted && runStatus === RUN_STATUS_STOPPED
Copy link
Member

Choose a reason for hiding this comment

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

i dont think we care whether the run has been stopped right? we just care if its has been started

Copy link
Collaborator Author

@ncdiehl11 ncdiehl11 Apr 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think we do need to check whether the run has been stopped because otherwise we will show "run never started" immediately when starting protocol setup for a non-RTP protocol, and we actually want to show "no parameters" in that case

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

example:
https://github.com/Opentrons/opentrons/assets/47604184/20d97382-2e6c-4bfa-a8dd-bdd9bd0dc20e

components/src/molecules/ParametersTable/InfoScreen.tsx Outdated
Comment on lines 17 to 24
case 'parameters':
bodyText = 'No parameters specified in this protocol'
break
case 'moduleControls':
bodyText = 'Connect modules to see controls'
break
case 'runNotStarted':
bodyText = 'Run was never started'
Copy link
Member

Choose a reason for hiding this comment

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

yeah lets use i18n here — you can use the useTranslation hook directly from this component

@ncdiehl11 ncdiehl11 merged commit 26929a2 into edge Apr 23, 2024
49 checks passed
y3rsh added a commit that referenced this pull request Apr 23, 2024
@y3rsh
Merge branch 'edge' into AUTH-73-golden-RTP-examples
ddb5020 
* edge: (194 commits)
  fix(app): clone run with RTPs from HistoricalProtocolRun (#14959)
  fix(api): Filter out `air_gap()` calls as higher-order commands (#14985)
  fix(app): fix infinitely re-rendering/never rendering firmware success toasts (#14981)
  feat(api): add option to ignore different tip presence states (#14980)
  feat(opentrons-ai-client) add input textbox to container (#14968)
  fix(app): add robotSerialNumber to proceedToRun event (#14976)
  fix(api): remove homing patch fix for right mount when a 96-channel is attached (#14975)
  feat(api-client,app,react-api-client): upload splash logo from desktop app (#14941)
  fix(robot-server): notify /runs when a non-current run is deleted (#14974)
  feature(api, robot-server): Allow fixit commands to recover from an error (#14908)
  feat(hardware-testing): enable multi sensor processing in liquid probe (#14883)
  fix(app): prevent "run again" banner from rendering after navigating away from the current run (#14973)
  refactor(components): refactor roundtab stories (#14956)
  refactor(protocol-designer): assign module slot in createFileWizard instead of modal (#14951)
  fix(app, api-client): fix choose protocol slideout issue (#14949)
  refactor(protocol-designer): tip position modal max values round down (#14972)
  feat(app): add tiprack selection step to quick transfer flow (#14950)
  ci(shared-data): install dependencies in workflow (#14958)
  fix(components): fix icon stories (#14969)
  feat(opentrons-ai-client): introduce react-markdown to chat display component (#14965)
  ...
Carlos-fernandez pushed a commit that referenced this pull request May 20, 2024
@ncdiehl11 @Carlos-fernandez
fix(app): clone run with RTPs from HistoricalProtocolRun (#14959)
6849d64 
closes RQA-2601
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjhuff mjhuff mjhuff left review comments

@shlokamin shlokamin shlokamin approved these changes

@jerader jerader jerader approved these changes

Assignees
No one assigned
Labels
authorship
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@ncdiehl11 @shlokamin @mjhuff @jerader @koji @ncdiehl