766 store circuit inputs translation schema #783
Conversation
…o assert error handling
…cuit input is provided in the circuit input parser; Simplify calculateStringCharCodeSum call in case of string type
…ircuitInputParser service
…d to postman collection; Rename add cis command to set cis command;
…hrow error in case validation fails
ognjenkurtic
requested review from
skarred14,
Therecanbeonlyone1969,
Kasshern,
Ybittan,
biscuitdey and
skosito
as code owners
ognjenkurtic
changed the base branch from
main
to
feat/776-circuit-inputs-parser
...ri/workgroup/worksteps/capabilities/setCircuitInputsSchema/setCircuitInputsSchema.command.ts
Outdated
Show resolved
Hide resolved
@Therecanbeonlyone1969 Please have a look at my answers and let me know if there is anything else to clarify |
see my replies ... JSON and use a library such as suretype. @ognjenkurtic |
| public throwIfCircuitInputTranslationSchemaInvalid(schema): void { | ||
| const [result, error] = | ||
| this.cips.validateCircuitInputTranslationSchema(schema); | ||
| if (!result) { |
There was a problem hiding this comment.
probably should check if there is error instead of result and returning error imo
There was a problem hiding this comment.
renamed 'result' to 'valid' for better readability
| workstepToUpdate: Workstep, | ||
| schema: string, | ||
| ): void { | ||
| workstepToUpdate.updateCircuitInputTranslationSchema(schema); |
There was a problem hiding this comment.
Not really, asigning string value
| @@ -71,6 +73,17 @@ export class WorkstepController { | |||
| ); | |||
| } | |||
|
|
|||
| @Put('/:id/circuitinputsschema') | |||
| @CheckAuthz({ action: 'update', type: 'Workstep' }) | |||
| async setCircuitInputsSchemaCommand( | |||
There was a problem hiding this comment.
small thing but i would name this update instead of set to be clear, but up to you
| } | ||
| } | ||
|
|
||
| return [true, '']; |
There was a problem hiding this comment.
i dont think we should return both boolean and error, looks kinda weird in javascript, lets just return error which implies that validation is true or false? but up to you
There was a problem hiding this comment.
@ognjenkurtic are you going to leave it like that? L57 still returns a boolean and error message. Same with L55 ... I would return a No-Error Message
|
|
Description
Implements an endpoint through which a circuit input translation schema can be attached to a workstep.
Implements a command handler that validates the provided schema and stores it.
Implements a method in the CircuitInputsParserService for schema validation together with tests
Extends the e2e test to trigger dummy schema creation, in preparation for #777
Related Issue
#766
Motivation and Context
Given in the discussions in the related issues and docs
How Has This Been Tested
Wrote a set of new unit tests
Extended e2e tests
Added an endpoint to Postman collection and tested manually
Screenshots (if appropriate)
Types of changes
Checklist