Fix: Deployed by filter capitalization #479
Conversation
| */ | ||
|
|
||
| export async function up(pgm: MigrationBuilder): Promise<void> { | ||
| pgm.sql(`CREATE INDEX deployer_address_lower_case on deployments (LOWER(deployer_address) text_pattern_ops);`) |
There was a problem hiding this comment.
Should we delete the original index on deployer_address ?
| when(db.map(anything(), anything(), anything())).thenReturn(Promise.resolve(dbResult)) | ||
| }) | ||
|
|
||
| it('should return a map of entity id to the deployment status', async () => { |
There was a problem hiding this comment.
Please update this text to reflect what you are verifying
There was a problem hiding this comment.
This is the correct verification, splitted the test for two different verifications
| const entities = ['1', '3'] | ||
| const result = await repository.areEntitiesDeployed(entities) | ||
|
|
||
| expect(result).toEqual( |
There was a problem hiding this comment.
I'd remove this as the only thing that you are testing is the mock
There was a problem hiding this comment.
This is asserting on the logic of the map creation in the return statement of the function
| }) | ||
| }) | ||
|
|
||
| describe('when the entities list is empty', () => { |
There was a problem hiding this comment.
This test doesn't seem to add value as you are only testing the mock.
There was a problem hiding this comment.
This is asserting on the first if of the function (
) that's why It also asserts onnever called of the mock
| const args = capture(db.map).last() | ||
|
|
||
| expect(args[0]).toContain(`ORDER BY dep1.entity_timestamp ASC`) | ||
| expect(args[0]).toContain( |
| describe('when it receives a lastId', () => { | ||
| const lastId = '1' | ||
|
|
||
| beforeEach(() => { |
There was a problem hiding this comment.
As we don't really care about the response from the MockedDataBase can we configure this only once for the entire file?
There was a problem hiding this comment.
We can, but that makes the tests coupled since the mock is the same among all tests
There was a problem hiding this comment.
That's why I initialized it in each test context
| }) | ||
|
|
||
| it('should add the expected where clause to the query with the addresses on lowercase', async () => { | ||
| const deployedBy = ['jOn', 'aGus'] |
| const args = capture(db.map).last() | ||
|
|
||
| expect(args[0]).toContain(`LOWER(dep1.deployer_address) IN ($(deployedBy:list))`) | ||
| expect(args[1]).toEqual(jasmine.objectContaining({ deployedBy: deployedBy.map((x) => x.toLowerCase()) })) |
There was a problem hiding this comment.
nit: as this is the expected param, shouldn't it be hardcoded?
There was a problem hiding this comment.
Updated to have it hardcoded
| } | ||
|
|
||
| export async function down(pgm: MigrationBuilder): Promise<void> { | ||
| pgm.sql(`DROP INDEX deployer_address_lower_case;`) |
There was a problem hiding this comment.
If we are maintaining those down functions then we should create the original index here
This PR:
createOrClausemethod where the timestamp type was hardcoded and didn't change based on the filter typeFixes #291