Easy renewal query #2080
Easy renewal query #2080
Conversation
https://eaflood.atlassian.net/browse/IWTF-3155 Implement amendments to the Easy Renewal query to improve performance
There was a problem hiding this comment.
I've put a few comments inline, but also there's a lot of use of let rather than const. I've highlighted a couple, but there shouldn't be any - I appreciate in general you're modifying existing code, but this is a clear improvement that would be fairly easy to make. Thanks!
| licenseePostcode | ||
| )}'` | ||
| filter += ` and ${licensee.property}/${licensee.entity.definition.mappings.birthDate.field} eq ${licenseeBirthDate}` | ||
| let filter = `${Permission.definition.mappings.referenceNumber.field} eq '${escapeODataStringValue(permissionReferenceNumber)}'` |
There was a problem hiding this comment.
I think this can be changed to a const now, with line nine incorporated into the expression
| let filter = `${Permission.definition.mappings.referenceNumber.field} eq '${escapeODataStringValue(permissionReferenceNumber)}'` | ||
| const formattedContactIds = contactIds.map(id => `defra_ContactId/contactid eq '${id}'`).join(' or ') | ||
|
|
||
| let filter = `(${formattedContactIds})` |
There was a problem hiding this comment.
again, modify to a const now
| export const contactForLicenseeNoReference = (licenseeBirthDate, licenseePostcode) => { | ||
| let filter = `${Contact.definition.mappings.postcode.field} eq '${escapeODataStringValue(licenseePostcode)}'` | ||
| filter += ` and ${Contact.definition.mappings.birthDate.field} eq ${licenseeBirthDate}` | ||
| filter += ` and ${Contact.definition.defaultFilter}` |
There was a problem hiding this comment.
Use const, not let, please
| filter += ` and ${Contact.definition.defaultFilter}` | ||
| return new PredefinedQuery({ | ||
| root: Contact, | ||
| filter: filter, |
There was a problem hiding this comment.
use the shorthand rather than repeating
| expand: [], | ||
| filter: "defra_postcode eq 'AB12 3CD' and birthdate eq 03/12/1990 and statecode eq 0", | ||
| select: expect.any(Array) | ||
| }) |
There was a problem hiding this comment.
Rather than this (I know you've copied other tests, but I think they're incorrect too - we can begin an improvement here), test that contactForLicenseeNoReference is of type PredefinedQuery, and that the three properties that are set (root, filter and expand), are set to the expected value. Also, please use several cases with different birth dates and postcodes.
The function also relies on Contact.definition.mappings and Contact.definition.defaultFilter, so mock these and test that the mocked values are used. At the moment, no tests would fail if contactForLicenseeNoReference hardcoded 'defra_postcode' rather than using Contact.definition.mappings.postcode.field.
| expect(query.toRetrieveRequest()).toEqual({ | ||
| collection: 'defra_permissions', | ||
| expand: expect.arrayContaining([ | ||
| expect.objectContaining({ property: 'defra_ContactId' }), | ||
| expect.objectContaining({ property: 'defra_PermitId' }), | ||
| expect.objectContaining({ property: 'defra_defra_permission_defra_concessionproof_PermissionId' }) | ||
| ]), | ||
| filter: | ||
| "endswith(defra_name, 'ABC123') and defra_ContactId/defra_postcode eq 'AB12 3CD' and defra_ContactId/birthdate eq 2000-01-01 and statecode eq 0", | ||
| filter: "defra_name eq 'ABC123' and statecode eq 0", |
There was a problem hiding this comment.
Ah, I see where the test format above comes from. Please could you improve the checking of the other values by doing expect.any(Contact) rather than expect.objectContaining({ property: 'defra_ContactId' }), as well as the other values (expect.any(Permit), expect.any(ConcessionProof)).
There was a problem hiding this comment.
in predefined query we strip the entity out so test is a Contact fails but can test the property returned is defer_ContactId
let me know if above makes sense or we can have quick chat
| expect(query.toRetrieveRequest()).toEqual({ | ||
| collection: 'defra_permissions', | ||
| expand: expect.arrayContaining([ | ||
| expect.objectContaining({ property: 'defra_ContactId' }), | ||
| expect.objectContaining({ property: 'defra_PermitId' }), | ||
| expect.objectContaining({ property: 'defra_defra_permission_defra_concessionproof_PermissionId' }) | ||
| ]), | ||
| filter: "defra_name eq 'ABC123' and statecode eq 0", | ||
| filter: "(defra_ContactId/contactid eq '12345') and statecode eq 0", |
There was a problem hiding this comment.
Static values like these need multiple cases to avoid naive implementations.
Also, please could you improve the checking of the other values by doing expect.any(Contact) rather than expect.objectContaining({ property: 'defra_ContactId' }), as well as the other values.
| @@ -5,11 +5,10 @@ import { permitSchema } from './permit.schema.js' | |||
| import { contactResponseSchema } from './contact.schema.js' | |||
| import { finalisedPermissionSchemaContent } from './permission.schema.js' | |||
|
|
|||
| const referenceLength = 6 | |||
There was a problem hiding this comment.
By convention, we use ALL_CAPS_WITH_UNDERSCORES for constant values like this, so change to REFERENCE_LENGTH, please
|
https://eaflood.atlassian.net/browse/IWTF-3155
Implement amendments to the Easy Renewal query to improve performance