feat(DTFS2-7052): applying Oscars suggestions on my PR

UK-Export-Finance · Apr 24, 2024 · f68ac66 · f68ac66
1 parent df90b33
commit f68ac66
Show file tree

Hide file tree

Showing 9 changed files with 83 additions and 131 deletions.
diff --git a/src/helper-modules/ordnance-survey/dto/get-addresses-ordnance-survey-response.dto.ts b/src/helper-modules/ordnance-survey/dto/get-addresses-ordnance-survey-response.dto.ts
@@ -1,4 +1,4 @@
-export type GetAddressOrdnanceSurveyResponse = {
+export type GetAddressesOrdnanceSurveyResponse = {
   header: {
     uri: string;
     query: string;
@@ -12,14 +12,14 @@ export type GetAddressOrdnanceSurveyResponse = {
     lastupdate: string;
     output_srs: string;
   };
-  results?: GetAddressOrdnanceSurveyResponseItem[];
+  results?: GetAddressesOrdnanceSurveyResponseItem[];
 };
 
-interface GetAddressOrdnanceSurveyResponseItem {
-  DPA: GetAddressOrdnanceSurveyResponseAddress;
+interface GetAddressesOrdnanceSurveyResponseItem {
+  DPA: GetAddressesOrdnanceSurveyResponseAddress;
 }
 
-interface GetAddressOrdnanceSurveyResponseAddress {
+interface GetAddressesOrdnanceSurveyResponseAddress {
   UPRN: string;
   UDPRN: string;
   ADDRESS: string;

diff --git a/src/helper-modules/ordnance-survey/known-errors.ts b/src/helper-modules/ordnance-survey/known-errors.ts
diff --git a/src/helper-modules/ordnance-survey/ordnance-survey.service.test.ts b/src/helper-modules/ordnance-survey/ordnance-survey.service.test.ts
@@ -4,8 +4,8 @@ import { RandomValueGenerator } from '@ukef-test/support/generator/random-value-
 import { AxiosError } from 'axios';
 import { when } from 'jest-when';
 import { of, throwError } from 'rxjs';
-import expectedResponse = require('./examples/example-response-for-search-places-v1-postcode.json');
-import noResultsResponse = require('./examples/example-response-for-search-places-v1-postcode-no-results.json');
+import expectedResponseData = require('./examples/example-response-for-search-places-v1-postcode.json');
+import noResultsResponseData = require('./examples/example-response-for-search-places-v1-postcode-no-results.json');
 
 import { GEOSPATIAL } from '@ukef/constants';
 
@@ -23,6 +23,14 @@ describe('OrdnanceSurveyService', () => {
   const testKey = valueGenerator.string({ length: 10 });
   const basePath = '/search/places/v1/postcode';
 
+  const expectedResponse = of({
+    data: expectedResponseData,
+    status: 200,
+    statusText: 'OK',
+    config: undefined,
+    headers: undefined,
+  });
+
   beforeEach(() => {
     const httpService = new HttpService();
     const configService = new ConfigService();
@@ -40,26 +48,11 @@ describe('OrdnanceSurveyService', () => {
 
     const expectedHttpServiceGetArgs: [string, object] = [expectedPath, { headers: { 'Content-Type': 'application/json' } }];
 
-    it('sends a GET to the Ordnance Survey API /search endpoint with the specified request', async () => {
-      when(httpServiceGet)
-        .calledWith(...expectedHttpServiceGetArgs)
-        .mockReturnValueOnce(
-          of({
-            data: expectedResponse,
-            status: 200,
-            statusText: 'OK',
-            config: undefined,
-            headers: undefined,
-          }),
-        );
-
-      await service.getAddressesByPostcode(testPostcode);
-
-      expect(httpServiceGet).toHaveBeenCalledTimes(1);
-      expect(httpServiceGet).toHaveBeenCalledWith(...expectedHttpServiceGetArgs);
-    });
-
-    it.each([
+    describe.each([
+      {
+        postcode: testPostcode,
+        expectedUrlQueryPart: `?postcode=${encodeURIComponent(testPostcode)}`,
+      },
       {
         postcode: 'W1A 1AA',
         expectedUrlQueryPart: `?postcode=W1A%201AA`,
@@ -68,34 +61,40 @@ describe('OrdnanceSurveyService', () => {
         postcode: 'W1A1AA',
         expectedUrlQueryPart: '?postcode=W1A1AA',
       },
-    ])('call Ordnance Survey API with correct and safe query parameters "$expectedUrlQueryPart"', async ({ postcode, expectedUrlQueryPart }) => {
-      const expectedPath = `${basePath}${expectedUrlQueryPart}&lr=EN&key=${encodeURIComponent(testKey)}`;
-      const expectedHttpServiceGetArgs: [string, object] = [expectedPath, { headers: { 'Content-Type': 'application/json' } }];
+    ])('test postcode $postcode', ({ postcode, expectedUrlQueryPart }) => {
+      it('call Ordnance Survey with the correct arguments', async () => {
+        const expectedPath = `${basePath}${expectedUrlQueryPart}&lr=EN&key=${encodeURIComponent(testKey)}`;
+        const expectedHttpServiceGetArgs: [string, object] = [expectedPath, { headers: { 'Content-Type': 'application/json' } }];
 
-      when(httpServiceGet)
-        .calledWith(...expectedHttpServiceGetArgs)
-        .mockReturnValueOnce(
-          of({
-            data: expectedResponse,
-            status: 200,
-            statusText: 'OK',
-            config: undefined,
-            headers: undefined,
-          }),
-        );
+        when(httpServiceGet)
+          .calledWith(...expectedHttpServiceGetArgs)
+          .mockReturnValueOnce(expectedResponse);
+        await service.getAddressesByPostcode(postcode);
 
-      await service.getAddressesByPostcode(postcode);
+        expect(httpServiceGet).toHaveBeenCalledTimes(1);
+        expect(httpServiceGet).toHaveBeenCalledWith(...expectedHttpServiceGetArgs);
+      });
 
-      expect(httpServiceGet).toHaveBeenCalledTimes(1);
-      expect(httpServiceGet).toHaveBeenCalledWith(...expectedHttpServiceGetArgs);
+      it('call Ordnance Survey returns expectedResponse', async () => {
+        const expectedPath = `${basePath}${expectedUrlQueryPart}&lr=EN&key=${encodeURIComponent(testKey)}`;
+        const expectedHttpServiceGetArgs: [string, object] = [expectedPath, { headers: { 'Content-Type': 'application/json' } }];
+
+        when(httpServiceGet)
+          .calledWith(...expectedHttpServiceGetArgs)
+          .mockReturnValueOnce(expectedResponse);
+
+        const response = await service.getAddressesByPostcode(postcode);
+
+        expect(response).toBe(expectedResponseData);
+      });
     });
 
-    it('no results - returns 200 without results', async () => {
+    it('returns a 200 response without results when Ordnance Survey returns no results', async () => {
       when(httpServiceGet)
         .calledWith(...expectedHttpServiceGetArgs)
         .mockReturnValueOnce(
           of({
-            data: noResultsResponse,
+            data: noResultsResponseData,
             status: 200,
             statusText: 'OK',
             config: undefined,
@@ -107,7 +106,7 @@ describe('OrdnanceSurveyService', () => {
 
       expect(httpServiceGet).toHaveBeenCalledTimes(1);
       expect(httpServiceGet).toHaveBeenCalledWith(...expectedHttpServiceGetArgs);
-      expect(results).toBe(noResultsResponse);
+      expect(results).toBe(noResultsResponseData);
     });
 
     it('throws an OrdnanceSurveyException if the request to Ordnance Survey fails', async () => {

diff --git a/src/helper-modules/ordnance-survey/ordnance-survey.service.ts b/src/helper-modules/ordnance-survey/ordnance-survey.service.ts
@@ -5,9 +5,8 @@ import { KEY as ORDNANCE_SURVEY_CONFIG_KEY, OrdnanceSurveyConfig } from '@ukef/c
 import { GEOSPATIAL } from '@ukef/constants';
 import { HttpClient } from '@ukef/modules/http/http.client';
 
-import { GetAddressOrdnanceSurveyResponse } from './dto/get-addresses-ordnance-survey-response.dto';
-// import { getCustomersNotFoundKnownOrdnanceSurveyError } from './known-errors';
-import { createWrapOrdnanceSurveyHttpGetErrorCallback } from './wrap-ordnance-survey-http-error-callback';
+import { GetAddressesOrdnanceSurveyResponse } from './dto/get-addresses-ordnance-survey-response.dto';
+import { OrdnanceSurveyException } from './exception/ordnance-survey.exception';
 
 @Injectable()
 export class OrdnanceSurveyService {
@@ -20,16 +19,15 @@ export class OrdnanceSurveyService {
     this.key = key;
   }
 
-  async getAddressesByPostcode(postcode): Promise<GetAddressOrdnanceSurveyResponse> {
+  async getAddressesByPostcode(postcode: string): Promise<GetAddressesOrdnanceSurveyResponse> {
     const path = `/search/places/v1/postcode?postcode=${encodeURIComponent(postcode)}&lr=${GEOSPATIAL.DEFAULT.RESULT_LANGUAGE}&key=${encodeURIComponent(this.key)}`;
-    const { data } = await this.httpClient.get<GetAddressOrdnanceSurveyResponse>({
+    const { data } = await this.httpClient.get<GetAddressesOrdnanceSurveyResponse>({
       path,
       headers: { 'Content-Type': 'application/json' },
-      onError: createWrapOrdnanceSurveyHttpGetErrorCallback({
-        messageForUnknownError: `Failed to get response from Ordnance Survey API.`,
-        knownErrors: [],
-        // knownErrors: [getCustomersNotFoundKnownOrdnanceSurveyError()], // TODO: should we change 200 no results to 404?
-      }),
+      onError: (error: Error) => {
+        console.error('Http call error happened, error %o', error);
+        throw new OrdnanceSurveyException('Failed to get response from Ordnance Survey API.', error);
+      },
     });
     return data;
   }

diff --git a/src/helper-modules/ordnance-survey/wrap-ordnance-survey-http-error-callback.ts b/src/helper-modules/ordnance-survey/wrap-ordnance-survey-http-error-callback.ts
diff --git a/.../dto/get-address-by-postcode-query.dto.ts → ...to/get-addresses-by-postcode-query.dto.ts b/.../dto/get-address-by-postcode-query.dto.ts → ...to/get-addresses-by-postcode-query.dto.ts
@@ -4,7 +4,7 @@ import { Matches, MaxLength, MinLength } from 'class-validator';
 
 const UK_POSTCODE = /^[A-Za-z]{1,2}[\dRr][\dA-Za-z]?\s?\d[ABD-HJLNP-UW-Zabd-hjlnp-uw-z]{2}$/;
 
-export class GetAddressByPostcodeQueryDto {
+export class GetAddressesByPostcodeQueryDto {
   @ApiProperty({
     example: GEOSPATIAL.EXAMPLES.POSTCODE,
     description: 'Postcode to search for',

diff --git a/src/modules/geospatial/geospatial.controller.ts b/src/modules/geospatial/geospatial.controller.ts
@@ -1,7 +1,7 @@
 import { Controller, Get, Query } from '@nestjs/common';
 import { ApiNotFoundResponse, ApiOperation, ApiResponse, ApiTags } from '@nestjs/swagger';
 
-import { GetAddressByPostcodeQueryDto } from './dto/get-address-by-postcode-query.dto';
+import { GetAddressesByPostcodeQueryDto } from './dto/get-addresses-by-postcode-query.dto';
 import { GetAddressesResponse, GetAddressesResponseItem } from './dto/get-addresses-response.dto';
 import { GeospatialService } from './geospatial.service';
 
@@ -23,7 +23,7 @@ export class GeospatialController {
   @ApiNotFoundResponse({
     description: 'Customer not found.',
   })
-  getAddressesByPostcode(@Query() query: GetAddressByPostcodeQueryDto): Promise<GetAddressesResponse> {
+  getAddressesByPostcode(@Query() query: GetAddressesByPostcodeQueryDto): Promise<GetAddressesResponse> {
     return this.geospatialService.getAddressesByPostcode(query.postcode);
   }
 }
diff --git a/src/modules/geospatial/geospatial.service.ts b/src/modules/geospatial/geospatial.service.ts
@@ -1,6 +1,6 @@
 import { Injectable } from '@nestjs/common';
 import { ENUMS } from '@ukef/constants';
-import { GetAddressOrdnanceSurveyResponse } from '@ukef/helper-modules/ordnance-survey/dto/get-addresses-ordnance-survey-response.dto';
+import { GetAddressesOrdnanceSurveyResponse } from '@ukef/helper-modules/ordnance-survey/dto/get-addresses-ordnance-survey-response.dto';
 import { OrdnanceSurveyService } from '@ukef/helper-modules/ordnance-survey/ordnance-survey.service';
 
 import { GetAddressesResponse } from './dto/get-addresses-response.dto';
@@ -11,7 +11,7 @@ export class GeospatialService {
 
   async getAddressesByPostcode(postcode: string): Promise<GetAddressesResponse> {
     const addresses = [];
-    const response: GetAddressOrdnanceSurveyResponse = await this.ordnanceSurveyService.getAddressesByPostcode(postcode);
+    const response: GetAddressesOrdnanceSurveyResponse = await this.ordnanceSurveyService.getAddressesByPostcode(postcode);
 
     if (!response?.results) {
       return [];

diff --git a/test/support/generator/get-geospatial-addresses-generator.ts b/test/support/generator/get-geospatial-addresses-generator.ts
@@ -1,7 +1,7 @@
 import { ENUMS, GEOSPATIAL } from '@ukef/constants';
-import { GetAddressOrdnanceSurveyResponse } from '@ukef/helper-modules/ordnance-survey/dto/get-addresses-ordnance-survey-response.dto';
+import { GetAddressesOrdnanceSurveyResponse } from '@ukef/helper-modules/ordnance-survey/dto/get-addresses-ordnance-survey-response.dto';
 import { OrdnanceSurveyAuthErrorResponse } from '@ukef/helper-modules/ordnance-survey/dto/ordnance-survey-auth-error-response.dto';
-import { GetAddressByPostcodeQueryDto } from '@ukef/modules/geospatial/dto/get-address-by-postcode-query.dto';
+import { GetAddressesByPostcodeQueryDto } from '@ukef/modules/geospatial/dto/get-addresses-by-postcode-query.dto';
 import { GetAddressesResponse } from '@ukef/modules/geospatial/dto/get-addresses-response.dto';
 
 import { AbstractGenerator } from './abstract-generator';
@@ -28,19 +28,19 @@ export class GetGeospatialAddressesGenerator extends AbstractGenerator<AddressVa
   protected transformRawValuesToGeneratedValues(values: AddressValues[], { postcode, key }: GenerateOptions): GenerateResult {
     const useKey = key || 'test';
 
-    const request: GetAddressByPostcodeQueryDto[] = values.map((v) => ({ postcode: postcode || v.POSTCODE }) as GetAddressByPostcodeQueryDto);
+    const requests: GetAddressesByPostcodeQueryDto[] = values.map((v) => ({ postcode: postcode || v.POSTCODE }) as GetAddressesByPostcodeQueryDto);
 
-    const ordnanceSurveyPath: string[] = values.map((v) => {
+    const ordnanceSurveyPaths: string[] = values.map((v) => {
       const usePostcode = postcode || v.POSTCODE;
       return `/search/places/v1/postcode?postcode=${encodeURIComponent(usePostcode)}&lr=${GEOSPATIAL.DEFAULT.RESULT_LANGUAGE}&key=${encodeURIComponent(useKey)}`;
     });
 
-    const mdmPath: string[] = values.map((v) => {
+    const mdmPaths: string[] = values.map((v) => {
       const usePostcode = postcode || v.POSTCODE;
       return `/api/v1/geospatial/addresses/postcode?postcode=${usePostcode}`;
     });
 
-    const getAddressByPostcodeResponse: GetAddressesResponse[] = values.map((v) => [
+    const getAddressesByPostcodeResponse: GetAddressesResponse[] = values.map((v) => [
       {
         organisationName: v.ORGANISATION_NAME,
         addressLine1: `${v.BUILDING_NAME} ${v.BUILDING_NUMBER} ${v.THOROUGHFARE_NAME}`,
@@ -52,9 +52,9 @@ export class GetGeospatialAddressesGenerator extends AbstractGenerator<AddressVa
       },
     ]);
 
-    const getAddressByPostcodeMultipleResponse = getAddressByPostcodeResponse.map((response) => response[0]);
+    const getAddressesByPostcodeMultipleResponse = getAddressesByPostcodeResponse.map((response) => response[0]);
 
-    const getAddressOrdnanceSurveyResponse: GetAddressOrdnanceSurveyResponse[] = values.map((v) => ({
+    const getAddressesOrdnanceSurveyResponse: GetAddressesOrdnanceSurveyResponse[] = values.map((v) => ({
       header: {
         uri: 'test',
         query: 'test',
@@ -109,7 +109,7 @@ export class GetGeospatialAddressesGenerator extends AbstractGenerator<AddressVa
       ],
     }));
 
-    const getAddressOrdnanceSurveyMultipleResponse: GetAddressOrdnanceSurveyResponse = {
+    const getAddressesOrdnanceSurveyMultipleResponse: GetAddressesOrdnanceSurveyResponse = {
       header: {
         uri: 'test',
         query: 'test',
@@ -162,7 +162,7 @@ export class GetGeospatialAddressesGenerator extends AbstractGenerator<AddressVa
       })),
     };
 
-    const getAddressOrdnanceSurveyEmptyResponse: GetAddressOrdnanceSurveyResponse[] = values.map(() => ({
+    const getAddressesOrdnanceSurveyEmptyResponse: GetAddressesOrdnanceSurveyResponse = {
       header: {
         uri: 'test',
         query: 'test',
@@ -176,9 +176,9 @@ export class GetGeospatialAddressesGenerator extends AbstractGenerator<AddressVa
         lastupdate: 'test',
         output_srs: 'test',
       },
-    }));
+    };
 
-    const ordnanceSurveyAuthErrorResponse = {
+    const ordnanceSurveyAuthErrorResponse: OrdnanceSurveyAuthErrorResponse = {
       fault: {
         faultstring: 'Invalid ApiKey',
         detail: {
@@ -188,14 +188,14 @@ export class GetGeospatialAddressesGenerator extends AbstractGenerator<AddressVa
     };
 
     return {
-      request,
-      ordnanceSurveyPath,
-      mdmPath,
-      getAddressByPostcodeResponse,
-      getAddressByPostcodeMultipleResponse,
-      getAddressOrdnanceSurveyResponse,
-      getAddressOrdnanceSurveyEmptyResponse,
-      getAddressOrdnanceSurveyMultipleResponse,
+      requests,
+      ordnanceSurveyPath: ordnanceSurveyPaths,
+      mdmPath: mdmPaths,
+      getAddressByPostcodeResponse: getAddressesByPostcodeResponse,
+      getAddressByPostcodeMultipleResponse: getAddressesByPostcodeMultipleResponse,
+      getAddressOrdnanceSurveyResponse: getAddressesOrdnanceSurveyResponse,
+      getAddressOrdnanceSurveyEmptyResponse: getAddressesOrdnanceSurveyEmptyResponse,
+      getAddressOrdnanceSurveyMultipleResponse: getAddressesOrdnanceSurveyMultipleResponse,
       ordnanceSurveyAuthErrorResponse,
     };
   }
@@ -218,13 +218,13 @@ interface GenerateOptions {
 }
 
 interface GenerateResult {
-  request: GetAddressByPostcodeQueryDto[];
+  requests: GetAddressesByPostcodeQueryDto[];
   ordnanceSurveyPath: string[];
   mdmPath: string[];
   getAddressByPostcodeResponse: GetAddressesResponse[];
   getAddressByPostcodeMultipleResponse: GetAddressesResponse;
-  getAddressOrdnanceSurveyResponse: GetAddressOrdnanceSurveyResponse[];
-  getAddressOrdnanceSurveyMultipleResponse: GetAddressOrdnanceSurveyResponse;
-  getAddressOrdnanceSurveyEmptyResponse: GetAddressOrdnanceSurveyResponse[];
+  getAddressOrdnanceSurveyResponse: GetAddressesOrdnanceSurveyResponse[];
+  getAddressOrdnanceSurveyMultipleResponse: GetAddressesOrdnanceSurveyResponse;
+  getAddressOrdnanceSurveyEmptyResponse: GetAddressesOrdnanceSurveyResponse;
   ordnanceSurveyAuthErrorResponse: OrdnanceSurveyAuthErrorResponse;
 }