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): delete runs and protocols from ODD #12319

Merged
merged 11 commits into from
Apr 17, 2023

Conversation

shlokamin
Copy link
Member

@shlokamin shlokamin commented Mar 20, 2023

Overview

This PR enables users to delete protocols from the ODD. In order to do this, we must first delete a protocol's associated run ids.

Note: still need to add a loading state but waiting for designs.

closes RCORE-364, RCORE-365

Test Plan

  • Created a protocol and ran it so that the server marks it as used
  • Deleted protocol from protocol details page + long press menu and verified the protocol record + run record(s) were removed from server

Review requests

Delete a protocol from the long press modal + protocol details page and make sure the protocol actually gets deleted

Risk assessment

Low

wip

Unverified

No user is associated with the committer email.
@codecov
Copy link

codecov bot commented Mar 20, 2023

Codecov Report

Merging #12319 (634a5fa) into edge (d06eed1) will decrease coverage by 0.02%.
The diff coverage is 54.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #12319      +/-   ##
==========================================
- Coverage   73.66%   73.65%   -0.02%     
==========================================
  Files        2252     2252              
  Lines       61974    62019      +45     
  Branches     6517     6529      +12     
==========================================
+ Hits        45656    45682      +26     
- Misses      14757    14764       +7     
- Partials     1561     1573      +12     
Flag Coverage Δ
app 72.21% <54.83%> (-0.05%) ⬇️
react-api-client 74.48% <ø> (ø)

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

Impacted Files Coverage Δ
...DeviceDisplay/ProtocolDashboard/PinnedProtocol.tsx 70.58% <ø> (ø)
.../pages/OnDeviceDisplay/ProtocolDashboard/index.tsx 14.06% <ø> (ø)
...rc/pages/OnDeviceDisplay/ProtocolDetails/index.tsx 63.75% <54.54%> (-0.14%) ⬇️
...DeviceDisplay/ProtocolDashboard/LongPressModal.tsx 52.00% <55.00%> (+4.63%) ⬆️

... and 2 files with indirect coverage changes


export function LongPressModal(props: {
longpress: UseLongPressResult
protocol: ProtocolResource
protocolId: string
Copy link
Member Author

Choose a reason for hiding this comment

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

long press modal was only ever using the protocol id, it didn't need the whole protocol resource object

@@ -128,6 +142,7 @@ export function LongPressModal(props: {
height="4.875rem"
padding={SPACING.spacing5}
onClick={handleRunClick}
as="button"
Copy link
Member Author

Choose a reason for hiding this comment

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

added these button tags for for accessibility

@shlokamin shlokamin marked this pull request as ready for review April 17, 2023 02:08
@shlokamin shlokamin requested review from a team as code owners April 17, 2023 02:08
@shlokamin shlokamin requested review from smb2268, ewagoner, koji and b-cooper and removed request for a team April 17, 2023 02:08
Copy link
Contributor

@b-cooper b-cooper left a comment

Choose a reason for hiding this comment

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

Code looks good 👍

@@ -18,7 +18,14 @@ export interface ProtocolMetadata {
}

export interface Protocol {
links?: ResourceLinks
links?: {
Copy link
Contributor

Choose a reason for hiding this comment

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

This typing change seems to be a more specific definition of what was already there, not necessarily something new. It's a pretty foundational change, though, since Protocol is used so much. What are the implications for it, and should ResourceLinks itself get changed to match, since it's used in this file by ProtocolAnalyses and Protocols?

Copy link
Member Author

Choose a reason for hiding this comment

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

resource links on the server side are typed per route. when we implemented types on the frontend we just made a generic type and shared it across all endpoints. so the Protocols type that gets retuned from the /protocols endpoint wont have this same interface

@shlokamin shlokamin merged commit aab3b51 into edge Apr 17, 2023
@shlokamin shlokamin deleted the app_delete-runs-and-protocols-ODD branch April 17, 2023 17:13
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.

None yet

3 participants