Can only search and lookup verified records #954
Conversation
There was a problem hiding this comment.
This looks good to me, I think the test coverage is sufficient and they were pretty easy to interpret - it made me think if there are other opportunities in the rest of the codebase where we can employ the subtest technique, maybe on some of the other "mega" endpoint tests?
| } | ||
| _, err := client.Lookup(ctx, request) | ||
| require.EqualError(err, "rpc error: code = NotFound desc = could not find VASP by ID") | ||
| }) |
There was a problem hiding this comment.
I like the isolation of the subtests; it makes it way easier to read!
There was a problem hiding this comment.
I agree that the other mega endpoint tests could use a test breakdown like this - it makes it far easier to debug the tests too since each test is run independently.
| require.NoError(err) | ||
| require.Empty(reply.Error) | ||
| require.Len(reply.Results, 0) | ||
| require.Equal(hotelVASP.VerificationStatus, pb.VerificationState_VERIFIED, "expected fixture to be in verified state") |
| require.Len(reply.Results, 1) | ||
| require.Equal(novemberVASP.Id, reply.Results[0].Id) | ||
|
|
||
| // Prefix search must have at least three characters |
There was a problem hiding this comment.
Is this behavior documented on the Search endpoint?
There was a problem hiding this comment.
That's a good question -- the documentation doesn't mention it. Might be something to comment on.
Scope of changes
This PR adapts the GDS Search and Lookup RPCs to only returned VERIFIED records. The VerificationStatus RPC still returns any state, but you must have the ID or common name for this lookup so it is safer than Search and Lookup.
Type of change
Acceptance criteria
The change is pretty simple, just a couple of lines of code in the Search and Lookup RPCs. The tests had to be radically altered however. I used a subtest method to isolate our different tests. Are the tests we have sufficient?
Author checklist
Reviewer(s) checklist