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, api-client, react-api-client): unify analysis and run record for CommandText use #15125

Merged
merged 3 commits into from
May 8, 2024

Conversation

ncdiehl11
Copy link
Collaborator

@ncdiehl11 ncdiehl11 commented May 7, 2024

closes AUTH-380

Overview

After a run is terminal, we access the run record rather than the most recently completed analysis in the command list in the run preview (or more aptly, run postview). This PR refactors the data sent to CommandText and ultimately many of its children and utilities to include only the necessary data from the run record if the run is terminal, or the most recent analysis if the run has not started or is in progress.

Test Plan

  • Run a protocol that loads at least one liquid (example linked as rtp_tests.py here)
  • Verify that run log before and during run shows full detail for load liquid command
  • Cancel or finish run and verify that run log still shows full detail for load liquid command

Changelog

add new interface CommandTextData specifying the following subset of the union of CompletedProtocolAnalysis and LegacyGoodRunData

  • pipettes
  • labware
  • modules
  • liquids
  • commands

implement this new interface in CommandText, its child components covering all possible command types, and its utilities that lookup entity data from their respective load commands

Review requests

auth js

Risk assessment

low-medium

…ecord for CommandText use

After a run is terminal, we access the run record rather than the most recently completed analysis
in the command list in the run preview (or more aptly, run postview). This PR refactors the data
sent to CommandText and ultimately many of its children and utilities to include only the necessary
data from the run record if the run is terminal, or the most recent analysis if the run has not
started or is in progress.
@ncdiehl11 ncdiehl11 self-assigned this May 7, 2024
@ncdiehl11 ncdiehl11 requested review from koji, shlokamin and jerader May 7, 2024 21:09
@ncdiehl11 ncdiehl11 marked this pull request as ready for review May 7, 2024 21:09
@ncdiehl11 ncdiehl11 requested a review from a team as a code owner May 7, 2024 21:09
@ncdiehl11 ncdiehl11 requested review from koji and jerader May 8, 2024 15:32
Copy link
Contributor

@koji koji left a comment

Choose a reason for hiding this comment

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

the changes look good to me.
thank you for refactoring!

@ncdiehl11 ncdiehl11 merged commit 73211b9 into edge May 8, 2024
21 checks passed
@ncdiehl11 ncdiehl11 deleted the refactor_app-command-text-data branch May 8, 2024 17:01
Carlos-fernandez pushed a commit that referenced this pull request May 20, 2024
Carlos-fernandez pushed a commit that referenced this pull request Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants