feat: Validate credential record state transitions #130
Conversation
Signed-off-by: Jakub Koci <[email protected]>
Signed-off-by: Jakub Koci <[email protected]>
Signed-off-by: Jakub Koci <[email protected]>
Signed-off-by: Jakub Koci <[email protected]>
| @@ -266,6 +280,12 @@ export class CredentialService extends EventEmitter { | |||
| return this.credentialRepository.find(id); | |||
| } | |||
|
|
|||
| private validateState(current: CredentialState, valid: CredentialState) { | |||
There was a problem hiding this comment.
Maybe call it assertState instead? To me validate state suggests there is some custom validation done in the function, while it basically checks if the first state passed is equal to the second state passed.
| private validateState(current: CredentialState, valid: CredentialState) { | |
| private assertState(current: CredentialState, expected: CredentialState) { |
There was a problem hiding this comment.
That's good point, let's change it. Updated.
There was a problem hiding this comment.
I'm thinking that we could create another object, something like CredentialStateMachine which would be responsible for such assertions and state transitions. It could simplify tests. I haven't thought through all the details yet.
| @@ -167,24 +169,25 @@ export class CredentialService extends EventEmitter { | |||
| * @param credentialId Credential record ID | |||
| * @param credentialResponseOptions | |||
| */ | |||
| public async createCredentialResponse( | |||
| public async createResponse( | |||
There was a problem hiding this comment.
I'm not sure createResponse is the best name? I'm thinking of createCredential. Then we could also have processCredential
There was a problem hiding this comment.
Hmm, I understand. I also had a problem with this naming. It could be confused with an HTTP request/response. However, now we have consistency at least. Otherwise, we would need to change also other methods, I think. If you look at it from the protocol perspective, this naming is not actually that bad (createRequest, createResponse). Maybe, this message creation and processing stuff could evolve into something called CredentialProtocol calling services to do other tasks (saving credential record, creating protocol attachments, ...)
Signed-off-by: Jakub Koci <[email protected]>
I added validation to every credential service method, not sure if it is necessary. I use different parametrized tests than
jest.each. I'm aware that's not so simple to read in this way, but I wanted to have just one test for every invalid state. This should fix #123.I also added two unrelated tests for credential record attributes.