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

feat(app, api-client, react-api-client): send CSV RTP file to robot before creating protocol and run #15527

Merged
merged 5 commits into from
Jun 27, 2024

Conversation

ncdiehl11
Copy link
Collaborator

@ncdiehl11 ncdiehl11 commented Jun 26, 2024

Overview

The new RTP paradigm for utilizing CSV files as runtime parameters requires sending the ID of the file to be used as the value in the runTimeParameterValues object sent with createProtocol and createRun. Here, we send any selected RTP files to the new /dataFiles endpoint and await the response containing an ID for each file. We then associate the returned ID with its respective runtime parameter variable name, and send that key:value pair along with the other value runtime parameters.

Test Plan

This is a little tricky to test right now because the /dataFiles endpoint is not yet wired up, and the /protocols and /runs endpoint only accepts <variableName>: <value> pairs for runTimeParameterValues, not the new format of <variableName>: {file_id: <id>} for CSV parameters.

Protocol card and detail

  • upload protocol that requires CSV file parameter like this:
    rtp_tests.py.zip

  • verify that CSV file required banner renders on protocol card

  • select protocol card

  • verify that CSV file required banner renders on protocol details

Changelog

  • post CSV files from RTPs when confirming values on both ChooseProtocolSlideout and ChooseRobotToRunProtocolSlideout, and use returned IDs in runTimeParametersValues sent with createRunFromProtocolSource
  • fix MiniCard warning text margins on above slideouts
  • update getAnalysisStatus util to return new parameterRequired status when analysis result is parameter-value-required
  • refactor useCreateProtocolMutation to accept runTimeParameterValues argument in its returned mutation's callback rather than in the hook's invocation itself
  • update tests

Review requests

Go through flow of uploading a CSV parameter protocol according to test plan above. Expected behavior is for a runCreationError to be raised when confirming value because the robot server does not yet know what to do with CSV parameter runTimeParameterValues overrides.

Risk assessment

medium

…efore creating protocol and run

The new RTP paradigm for utilizing CSV files as runtime parameters requires sending the ID of the
file to be used as the value in the `runTimeParameterValues` object sent with `createProtocol` and
`createRun`. Here, we send any selected RTP files to the new `/dataFiles` endpoint and await the
response containing an ID for each file. We then associate the returned ID with its respective
runtimeparameter variable name, and send that key:value pair along with the other value
runtimeparameters.
@ncdiehl11 ncdiehl11 marked this pull request as ready for review June 26, 2024 21:06
@ncdiehl11 ncdiehl11 requested a review from a team as a code owner June 26, 2024 21:06
@ncdiehl11 ncdiehl11 requested review from b-cooper, koji and jerader and removed request for a team and b-cooper June 26, 2024 21:06
@ncdiehl11 ncdiehl11 requested a review from jerader June 27, 2024 14:01
@ncdiehl11 ncdiehl11 requested a review from koji June 27, 2024 15:04
@koji
Copy link
Contributor

koji commented Jun 27, 2024

@ncdiehl11

The design doesn't have info icon next to CSV file. Maybe we don't need to display that for CSV file?

Screenshot 2024-06-27 at 12 37 45 PM

@ncdiehl11
Copy link
Collaborator Author

ncdiehl11 commented Jun 27, 2024

@ncdiehl11

The design doesn't have info icon next to CSV file. Maybe we don't need to display that for CSV file?

Screenshot 2024-06-27 at 12 37 45 PM

@koji Do you mean for the description?

I mean literary the icon.
Your branch displays icon next to CSV Data. But in the design there is no icon (the above screenshot).
I'm not 100% sure that removing the icon is intentional or not.

Screenshot 2024-06-27 at 12 43 14 PM

Another is Name of csv file.
The design says CSV File which I'm also not 100% sure this is the fixed text or displayName since csv file row is the unique can be the fixed text or displayName.
Probably we would need to ask about this to Rob/Mel.

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.

left a comment about Parameter Table's csv file part, but the changes look good to me.

@ncdiehl11 ncdiehl11 merged commit c874331 into edge Jun 27, 2024
22 checks passed
@ncdiehl11 ncdiehl11 deleted the feat_app-send-csv-with-protocool branch June 27, 2024 18:01
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.

3 participants