Merge pull request #1136 from ministryofjustice/CAS-1225-fe-configure…

…-the-ui-to-be-able-to-support-csv-report-downloads

CAS-1225 Change restClient pipe method from buffering to streaming

integration_tests/mockApis/report.ts

@@ -58,9 +58,11 @@ export default {
       response: {
         status: 200,
         headers: {
-          'Content-Type': 'text/plain',
+          'Content-Type': 'application/octet-stream',
+          'Transfer-Encoding': 'chunked',
         },
-        body: args.data,
+        // Simulate streaming by providing the data in chunks
+        body: `0\r\n\r\n${args.data}\r\n0\r\n\r\n`, // This simulates chunked transfer encoding
       },
     }),
   verifyReportForRegion: async (args: {

integration_tests/tests/temporary-accommodation/manage/report.cy.ts

@@ -1,5 +1,4 @@
 import path from 'path'
-import { Cas3ReportType } from '@approved-premises/api'
 import Page from '../../../../cypress_shared/pages/page'
 import DashboardPage from '../../../../cypress_shared/pages/temporary-accommodation/dashboardPage'
 import ReportIndexPage from '../../../../cypress_shared/pages/temporary-accommodation/manage/reportIndex'
@@ -32,39 +31,6 @@ context('Report', () => {
     Page.verifyOnPage(ReportIndexPage)
   })
 
-  it('does not download a file when the API returns an error', () => {
-    // Given I am signed in
-    cy.signIn()
-
-    // When I visit the report page
-    cy.task('stubReportReferenceData')
-    const page = ReportIndexPage.visit()
-
-    // And I fill out the form
-    const type: Cas3ReportType = 'booking'
-    cy.then(function _() {
-      const probationRegion = this.actingUserProbationRegion
-      const startDate = '12/01/2024'
-      const endDate = '12/03/2024'
-
-      page.completeForm(startDate, endDate)
-
-      cy.task('stubReportError', {
-        data: 'some-data',
-        probationRegionId: probationRegion.id,
-        startDate: DateFormats.datepickerInputToIsoString(startDate),
-        endDate: DateFormats.datepickerInputToIsoString(endDate),
-        type,
-      })
-    })
-
-    page.expectDownload()
-    page.clickDownload(type)
-
-    // Then I should see an error message
-    cy.get('h1').contains('Internal Server Error')
-  })
-
   it("allows me to download a booking report for the acting user's region", () => {
     // Given I am signed in
     cy.signIn()

server/data/reportClient.test.ts

@@ -1,23 +1,57 @@
 import nock from 'nock'
-
 import { createMock } from '@golevelup/ts-jest'
 import { Response } from 'express'
+import { Readable } from 'stream'
+import { Cas3ReportType } from '@approved-premises/api'
 import config from '../config'
-import paths from '../paths/api'
-import { probationRegionFactory } from '../testutils/factories'
 import ReportClient from './reportClient'
 import { CallConfig } from './restClient'
 
+jest.mock('superagent', () => ({
+  get: jest.fn().mockReturnThis(), // Mock the .get method of superagent
+  pipe: jest.fn(), // Mock the .pipe method
+}))
+
 describe('ReportClient', () => {
-  let fakeApprovedPremisesApi: nock.Scope
   let reportClient: ReportClient
+  let mockStream: Readable
+  let filename: string
+  let probationRegionId: string
+  let startDate: string
+  let endDate: string
+  let type: Cas3ReportType
+  let mockResponse: Response
+  let restClientMock: { pipe: jest.Mock }
 
   const callConfig = { token: 'some-token' } as CallConfig
 
   beforeEach(() => {
+    // Mock the restClient instance and its pipe method
+    restClientMock = {
+      pipe: jest.fn(),
+    }
+
+    // Mock the ReportClient's restClient property
     config.apis.approvedPremises.url = 'http://localhost:8080'
-    fakeApprovedPremisesApi = nock(config.apis.approvedPremises.url)
     reportClient = new ReportClient(callConfig)
+    // eslint-disable-next-line  @typescript-eslint/no-explicit-any
+    reportClient.restClient = restClientMock as any
+
+    filename = 'report.csv'
+    probationRegionId = '12345'
+    startDate = '2024-01-01'
+    endDate = '2024-01-31'
+    type = 'referral'
+
+    // Create a mock stream that has an `on` method (simulating a Readable stream)
+    mockStream = new Readable({
+      read() {
+        // We don't need to do anything for this simple mock
+      },
+    })
+
+    mockResponse = createMock<Response>()
+    restClientMock.pipe.mockReturnValue(mockStream)
   })
 
   afterEach(() => {
@@ -29,87 +63,53 @@ describe('ReportClient', () => {
     nock.cleanAll()
   })
 
-  describe('bookings', () => {
-    it('pipes all bookings to an express response', async () => {
-      const data = 'some-data'
-
-      const year = '2023'
-      const month = '3'
-
-      fakeApprovedPremisesApi
-        .get(paths.reports.bookings({}))
-        .matchHeader('authorization', `Bearer ${callConfig.token}`)
-        .query({ year, month })
-        .reply(200, data, { 'content-type': 'some-content-type' })
-
-      const response = createMock<Response>()
-
-      await reportClient.bookings(response, 'some-filename', month, year)
-
-      expect(response.set).toHaveBeenCalledWith({
-        'Content-Type': 'some-content-type',
-        'Content-Disposition': `attachment; filename="some-filename"`,
-      })
-      expect(response.send).toHaveBeenCalledWith(Buffer.alloc(data.length, data))
-    })
-  })
-
-  describe('bookings', () => {
-    it('pipes all bookings to an express response', async () => {
-      const data = 'some-data'
-
-      const year = '2024'
-      const month = '0'
-
-      fakeApprovedPremisesApi
-        .get(paths.reports.bookings({}))
-        .matchHeader('authorization', `Bearer ${callConfig.token}`)
-        .query({ year, month })
-        .reply(200, data, { 'content-type': 'some-content-type' })
-
-      const response = createMock<Response>()
-
-      await reportClient.bookings(response, 'some-filename', month, year)
-
-      expect(response.set).toHaveBeenCalledWith({
-        'Content-Type': 'some-content-type',
-        'Content-Disposition': `attachment; filename="some-filename"`,
-      })
-      expect(response.send).toHaveBeenCalledWith(Buffer.alloc(data.length, data))
+  describe('reportForProbationRegion', () => {
+    it('should call restClient.pipe with correct arguments', async () => {
+      await reportClient.reportForProbationRegion(mockResponse, filename, probationRegionId, startDate, endDate, type)
+
+      expect(restClientMock.pipe).toHaveBeenCalledWith(
+        mockResponse,
+        expect.objectContaining({
+          path: expect.any(String),
+          query: {
+            probationRegionId,
+            startDate,
+            endDate,
+          },
+          filename,
+        }),
+      )
     })
-  })
 
-  describe('reportForProbationRegion', () => {
-    it('pipes data for a probation region to an express response', async () => {
-      const probationRegion = probationRegionFactory.build()
-
-      const data = 'some-data'
-      const startDate = '2024-01-01'
-      const endDate = '2024-04-01'
-      const type = 'bedOccupancy'
-
-      fakeApprovedPremisesApi
-        .get(paths.reports.bedspaceUtilisation({}))
-        .matchHeader('authorization', `Bearer ${callConfig.token}`)
-        .query({ probationRegionId: probationRegion.id, startDate, endDate })
-        .reply(200, data, { 'content-type': 'some-content-type' })
-
-      const response = createMock<Response>()
-
-      await reportClient.reportForProbationRegion(
-        response,
-        'some-filename',
-        probationRegion.id,
+    it('should handle the stream properly when restClient.pipe returns a stream', async () => {
+      const result = await reportClient.reportForProbationRegion(
+        mockResponse,
+        filename,
+        probationRegionId,
         startDate,
         endDate,
         type,
       )
 
-      expect(response.set).toHaveBeenCalledWith({
-        'Content-Type': 'some-content-type',
-        'Content-Disposition': `attachment; filename="some-filename"`,
+      expect(result).toBeUndefined()
+      expect(restClientMock.pipe).toHaveReturnedWith(mockStream)
+
+      expect(mockStream.on).toBeDefined()
+
+      const eventCallback = jest.fn()
+      mockStream.on('data', eventCallback)
+      expect(eventCallback).not.toHaveBeenCalled()
+    })
+
+    it('should throw an error if restClient.pipe fails', async () => {
+      const error = new Error('Failed to pipe')
+      restClientMock.pipe.mockImplementationOnce(() => {
+        throw error
       })
-      expect(response.send).toHaveBeenCalledWith(Buffer.alloc(data.length, data))
+
+      await expect(
+        reportClient.reportForProbationRegion(mockResponse, filename, probationRegionId, startDate, endDate, type),
+      ).rejects.toThrow('Failed to pipe')
     })
   })
 })

server/data/reportClient.ts

@@ -1,7 +1,6 @@
 import { Response } from 'express'
 import { Cas3ReportType } from '@approved-premises/api'
 import config, { ApiConfig } from '../config'
-import paths from '../paths/api'
 import RestClient, { CallConfig } from './restClient'
 import { getApiReportPath } from '../utils/reportUtils'
 
@@ -12,14 +11,6 @@ export default class ReportClient {
     this.restClient = new RestClient('personClient', config.apis.approvedPremises as ApiConfig, callConfig)
   }
 
-  async bookings(response: Response, filename: string, month: string, year: string): Promise<void> {
-    await this.restClient.pipe(response, {
-      path: paths.reports.bookings({}),
-      query: { year, month },
-      filename,
-    })
-  }
-
   async reportForProbationRegion(
     response: Response,
     filename: string,

server/data/restClient.test.ts

@@ -1,10 +1,13 @@
 import { createMock } from '@golevelup/ts-jest'
 import { Response } from 'express'
 import nock from 'nock'
+import { Readable } from 'stream'
 import type { ApiConfig } from '../config'
 import { probationRegionFactory } from '../testutils/factories'
 import RestClient, { CallConfig } from './restClient'
-import * as sanitiseError from '../sanitisedError'
+import logger from '../../logger'
+
+jest.mock('../../logger')
 
 describe('restClient', () => {
   let fakeApprovedPremisesApi: nock.Scope
@@ -147,63 +150,53 @@ describe('restClient', () => {
   })
 
   describe('pipe', () => {
-    it('should make a GET request and pipe the response, using the specified filename', async () => {
-      const data = 'some-data'
+    const response = createMock<Response>({})
+    const mockReadStream = jest.fn().mockImplementation(() => {
+      const readable = new Readable()
+      readable.push('hello')
+      readable.push('world')
+      readable.push(null)
+
+      return readable
+    })
 
-      fakeApprovedPremisesApi.get(`/some/path`).reply(200, data, { 'content-type': 'some-content-type' })
+    it('should pipe a streaming response to a response', async () => {
+      fakeApprovedPremisesApi.get('/some/path').reply(200, () => mockReadStream())
 
-      const response = createMock<Response>()
+      const writeSpy = jest.spyOn(response, 'write')
 
-      await restClient.pipe(response, { path: '/some/path', filename: 'some-filename' })
+      await restClient.pipe(response, { path: '/some/path' })
 
-      expect(response.set).toHaveBeenCalledWith({
-        'Content-Type': 'some-content-type',
-        'Content-Disposition': 'attachment; filename="some-filename"',
-      })
+      expect(writeSpy).toHaveBeenCalledWith(Buffer.from('hello', 'utf-8'))
+      expect(writeSpy).toHaveBeenCalledWith(Buffer.from('world', 'utf-8'))
 
-      expect(response.send).toHaveBeenCalledWith(Buffer.from(data))
       expect(nock.isDone()).toBeTruthy()
     })
 
-    it('should make a GET request and pipe the response, using the filename from the API when none is specified', async () => {
-      const data = 'some-data'
-      fakeApprovedPremisesApi.get(`/some/path`).reply(200, data, {
-        'content-type': 'some-content-type',
-        'content-disposition': 'attachment; filename="some-filename"',
-      })
-
-      const response = createMock<Response>()
+    it('should send the content-disposition header to the response', async () => {
+      fakeApprovedPremisesApi
+        .get('/some/path')
+        .reply(200, () => mockReadStream(), { 'content-disposition': 'attachment; filename=foo.txt' })
 
-      await restClient.pipe(response, { path: '/some/path' })
+      const setSpy = jest.spyOn(response, 'set')
 
-      expect(response.set).toHaveBeenCalledWith({
-        'Content-Type': 'some-content-type',
-        'Content-Disposition': 'attachment; filename="some-filename"',
-      })
+      await restClient.pipe(response, { path: '/some/path', passThroughHeaders: ['content-disposition'] })
 
-      expect(response.send).toHaveBeenCalledWith(Buffer.from(data))
-      expect(nock.isDone()).toBeTruthy()
+      expect(setSpy).toHaveBeenCalledWith('content-disposition', 'attachment; filename=foo.txt')
     })
 
-    it('should throw with a sanitised error when the API returns an error', async () => {
-      jest.spyOn(sanitiseError, 'default')
-      fakeApprovedPremisesApi.get(`/some/path`).reply(400, { some: 'data' })
+    it('should throw error if the response is unsuccessful', async () => {
+      fakeApprovedPremisesApi.get('/some/path').reply(404)
 
-      const response = createMock<Response>()
-      let error: Error
+      const loggerSpy = jest.spyOn(logger, 'warn')
 
-      try {
-        await restClient.pipe(response, { path: '/some/path' })
-      } catch (e) {
-        error = e
-      }
+      await expect(restClient.pipe(response, { path: '/some/path' })).rejects.toThrowError(
+        'cannot GET /some/path (404)',
+      )
 
-      expect(sanitiseError.default).toHaveBeenCalledWith(new Error(error.message))
+      expect(loggerSpy).toHaveBeenCalledWith(new Error('cannot GET /some/path (404)'), 'Error calling premisesClient')
 
-      expect(response.set).not.toHaveBeenCalled()
-      expect(response.send).not.toHaveBeenCalled()
-
-      expect(nock.isDone()).toBeTruthy()
+      nock.cleanAll()
     })
   })
 })