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

chore(merge): 8.0.0 alpha.1 mergeback #15981

Merged
merged 23 commits into from
Aug 13, 2024
Merged

chore(merge): 8.0.0 alpha.1 mergeback #15981

merged 23 commits into from
Aug 13, 2024

Conversation

mjhuff
Copy link
Contributor

@mjhuff mjhuff commented Aug 13, 2024

Overview

Keeping the spirit of #15940 alive.

All the merge conflicts were my own.

shlokamin and others added 22 commits August 8, 2024 15:48
…dy in progress (#15938)

# Overview

Ugh... this is a pretty bad oopsy. If the ODD is downloading a robot
system zip, it is possible (and in fact, common) that additional
downloads of the same system zip would get started into new temp dirs.

This has been an issue for the entirety of the Flex's existence, but we
think this issue is made worse in China because of their harsh
firewalls.

This PR properly short circuits system downloads early if one is already
in progress.

Props to @vegano1 for finding this!


## Changelog

- Do not initiate new system file downloads if one is already in
progress


## Risk assessment

Medium
Update text for SetupLiquids confirmation button on desktop to reflect
designs.

Closes RQA-2929
If both need to be shown, both need to be shown. The setup steps modal
comes first and then the heater shaker modal.

## review/dev-testing
- [x] on ODD, when a heater/shaker is in the protocol, the confirm steps
modal comes first and then the confirm HS. clicking confirm HS is the
one that starts the run
- [x] on ODD, when there's no heater/shaker, the confirm steps modal
still pops and it starts the run when you click confirm
- [x] on Desktop, when there's a heater shaker in the protocol and you
haven't disabled the popup, the confirm steps modal comes first and then
confirm HS. clicking confirm HS is the one that starts the run
- [x] on Desktop, when there's a heater shaker in the protocol and you
have disabled the popup, the confirm steps modal is the only one and
clicking confirm starts the run
- [x] on Desktop, when there's no heater shaker in the protocol, the
confirm steps modal is the only one and clicking confirm starts the run


Closes RQA-2932
…al `PLAY` action (#15943)

When a client posts a PLAY action that starts a protocol run, any existing maintenance run ID is cleared from the server. This means that MQTT should update subscribers interested in the current maintenance run.
Add stack icon, highlight, and click handling to non-module stacks
(example: slot + adapter + labware)
Special case thermocycler location on ODD ProtocolDetails Hardware
(A1+B1)

Closes RQA-2938
Fixes RQA-2925 for desktop: the liquids entry now will count as complete
for the purposes of not popping or being listed in the modal if you have
no liquids. It also gets complete decorations if there are no liquids.
That required adding that rightElement thing to the empty version of the
setup step, and that also is what we would need to do for modules, so
also add decorations for modules and fix the completion logic to count
the module actually being plugged in again (this was RQA-2928).

On the ODD, similar kind of logic but a lot easier to implement because
the data is more local; we can default the state to !hasLiquids, and
then use the same styling we already had implemented for runtime
parameters for the setup step.

Also, remove some stylistically out of date text transforms on the setup
cards.

## testing and review
- [x] Upload a protocol that has no liquids and no modules and no deck
fixtures
- [x] the liquids and modules tabs should should default to completed,
meaning
- [x] on desktop, they get complete badges and text (including on the
OT-2)
      - [x] on the ODD, they get rendered green
      - [x] on desktop and ODD, they don't inhibit starting the run
- [x] including that if you haven't confirmed some other step, the steps
nag modal doesn't mention liquids
- [x] on a protocol that requires modules, if you don't have the modules
plugged in you get an action needed badge


Closes RQA-2928
Closes RQA-2925
This forces you to take a pass through LPC or just commit to skipping it
all.

## reviewing and testing
- [x] you can't click the button on odd when there are no offsets
- [x] you can't click the button on desktop when there are no offsets
- [x] you can click the button on odd when there are offsets
- [x] you can click the button on desktop when there are offsets

Closes RQA-2930
Drop tip wizard CTAs now home the pipette if skipping the flow and close the current run context in both the "skip" and "remove tips" cases. Copy is updated throughout all drop tip flow entry points as well.

This is the first time the app directly POSTs a one-off command via a maintenance run. We do POST one-off commands elsewhere in the app, but we do so using older, less-supported endpoints. If these one-off commands become more common, we should consider creating a dedicated API for doing so (more hooks!). To solve the immediate problem now, I refactored useDropTipMaintenanceRun.

The real complexity of this PR is managing all the interactions with closing run context and ensuring other clients not engaging with the drop tip flows see/don't see the drop tip CTAs when appropriate. It's probably easiest to follow this PR commit-by-commit.

NOTE: If a user completes a run with tips attached on both pipettes intentionally (the protocol itself has tips attached for both pipettes at the end of the run), the wizard won't display for both pipettes, only the left pipette. The run context closes as soon as the home happens, so there's no second CTA. The skip command won't home the plungers, so that's safe, but current behavior is the user will have to do drop tips via the Instruments page. I imagine this is quite the edge case, but it's worth noting. I can re-address this when I update the tip detection logic shortly.

Here's a complete breakdown of all the changes:

Drop Tip Wizard: Error Recovery, Desktop and ODD

*DT Wiz flows remain unchanged for all ER options that are not "cancel run".
*For "cancel run", only the copy changes for what was once "Skip" to something along the lines of "skip and home pipette." The pipette will home during cancel.

Drop Tip Wizard: Post-Run, Desktop and ODD

*If the user has tips attached AND does not see dtwiz within error recovery when doing "cancel run", they will see an unskippable modal (no X button in the upper right) that says "skip and home pipettes" and "begin drop tip wizard".
*Once the drop tip flows are successfully completed, the run context closes.
*If a user clicks the upper-right corner "exit" button within, the confirmation screen now contains copy alerting the user that the pipette will home after you confirm the exit.
*Once the user confirms they want to exit, the pipette will home, and the run context will close.

Drop Tip Wizard: Instruments Page, Desktop and ODD

*The upper-right corner exit confirmation page copy and homing behavior is the same as the post-run behavior. We do not close any sort of run context here.
*Note that the drop-tip banner will no longer be present on the desktop, post-run.
<!--
Thanks for taking the time to open a Pull Request (PR)! Please make sure
you've read the "Opening Pull Requests" section of our Contributing
Guide:


https://github.com/Opentrons/opentrons/blob/edge/CONTRIBUTING.md#opening-pull-requests

GitHub provides robust markdown to format your PR. Links, diagrams,
pictures, and videos along with text formatting make it possible to
create a rich and informative PR. For more information on GitHub
markdown, see:


https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax

To ensure your code is reviewed quickly and thoroughly, please fill out
the sections below to the best of your ability!
-->

# Overview
The Z is supposed to retract out to the liquid before sending over the
buffered data, however a line from testing got merged in there and this
wasn't happening. Also had to fixup the api side since it was
calculating the liquid height based off where the machine position was
after hardware control took over, now it actually uses the return value
to that it returns, converts it to deck coordinates and applies the
corresponding offsets.

<!--
Describe your PR at a high level. State acceptance criteria and how this
PR fits into other work. Link issues, PRs, and other relevant resources.
-->

## Test Plan and Hands on Testing

<!--
Describe your testing of the PR. Emphasize testing not reflected in the
code. Attach protocols, logs, screenshots and any other assets that
support your testing.
-->

## Changelog

<!--
List changes introduced by this PR considering future developers and the
end user. Give careful thought and clear documentation to breaking
changes.
-->

## Review requests

<!--
- What do you need from reviewers to feel confident this PR is ready to
merge?
- Ask questions.
-->

## Risk assessment

<!--
- Indicate the level of attention this PR needs.
- Provide context to guide reviewers.
- Discuss trade-offs, coupling, and side effects.
- Look for the possibility, even if you think it's small, that your
change may affect some other part of the system.
- For instance, changing return tip behavior may also change the
behavior of labware calibration.
- How do your unit tests and on hands on testing mitigate this PR's
risks and the risk of future regressions?
- Especially in high risk PRs, explain how you know your testing is
enough.
-->
…nd less reliance on file reading/creating (#15956)

# Overview

This PR follows up both #15855 and #15907 with further refinements and
small changes to defining and setting CSV parameters, along with behind
the scenes refactors.

The two main user/client facing changes of this PR are changing the HTTP
status code when invalid data file IDs are sent to the `POST` `/runs`,
`/protocols` and `/protocols/{protocolId}/analyses` endpoints from 400
to 422, and raising a new `IncompatibleParameterError` when multiple CSV
file parameters are added to a single protocol. The change in status
code is to reflect that the error is not because of a malformed request,
but because there was something in the request that could not be
processed (in this case being the data file ID). The new error was added
to align with the original PRD and the current UI. While there's no
inherent technical limitation for multiple CSV parameters (other than
the theoretical limitations with memory, number of file handlers, etc),
there are unexpected UI bugs when multiple ones are defined and this
change enforces only one per protocol.

The other major change of this PR is a non-user facing refactor of how
we set the `CSVParameter` interface. Rather than passing the `Path` of
the file to the interface and then using a `TemporaryFile` as the basis
for all further access of the CSV file and it's contents, now the
contents of the file (passed as `bytes`) is what everything else is
based off of, including CSV parsing in `parse_as_csv`. With this change,
we only ever make a temporary file handler when the user accesses
`.file`. With this change reducing the chance of there being an open
file handler, a new `file_opened` property was added to the public
interface to reduce needless file opening. This is technically
user-facing code meant more for internal usage, but I see no danger in
exposing it, though if needed we can tag it is a private non-guaranteed
method.

## Changelog

- Raise `IncompatibleParameterError` when `add_csv_file` in the
`ParameterContext` is used more than once
- Change status code of invalid data file IDs posted to runs and
protocols endpoints from `400` to `422`
- Refactor `CSVParameter` to only open temporary file handler when user
requests one

## Review requests

Should we label `file_opened` as private/use another way of preventing
spurious temporary file creation just to close them?

## Risk assessment

Low.
This PR fixes an error where a whitescreen occurs when opening device
details for a robot with robot server <7.3. Move `runTimeParameters`
property from `LegacyGoodRunData` to `KnownGoodRunData` since robot
server does not return RTP for server versions <7.3. Check for
`runTimeParameters` property explicitly when needed.
Closes RQA-2951

In Error Recovery (and DTWiz in general), the pipette is z-homed before users move to a location to drop tips. Because this location is quite far from the deck, users will likely want to press their arrow keys to jog the pipette quickly, and this will in turn cause the robot to execute potentially 50+ moveRelative commands with no way of stopping it.

While not debouncing other flows with jog controls (ex, LPC) has historical context and is less prone to users over-clicking the jog buttons, we should debounce the jog controls within dt wiz.

The solution here is to allow a queue of MAX_QUEUED_JOGS. Three is reasonable after testing.
# Overview

Closes [RQA-2923](https://opentrons.atlassian.net/browse/RQA-2923).
show warnings/errors depending on the run status.
modal header.
show error details on ODD if run completed with errors.

## Test Plan and Hands on Testing

- run a protocol with ER errors on ODD and desktop app.
- make sure that when the run is complete you are able to see the list
of errors.
- warnings shown when run is successfully completed, errors if canceled
or failed.

## Changelog

- inject run status to `RunFailedModal`.
- changed modal title
- changed text to show warnings/errors depending on the run status. 
- ODD - show view error details if there is an error list.

## Risk assessment

low. bug fixes.


[RQA-2923]:
https://opentrons.atlassian.net/browse/RQA-2923?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
Closes RQA-2934

We can safely get the pipetteId from a failed command that involves a pipette, so let's use that instead of loading a pipette with a custom pipetteId.
On modal produced by desktop app `DeviceLanding` "See how to setup a new
robot" link, shrink link clickable area to fit content.

Closes RQA-2961
@mjhuff mjhuff requested review from a team as code owners August 13, 2024 13:07
@mjhuff mjhuff requested review from shlokamin and removed request for a team August 13, 2024 13:07
@mjhuff
Copy link
Contributor Author

mjhuff commented Aug 13, 2024

Looks like CI is 504ing right now. Will re-run it in a bit.

@mjhuff mjhuff force-pushed the 8.0.0-alpha.0-mergeback_2 branch from 40515e6 to d7b2b50 Compare August 13, 2024 13:38
Copy link
Contributor

@SyntaxColoring SyntaxColoring left a comment

Choose a reason for hiding this comment

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

Thanks. Python changes look good, looks good to merge once CI passes.

@mjhuff mjhuff merged commit 6ffeef8 into edge Aug 13, 2024
69 checks passed
@mjhuff mjhuff deleted the 8.0.0-alpha.0-mergeback_2 branch August 13, 2024 14:05
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.

8 participants