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

fix(app): Fix "run again" button after cancelling a run #15987

Merged
merged 3 commits into from
Aug 14, 2024

Conversation

mjhuff
Copy link
Contributor

@mjhuff mjhuff commented Aug 13, 2024

Closes RQA-2982

Overview

After error recovery and drop tip changes, run context is required after the run is cancelled. Before, if the run was cancelled, the app would implicitly close the run context. We can't do that any longer, but in order for some setup buttons to work properly, we need to close the run context somehow.

Currently, after a run is cancelled, the "run again" button is greyed out if a user hasn't calibrated their instruments, although they were just running the protocol prior to cancelling it (note that users can't actually play a run without calibration, but they can create a run). Let's allow users to "run again" (create a run), and then be in the proper run state to perform setup. I don't know if this ever was the intended behavior of protocol runs in the past, but a lot of the existing copy points to this behavior being desired, anyway.

EDIT: After giving this more thought, while I do think we should make the "run again" button selectable to remain consistent with copy and behavioral expectations, it's possible to retain the old behavior as well and close the current run when run status is stopped. It requires more special casing within ProtocolRunHeader, but isRunCurrent is really something only utilized by drop tip CTAs at the end of the day, so it's not too difficult to reason about. Given the overall complexity of these interactions, I think it's best not to push our luck in assuming what does/doesn't work until we formally refactor all of this.

TLDR: From an end user perspective, nothing changes between 7.X and 8.0. Functionally, we special-case the drop tip logic to play nicely with run currenting/uncurrenting, tie the new recovery error rendering logic to isMostRecentRun (effectively what we were doing in 7.X), and now give the client an escape hatch if the run is cancelled but doesn't close the current run context.

Test Plan and Hands on Testing

  • Verified users can now calibrate instruments given the behavior described in the ticket.

Changelog

  • The run again button is no longer disabled after a run is cancelled.

Risk assessment

low

@mjhuff mjhuff requested a review from a team as a code owner August 13, 2024 20:52
@mjhuff mjhuff marked this pull request as draft August 13, 2024 23:31
@mjhuff mjhuff marked this pull request as ready for review August 14, 2024 00:07
@mjhuff mjhuff force-pushed the app_fix-run-again-button branch from 0d35e7d to 71ed017 Compare August 14, 2024 13:22
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.

Discussed in-person. I have like a 30% understanding of the problem and I'm not really qualified to review the code, but it seems like this is tested and we have ideas to make it less confusing in the future, so LGTM.

@mjhuff mjhuff force-pushed the app_fix-run-again-button branch 2 times, most recently from ee61901 to a200fed Compare August 14, 2024 15:09
@mjhuff mjhuff merged commit 1b29655 into chore_release-8.0.0 Aug 14, 2024
20 checks passed
@mjhuff mjhuff deleted the app_fix-run-again-button branch August 14, 2024 15: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.

2 participants