URO-206 getting started with orcidlink integration #206

eapearson · 2024-04-29T22:36:17Z

This set of changes introduces a primitive orcidlink feature. A link is displayed on the left nav area, a route is in place, the feature is invoked, and a message is displayed indicating whether the user has an orcid link or not. If so, some orcid link information is shown.

What is not included? Tests, actual (other than stub) redux store data definitions (state types), api calls in the the redux layer (they are in the components themselves), proper error handling.

Some bespoke layout (loading indicator, box around the content) was added just to keep have an implementation - I just couldn’t find anything satisfying to grab quickly.

Some light modifications were made to the jsonrpc api machinery. The orcidlink service is JSON-RPC 2.0, and the existing JSON-RPC support is not fully 2.0 compatible. I also made a more extensive revision of JSON-RPC, because error response support is also needs some improvement, but is not addressed in this ticket.

tweaks to jsonrpc to better support 2.0, detects whether linked, shows 3 fields from link if so, redux state not developed

placeholder icon

dauglyon

Looks good! The only things blocking approval are overrideExisting & hiding the icon from src/features/layout/LeftNavBar.tsx. (We're merging into main, so everything should be presentable if it's directly accessible, just in case we cut a release in the meantime. We have some nascent feature flag support, too, which could hide the page altogether if that's preferable.) I would also recommend stubbing out a test file with an "OrcidLink Renders" test before merging, but it'd just be a formality.

I figure if the parseError comment works out, it can be in the next PR.

Also, Re: queries in components, that's the way to do it with rtk-query. It handles caching for queries with the same query key. Sometimes this requires memoization to leverage but often not. Custom thunks are a good solution if there are dependant queries, however.

dauglyon · 2024-04-29T22:43:58Z

src/common/api/utils/kbaseBaseQuery.ts

  fetchArgs?: FetchArgs;
 }

+export interface JSONRPC11Body {
+  version: string;


Nit: You can hardcode the version string here, or use a template string type if we want to support minor versions

It will make discrimination (type guarding) between this type and the 2.0 type easier downstream

In terms of versions, as far as I know there are only two relevant JSON-RPC versions for us: 1.1 and 2.0. Also, 1.1 is not technically official (although it is used in many places in KBase). That being said, official documents describing their operation do exist, and I suggest that we hew as close to them as possible.

JSON-RPC 1.1 Wd

JSON-RPC 2.0

Yes, agreed @dakotablair, although our JSON-RPC 1.1 is not the same as the working draft that inspired it. So our reference for that should be our implementation.

@dakotablair Yes, the version could be hardcoded for sure; I did have another set of changes that addressed this more fundamentally, but more loc.

dauglyon · 2024-04-29T22:50:56Z

src/features/orcidlink/orcidlinkAPI.ts

+  .injectEndpoints({
+    // because many apis have "status", some have "info", and there may be
+    // other random clashes.
+    overrideExisting: true,


We don't want to do this, as it will squash any same-named endpoints. Just use unique names for your endpoints.

Eg "orcidLinkStatus"

injectEndpoints does not create a new API, but rather adds enpoints to the common baseAPI instance. We set it up like that so that we can expire query tags across services. So, moral of the story, setting this to true will probably cause runtime errors if a collision occurs.

This also means you can export the endpoints themselves (like we do elsewhere) without worrying about duplicates

@dauglyon thanks, you answered the question I had, though I thought my original researched showed this was safe. Thanks, Google!

dauglyon · 2024-04-29T22:52:13Z

src/features/orcidlink/orcidlinkAPI.ts

+// It is mostly a JSONRPC 2.0 service, although the oauth flow is rest-ish.
+const orcidlinkService = jsonRpcService({
+  url: '/services/orcidlink/api/v1',
+  version: '2.0',


dauglyon · 2024-04-29T22:55:31Z

src/features/layout/LeftNavBar.tsx

@@ -37,6 +38,7 @@ const LeftNavBar: FC = () => {
        <NavItem path="/legacy/search" desc="Search" icon={faSearch} />
        <NavItem path="/legacy/jobbrowser" desc="Jobs" icon={faSuitcase} />
        <NavItem path="/legacy/account" desc="Account" icon={faIdCard} />
+        <NavItem path="/orcidlink" desc="ORCID Link" icon={faLink} />


We should put this behind a feature flag or hide it from the sidebar whilst orcidlink is in development.

Suggested change

<NavItem path="/orcidlink" desc="ORCID Link" icon={faLink} />

{/*<NavItem path="/orcidlink" desc="ORCID Link" icon={faLink} />*/}

Agreed, and my thought exactly; are there feature flags used in the codebase?

dauglyon · 2024-04-29T22:57:53Z

src/common/api/utils/kbaseBaseQuery.ts

@@ -172,19 +189,35 @@ export const kbaseBaseQuery: (
      // Generate JsonRpc request id
      const reqId = Math.random().toString();

+      const body = ((): JSONRPCBody => {


dauglyon · 2024-04-29T23:06:04Z

src/features/orcidlink/ErrorMessage.tsx

+  const message = (() => {
+    if ('status' in error) {
+      switch (error.status) {
+        case 'JSONRPC_ERROR':
+          return error.data.error.message;
+        case 'FETCH_ERROR':
+          return 'Fetch Error';
+        case 'CUSTOM_ERROR':
+          return error.error;
+        case 'PARSING_ERROR':
+          return error.error;
+        case 'TIMEOUT_ERROR':
+          return error.error;
+      }
+    } else {
+      return error.message || 'Unknown Error';
+    }


Check out the parseError utility for this functionality. It may need to be updated to support exactly what you want, but will be reusable

src/common/api/utils/parseError.ts

We tend to avoid switch statements. My personal preference is that if we are going to use them then we should make use of their special features, in particular, fall-through and default cases. For example:

switch (error.status) { case 'JSONRPC_ERROR': return error.data.error.message; case 'FETCH_ERROR': return 'Fetch Error'; case 'CUSTOM_ERROR': case 'PARSING_ERROR': case 'TIMEOUT_ERROR': default: return error.error; }

I think many other switch users write code as you have to avoid confusion about these features, and it is this situation that has led us to avoid them on this project. There is another open PR with what I think is a good alternative.

I can abide by the codebase conventions. Yeah, first pass was not to use fallthrough because I hadn't explored those error conditions to see if there was more specialized information before; e.t. timeout in theory should have information about the timeout like the timeout limit and actual elapsed time. But, like I said, I didn't have time on this batch to create a test to exercise the conditions.

dauglyon · 2024-04-29T23:11:50Z

src/features/orcidlink/Linked.tsx

+  } else if (isSuccess) {
+    return renderLink(data);
+  } else {
+    return <div>???</div>;


I assume this is to appease typescript re: the function return type?

Yes, it is something I still wanted to follow up on, as I don't yet understand the full set of states for the various query state flags.

dauglyon · 2024-04-30T04:12:41Z

src/features/orcidlink/orcidlinkAPI.ts

We have been putting all the API defs in src/common/api/
That said, I can see the argument for feature-specific APIs like this and collections being in the feature folder itself. Thoughts? It's probably something to quickly talk about on the UI call

Yeah, I meant to move the api over to the centralized api directory, but forgot that piece of housekeeping.

eapearson · 2024-04-30T18:44:02Z

Looks good! The only things blocking approval are overrideExisting & hiding the icon from src/features/layout/LeftNavBar.tsx. (We're merging into main, so everything should be presentable if it's directly accessible, just in case we cut a release in the meantime. We have some nascent feature flag support, too, which could hide the page altogether if that's preferable.) I would also recommend stubbing out a test file with an "OrcidLink Renders" test before merging, but it'd just be a formality.

I figure if the parseError comment works out, it can be in the next PR.

Also, Re: queries in components, that's the way to do it with rtk-query. It handles caching for queries with the same query key. Sometimes this requires memoization to leverage but often not. Custom thunks are a good solution if there are dependant queries, however.

Yeah, I think there is a need for sequenced queries like this that I would usually do in an async thunk if working w/ redux.

and a bit of cleanup

without feature switches or being able to restrict to devs, we don't want the navigation to leak out in a release

eapearson added 2 commits April 29, 2024 15:18

create stub orcidlink feature [URO-206]

0ed2b8d

tweaks to jsonrpc to better support 2.0, detects whether linked, shows 3 fields from link if so, redux state not developed

add orcidlink to navigation and routes [URO-206]

73f086a

placeholder icon

dauglyon self-requested a review April 30, 2024 04:15

dauglyon reviewed Apr 30, 2024

View reviewed changes

eapearson added 2 commits April 30, 2024 10:03

adjust title [URO-206]

9d1eebb

add orcidlink to hamburger menu [URO-206]

c003f80

dakotablair approved these changes Apr 30, 2024

View reviewed changes

eapearson added 4 commits April 30, 2024 12:48

namespace api calls (endpoints), remove overrideExisting [URO-206]

682f010

and a bit of cleanup

replace ??? placeholder with CUSTOM_ERROR [URO-206]

3935be2

move orcidlink api file into common directory [URO-206]

7e4987a

remove orcidlink navigation [URO-206]

bcbf09a

without feature switches or being able to restrict to devs, we don't want the navigation to leak out in a release

eapearson merged commit 38d1743 into main May 2, 2024
4 checks passed

eapearson deleted the URO-206 branch May 2, 2024 23:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

URO-206 getting started with orcidlink integration #206

URO-206 getting started with orcidlink integration #206

eapearson commented Apr 29, 2024

dauglyon left a comment •

edited

Loading

dauglyon Apr 29, 2024

dakotablair Apr 30, 2024

eapearson Apr 30, 2024

dauglyon Apr 29, 2024

dauglyon Apr 29, 2024 •

edited

Loading

dauglyon Apr 30, 2024

eapearson Apr 30, 2024

dauglyon Apr 29, 2024

dauglyon Apr 29, 2024

dauglyon Apr 30, 2024

eapearson Apr 30, 2024

dauglyon Apr 29, 2024

dauglyon Apr 29, 2024

dauglyon Apr 29, 2024

dakotablair Apr 30, 2024

eapearson Apr 30, 2024

dauglyon Apr 29, 2024

eapearson Apr 30, 2024

dauglyon Apr 30, 2024

eapearson Apr 30, 2024

eapearson commented Apr 30, 2024

	<NavItem path="/orcidlink" desc="ORCID Link" icon={faLink} />
	{/<NavItem path="/orcidlink" desc="ORCID Link" icon={faLink} />/}

URO-206 getting started with orcidlink integration #206

URO-206 getting started with orcidlink integration #206

Conversation

eapearson commented Apr 29, 2024

dauglyon left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dauglyon Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eapearson commented Apr 30, 2024

dauglyon left a comment •

edited

Loading

dauglyon Apr 29, 2024 •

edited

Loading