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 view error details does nothing #12732

Merged
merged 5 commits into from
May 22, 2023

Conversation

koji
Copy link
Contributor

@koji koji commented May 17, 2023

Overview

[design]
https://www.figma.com/file/AoTLAYuWawlaWItB1umOjr/Release%3A-Opentrons-Flex-Touchscreen-Designs?type=design&node-id=8860-128824&t=SE4QZemOanWChb3k-0

hook RunFailedModal to the view error details button in RunSummary and update the modal itself since the size and content were updated recently.

going to the option 1
https://www.figma.com/file/AoTLAYuWawlaWItB1umOjr?node-id=8963:248012#447508536

Will be updated if the design is changed.

close RQA-814

Test Plan

  • run a protocol which will be failed
  • tap view error details button

Changelog

  • update RunFaildModal (removed red frame)
  • add RunFailedModal to RunSummary component

Review requests

Currently errorType in Error: errorType is empty since errorType we can retrieve doesn't represent the entire errors.
Should we display the first error's errorType? eg runRecord.data.errors[0].errorType?

Risk assessment

low

hook RunFailedModal to the vew error details button in RunSummary

RQA-814
@koji koji marked this pull request as ready for review May 17, 2023 22:36
@koji koji requested a review from a team as a code owner May 17, 2023 22:36
@koji koji removed the request for review from a team May 17, 2023 22:36
@codecov
Copy link

codecov bot commented May 17, 2023

Codecov Report

Merging #12732 (109bdeb) into edge (61a3290) will decrease coverage by 0.07%.
The diff coverage is 25.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #12732      +/-   ##
==========================================
- Coverage   73.43%   73.37%   -0.07%     
==========================================
  Files        2291     2292       +1     
  Lines       62861    63326     +465     
  Branches     6775     6969     +194     
==========================================
+ Hits        46163    46464     +301     
- Misses      15082    15217     +135     
- Partials     1616     1645      +29     
Flag Coverage Δ
app 71.51% <25.00%> (-0.25%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
app/src/pages/OnDeviceDisplay/RunSummary.tsx 13.20% <0.00%> (-0.52%) ⬇️
...OnDeviceDisplay/RunningProtocol/RunFailedModal.tsx 80.00% <100.00%> (-2.36%) ⬇️

... and 34 files with indirect coverage changes

@koji koji marked this pull request as draft May 18, 2023 15:19
@koji koji marked this pull request as ready for review May 18, 2023 19:11
Comment on lines 77 to 79
{/* (kj:05/17/2023) errorType will be added when the endpoint is ready */}
{t('error_type', {
errorType: '',
Copy link
Member

Choose a reason for hiding this comment

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

errorType is already returned from the runs endpoint right? we should already have it inside of the errorType prop in RunError:

interface RunError {
  id: string
  errorType: string
  createdAt: string
  detail: string
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But it doesn't represented the entire error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in the Desktop app, errorType isn't used but just parse errors[] and display details.

For this modal errorType came from a new error endpoint. Actually, I posted that current errorType that we can display doesn't represent the entire errors. But the design hasn't been update.

We can display errorType from the errors[0].details but I guess that may cause confusion because if there are more than 2 error details, a couple of errors might not related to the errorType completely.

Copy link
Member

Choose a reason for hiding this comment

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

i believe the only data we might get that dosent yet exist is the error code (cc @fsinapi).

let's sync on this during standup and try to get this merged before the afternoon

Copy link
Member

Choose a reason for hiding this comment

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

the backend won't ever give us a top level error type — global error state was cut from scope. i updated the component to show the first error type in the list of errors for now. just verified with @sanni-t that this should give users sufficient info

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.

i can't seem to get the error modal to render because i get redirected to the robot dashboard when i cancel the run. i think that might be because of the TopLevelRedirects brian and i added... ill look into it though

@koji
Copy link
Contributor Author

koji commented May 19, 2023

i can't seem to get the error modal to render because i get redirected to the robot dashboard when i cancel the run. i think that might be because of the TopLevelRedirects brian and i added... ill look into it though

hmm, on my end I could open the modal...

@shlokamin shlokamin merged commit 56f246f into edge May 22, 2023
@shlokamin shlokamin deleted the fix_view-error-details-modal branch May 22, 2023 19:39
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