feat(app, robot-server): add protocol receipt toasts and protocol ids endpoint #12725

shlokamin · 2023-05-17T18:51:56Z

Overview

This fUlLsTaCk PR adds an endpoint that returns all protocol ids on the robot server, and uses it to render protocol receipt toasts in the app.

Closes RCORE-490 and RCORE-714

Test Plan

Sent protocol to a flex
Verified a protocol receipt toast shows up

Changelog

Add protocol ids endpoint to robot server
Render protocol receipt toast on ODD when a new protocol is added to a Flex

Review requests

See test plan

Risk assessment

Low

codecov · 2023-05-17T19:00:12Z

Codecov Report

Merging #12725 (2f9b8d8) into edge (61ce223) will decrease coverage by 0.03%.
The diff coverage is 10.81%.

@@            Coverage Diff             @@
##             edge   #12725      +/-   ##
==========================================
- Coverage   73.35%   73.33%   -0.03%     
==========================================
  Files        2263     2266       +3     
  Lines       61497    61603     +106     
  Branches     6788     6811      +23     
==========================================
+ Hits        45112    45174      +62     
- Misses      14763    14813      +50     
+ Partials     1622     1616       -6

Flag	Coverage Δ
app	`71.63% <10.71%> (-0.05%)`	⬇️
react-api-client	`71.30% <11.11%> (-1.55%)`	⬇️

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

Impacted Files	Coverage Δ
...ot-server/robot_server/protocols/protocol_store.py	`94.73% <ø> (ø)`
robot-server/robot_server/protocols/router.py	`100.00% <ø> (ø)`
...t-server/robot_server/service/json_api/response.py	`100.00% <ø> (ø)`
app/src/App/hooks.ts	`12.76% <3.84%> (-11.05%)`	⬇️
...api-client/src/protocols/useAllProtocolIdsQuery.ts	`11.11% <11.11%> (ø)`
app/src/App/OnDeviceDisplayApp.tsx	`74.19% <100.00%> (+1.77%)`	⬆️

... and 43 files with indirect coverage changes

vegano1

Looks good to me pending testing
Just left one comment
I'm also wondering if we will see the same notifications when the robot-app restarts? Since the odd only keeps track of what it's seen in memory

vegano1 · 2023-05-19T22:47:26Z

robot-server/robot_server/protocols/protocol_store.py

+        select_ids = sqlalchemy.select(protocol_table.c.id)
+        with self._sql_engine.begin() as transaction:
+            protocol_ids = transaction.execute(select_ids).scalars().all()
+        return protocol_ids


Maybe have a default value in case we fail to initialize protocol_id variable because of a failed query

I think this question came up in another PR that I'm having trouble digging up, but I think the way that this is written now is good.

If the transaction fails, it will raise an exception out of the with ... block before protocol_ids is used. Providing a default value for protocol_ids would be misleading because it would never actually be returned.

talked irl, leaving as is so the robot server returns an error if the database transaction fails

Yeah I would agree with that, ignore my comment
In the case this does fail, we would return the appropriate HTTP response when handling the exception.

brenthagen

✔️

brenthagen · 2023-05-22T14:59:45Z

app/src/App/hooks.ts

+  // wrapping protocolIds in a useMemo doesnt fix the issue that this eslint calls out
+  // eslint-disable-next-line react-hooks/exhaustive-deps


i think this eslint disable is unused

SyntaxColoring

Sweet. Here are a few requests for the server side, but this otherwise looks great.

I'm also curious about @vegano1's question:

I'm also wondering if we will see the same notifications when the robot-app restarts? Since the odd only keeps track of what it's seen in memory

robot-server/robot_server/protocols/protocol_store.py

SyntaxColoring · 2023-05-22T15:26:20Z

robot-server/tests/protocols/test_protocols_router.py

Could we also add a /protocols/ids Tavern integration test? We don't need a whole new test file—we can just add a step to robot-server/tests/integration/http_api/protocols/test_upload.tavern.yaml, I think.

The specific thing that I'm concerned about is the ambiguity between GET /protocols/{id} and GET /protocols/ids. The functions need to be declared in the correct order for this to work, and I think the only way to cover that is with Tavern.

SyntaxColoring · 2023-05-22T15:42:23Z

robot-server/robot_server/protocols/router.py

@@ -306,6 +305,28 @@ async def get_protocols(
    )


+@protocols_router.get(
+    path="/protocols/ids",
+    summary="Get uploaded protocol ids",


If I may be a pain in the ass about docs:

Suggested change

summary="Get uploaded protocol ids",

summary="Get uploaded protocol ids",

description=(

"Get the IDs of all protocols stored on the server."

"\n\n"

"**Warning:**"

" This is an experimental endpoint and is only meant for internal use by Opentrons."

" We might remove it or change its behavior without warning."

),

This does two things.

First, the warning is because we're apparently publishing these docs now. Or publishing them imminently? I don't know.

Second, providing anything as a description prevents FastAPI from defaulting to the Python function docstring. We don't want it to do that because it has that "Args: protocol_store" internal junk.

SyntaxColoring · 2023-05-22T15:49:06Z

robot-server/tests/protocols/test_protocols_router.py

+async def test_get_protocol_ids_no_protocols(
+    decoy: Decoy,
+    protocol_store: ProtocolStore,
+) -> None:
+    """It should return an empty collection response with no protocols loaded."""
+    decoy.when(protocol_store.get_all_ids()).then_return([])
+
+    result = await get_protocol_ids(protocol_store=protocol_store)
+
+    assert result.content.data == []
+    assert result.content.meta == MultiBodyMeta(cursor=0, totalLength=0)
+    assert result.status_code == 200
+
+


Up to you, but you could leave this test out, if you wanted.

Suggested change

async def test_get_protocol_ids_no_protocols(

decoy: Decoy,

protocol_store: ProtocolStore,

) -> None:

"""It should return an empty collection response with no protocols loaded."""

decoy.when(protocol_store.get_all_ids()).then_return([])

result = await get_protocol_ids(protocol_store=protocol_store)

assert result.content.data == []

assert result.content.meta == MultiBodyMeta(cursor=0, totalLength=0)

assert result.status_code == 200

Your other test_get_protocol_ids() test makes sure that the router will correctly pass along a ["protocol_id_1", "protocol_id_2"] result from the ProtocolStore to the HTTP client. Our thinking for this kind of thing has been that if it can do that, it will naturally work if the result is [] instead.

We can get away with this non-exhaustiveness because the router function is written as a "collaborator," just passing values from one thing to the other, not doing any logic of its own.

Co-authored-by: Max <[email protected]>

shlokamin · 2023-05-22T16:39:48Z

thx for the feedback max! im gonna merge this for now and address in a follow up so we can get this into 0.9.0 and start testing

I'm also wondering if we will see the same notifications when the robot-app restarts? Since the odd only keeps track of what it's seen in memory

the app only renders the toast if it's already refetched. meaning, it must have already refetched data and gotten a different diff in order to render the toast. so here is what happens:

app renders, ids list is empty. no toast render
app fetches ids for the first time, list has 4 ids cuz there were 4 protocols on the server (no toast render, because this is the first fetch)
app refetches ids, list still has 4 ids (no toast render, because this is the first refetch)
app refetches ids, list still has 4 ids (no toast render, because the diff is the same)

shlokamin added 9 commits May 12, 2023 15:42

wip

ddc58f3

add hook to render new protocol toasts

6aead2d

remove basemodel constraint to generic response

367a20f

format

952cb74

add lru cache

98e0984

add api client utils and finish hook

d29da71

Merge branch 'edge' into robot_server-protocol-ids-endpoint

3849c0f

update hook

677a9b9

format

35f3061

shlokamin added 2 commits May 17, 2023 15:12

revert recent run protocols change

edf071b

format py

198b422

shlokamin marked this pull request as ready for review May 17, 2023 19:23

shlokamin requested review from a team as code owners May 17, 2023 19:23

shlokamin requested review from smb2268, sfoster1, SyntaxColoring, b-cooper, jbleon95 and koji and removed request for a team and smb2268 May 17, 2023 19:23

shlokamin added 5 commits May 17, 2023 15:30

lint-py

8d3bfb0

remove unused dep in test

f01983f

actually lint this time

93240ae

ise i18n

a7c3ab0

update copy

10b21dc

shlokamin changed the title ~~feat(robot-server, app): add protocol receipt toasts and protocol ids endpoint~~ feat(app, robot-server): add protocol receipt toasts and protocol ids endpoint May 17, 2023

shlokamin added 2 commits May 17, 2023 17:47

remove rogue test

2f60138

make sure on device display app is calling toast hook

67a55d8

vegano1 approved these changes May 19, 2023

View reviewed changes

brenthagen approved these changes May 22, 2023

View reviewed changes

remove unused eslint suppression

dbb1149

SyntaxColoring reviewed May 22, 2023

View reviewed changes

order oldest to newest

2f9b8d8

Co-authored-by: Max <[email protected]>

shlokamin merged commit bdb2bce into edge May 22, 2023

shlokamin deleted the robot_server-protocol-ids-endpoint branch May 22, 2023 17:58

SyntaxColoring mentioned this pull request Aug 11, 2023

refactor(robot-server): Followups to GET /protocols/ids #13292

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(app, robot-server): add protocol receipt toasts and protocol ids endpoint #12725

feat(app, robot-server): add protocol receipt toasts and protocol ids endpoint #12725

shlokamin commented May 17, 2023

codecov bot commented May 17, 2023 •

edited

Loading

vegano1 left a comment

vegano1 May 19, 2023

SyntaxColoring May 22, 2023

shlokamin May 22, 2023

vegano1 May 22, 2023

brenthagen left a comment

brenthagen May 22, 2023

SyntaxColoring left a comment •

edited

Loading

SyntaxColoring May 22, 2023 •

edited

Loading

SyntaxColoring May 22, 2023 •

edited

Loading

SyntaxColoring May 22, 2023

shlokamin commented May 22, 2023 •

edited

Loading

		// wrapping protocolIds in a useMemo doesnt fix the issue that this eslint calls out
		// eslint-disable-next-line react-hooks/exhaustive-deps

-    summary="Get uploaded protocol ids",
+    summary="Get uploaded protocol ids",
+    description=(
+        "Get the IDs of all protocols stored on the server."
+        "\n\n"
+        "**Warning:**"
+        " This is an experimental endpoint and is only meant for internal use by Opentrons."
+        " We might remove it or change its behavior without warning."
+    ),

feat(app, robot-server): add protocol receipt toasts and protocol ids endpoint #12725

feat(app, robot-server): add protocol receipt toasts and protocol ids endpoint #12725

Conversation

shlokamin commented May 17, 2023

Overview

Test Plan

Changelog

Review requests

Risk assessment

codecov bot commented May 17, 2023 • edited Loading

Codecov Report

vegano1 left a comment

Choose a reason for hiding this comment

vegano1 May 19, 2023

Choose a reason for hiding this comment

SyntaxColoring May 22, 2023

Choose a reason for hiding this comment

shlokamin May 22, 2023

Choose a reason for hiding this comment

vegano1 May 22, 2023

Choose a reason for hiding this comment

brenthagen left a comment

Choose a reason for hiding this comment

brenthagen May 22, 2023

Choose a reason for hiding this comment

SyntaxColoring left a comment • edited Loading

Choose a reason for hiding this comment

SyntaxColoring May 22, 2023 • edited Loading

Choose a reason for hiding this comment

SyntaxColoring May 22, 2023 • edited Loading

Choose a reason for hiding this comment

SyntaxColoring May 22, 2023

Choose a reason for hiding this comment

shlokamin commented May 22, 2023 • edited Loading

codecov bot commented May 17, 2023 •

edited

Loading

SyntaxColoring left a comment •

edited

Loading

SyntaxColoring May 22, 2023 •

edited

Loading

SyntaxColoring May 22, 2023 •

edited

Loading

shlokamin commented May 22, 2023 •

edited

Loading