Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces several new adapters to the epicenter-libs package, including automaton, context, dashboard, encyclopedia, fido2, forge, information, and notification. Each new adapter comes with comprehensive unit tests and updated type definitions. Additionally, common constants like MODEL_LANGUAGE and MORPHOLOGY have been centralized. Feedback highlights a necessary correction to the return type of the dashboard getAll method, the removal of redundant await keywords when chaining promises, and improvements for consistency in imports and test data access.
| @@ -0,0 +1,112 @@ | |||
| import type { RoutingOptions } from '../utils/router'; | |||
|
|
|||
| import Router from '../utils/router'; | |||
There was a problem hiding this comment.
^ I agree with keeping this consistent
| it('Should send the credential in the body', async () => { | ||
| await fido2Adapter.register(credential); | ||
| const req = capturedRequests[capturedRequests.length - 1]; | ||
| const body = JSON.parse(req.requestBody); |
There was a problem hiding this comment.
There's an inconsistency in how the request body is accessed in the new test files. Some use req.requestBody while others use req.options.body. To maintain consistency across the test suite, please prefer using req.options.body.
This comment also applies to line 165 in this file and similar instances in tests/forge.spec.js.
| const body = JSON.parse(req.requestBody); | |
| const body = JSON.parse(req.options.body); |
There was a problem hiding this comment.
this is a bit of a style nitpick but I do agree, and req.options.body is consistent with the rest of hte test suite
| * @param [optionals] Optional arguments; pass network call options overrides here. | ||
| * @returns promise that resolves when the translation is complete (204 no content) | ||
| */ | ||
| export async function translate( |
There was a problem hiding this comment.
The return type here is inaccurate per the Epicenter source, looks like the type varies by translator:
- ASCIIDOC → text/plain
- ASCIIDOC_TO_HTML → text/html
- OPENAPI → application/json
Since Router is designed for JSON only (and throws on other types), we can follow the pattern that the assetAdapter uses to get the URL then do a direct fetch. Something like:
export async function translate(version, api, translator, optionals = {}) {
const url = new Router()
.getURL(`/encyclopedia/as/${translator}/v${version}/${api}`, optionals)
.toString();
const session = authAdapter.getLocalSession();
const response = await fetch(url, {
headers: session ? { Authorization: `Bearer ${session.token}` } : {},
});
return translator === 'OPENAPI' ? response.json() : response.text();
}Then the return type of the function would be Promise<string | object>
| @@ -0,0 +1,112 @@ | |||
| import type { RoutingOptions } from '../utils/router'; | |||
|
|
|||
| import Router from '../utils/router'; | |||
There was a problem hiding this comment.
^ I agree with keeping this consistent
| * | ||
| * @param modelLanguage The model language to check readiness for | ||
| * @param [optionals] Optional arguments; pass network call options overrides here. | ||
| * @returns promise that resolves when the check is complete (204 no content) |
There was a problem hiding this comment.
Looks likeready returns a boolean (actually the only forge endpoint that doesn't return void)
| * @param optOutToken Token used to opt out of notifications | ||
| * @param [optionals] Optional arguments; pass network call options overrides here. | ||
| * @returns promise that resolves to the opt-out result | ||
| */ |
There was a problem hiding this comment.
Looks like both optOut and optOutByType return a 303 redirect (they're email links that would be executed in a browser, rather than a fetch call). So for the libs purposes, the return type would just be Promise<void> and we can explain the 303 in the docstring (or maybe not even include them since this wouldn't ever be called this way, but if we're keeping them in for completeness, fix return type and document behavior)
|
|
||
| export interface NotificationPreferenceCreateInView { | ||
| notificationType?: NotificationType; | ||
| adminKey?: string; |
There was a problem hiding this comment.
per NotificationResource in the Epicenter source, adminKey is required (so just drop the ?)
| export interface DashboardPreferenceReadOutView { | ||
| projectKey: string | null; | ||
| adminKey: string | null; | ||
| items: Record<string, unknown>; |
There was a problem hiding this comment.
per DashboardPreference in the java, items is also nullable -
items: Record<string, unknown> | null;
|
|
||
| it('Should pass executionContext in the request body', async () => { | ||
| const executionContext = { | ||
| version: 1, |
There was a problem hiding this comment.
With the update of the execution context version type to string (or V1), this should also change to V1
| it('Should send the credential in the body', async () => { | ||
| await fido2Adapter.register(credential); | ||
| const req = capturedRequests[capturedRequests.length - 1]; | ||
| const body = JSON.parse(req.requestBody); |
There was a problem hiding this comment.
this is a bit of a style nitpick but I do agree, and req.options.body is consistent with the rest of hte test suite
Created with the help of AI
Not entirely sure what some endpoints do so i may have made some wrong assumptions or added inaccurate descriptions