Remove PupilFeedback model dependency from the controller (#335)

* Remove PupilFeedback model dependency from the controller

* updating test as its looking for the wrong pupil

admin/controllers/pupil-feedback.js

@@ -1,7 +1,9 @@
-const PupilFeedback = require('../models/pupil-feedback')
+'use strict'
+
 const { verify } = require('../services/jwt.service')
 const checkDataService = require('../services/data-access/check.data.service')
 const apiResponse = require('./api-response')
+const pupilFeedbackDataService = require('../services/data-access/pupil-feedback.data.service')
 
 // TODO: add logging for all error paths
 
@@ -33,15 +35,15 @@ const setPupilFeedback = async (req, res, next) => {
     return apiResponse.serverError(res)
   }
 
-  const pupilFeedback = new PupilFeedback({
+  const pupilFeedbackData = {
     inputType: inputType,
     satisfactionRating: satisfactionRating,
     comments: comments,
     checkId: check._id
-  })
+  }
 
   try {
-    await pupilFeedback.save()
+    await pupilFeedbackDataService.create(pupilFeedbackData)
     apiResponse.sendJson(res, 'Pupil feedback saved', 201)
   } catch (error) {
     apiResponse.serverError(res)

admin/services/data-access/pupil-feedback.data.service.js

@@ -0,0 +1,18 @@
+'use strict'
+
+const PupilFeedback = require('../../models/pupil-feedback')
+const pupilFeedbackDataService = {
+
+  /**
+   * CREATE a new pupil-feedback record
+   * @param data
+   * @return {Promise<*>}
+   */
+  create: async (data) => {
+    const pf = new PupilFeedback(data)
+    await pf.save()
+    return pf.toObject()
+  }
+}
+
+module.exports = pupilFeedbackDataService

admin/spec/routes/pupil-feedback.spec.js → admin/spec/controllers/pupil-feedback.spec.js

@@ -6,30 +6,24 @@ const sinon = require('sinon')
 const jwtService = require('../../services/jwt.service')
 const { feedbackData } = require('../mocks/pupil-feedback')
 const checkDataService = require('../../services/data-access/check.data.service')
+const pupilFeedbackDataService = require('../../services/data-access/pupil-feedback.data.service')
 const checkMock = require('../mocks/check')
 
+
 require('sinon-mongoose')
 let sandbox
 let jwtPromiseHelper
 let mockCheckData
 let PupilFeedback
-let isSuccessful
 let findOneByCheckCodePromiseHelper
+let pupilFeedbackCreateStub
 
-describe('Pupil Feedback controller', () => {
-  const getResult = (isSuccessful) => {
-    if (isSuccessful) {
-      return isSuccessful
-    } else {
-      throw new Error('saving error')
-    }
-  }
+const resolve = () => Promise.resolve(feedbackData)
+const reject = () => Promise.reject(new Error('pupil feedback mock error'))
 
+describe('Pupil Feedback controller', () => {
   beforeEach(() => {
     sandbox = sinon.sandbox.create()
-    PupilFeedback = class PupilFeedbacks {
-      save () { return getResult(isSuccessful) }
-    }
 
     const jwtPromise = new Promise((resolve, reject) => {
       jwtPromiseHelper = {
@@ -45,13 +39,15 @@ describe('Pupil Feedback controller', () => {
       }
     })
 
-    mockCheckData = async() => {
+    mockCheckData = async (options) => {
       sandbox.stub(jwtService, 'verify').returns(jwtPromise)
       sandbox.stub(checkDataService, 'findOneByCheckCode').returns(findOneByCheckCodePromise)
+      pupilFeedbackCreateStub = sandbox.stub(pupilFeedbackDataService, 'create')
+        .callsFake((options && options.create) || resolve)
       const { setPupilFeedback } = proxyquire('../../controllers/pupil-feedback', {
         '../services/jwt-service': jwtService,
-        '../models/pupil-feedback': PupilFeedback,
-        '../services/data-access/check.data.service': checkDataService
+        '../services/data-access/check.data.service': checkDataService,
+        '../services/data-access/pupil-feedback.data.service': pupilFeedbackDataService
       })
       return setPupilFeedback
     }
@@ -61,7 +57,6 @@ describe('Pupil Feedback controller', () => {
 
   describe('when accessing token validation', () => {
     beforeEach(() => {
-      isSuccessful = true
       jwtPromiseHelper.resolve(true)
       findOneByCheckCodePromiseHelper.resolve(checkMock)
     })
@@ -95,6 +90,19 @@ describe('Pupil Feedback controller', () => {
       expect(data).toBe('Pupil feedback saved')
       done()
     })
+
+    it('saves the data', async(done) => {
+      const req = httpMocks.createRequest({
+        method: 'POST',
+        url: '/api/pupil-feedback',
+        body: feedbackData
+      })
+      const res = httpMocks.createResponse()
+      const setPupilFeedback = await mockCheckData()
+      await setPupilFeedback(req, res)
+      expect(pupilFeedbackCreateStub.callCount).toBe(1)
+      done()
+    })
   })
 
   describe('when access token validation fails', () => {
@@ -120,7 +128,6 @@ describe('Pupil Feedback controller', () => {
 
   describe('when setPupilFeedback fails', () => {
     beforeEach(() => {
-      isSuccessful = false
       jwtPromiseHelper.resolve(true)
       findOneByCheckCodePromiseHelper.resolve(checkMock)
     })
@@ -132,7 +139,7 @@ describe('Pupil Feedback controller', () => {
         body: feedbackData
       })
       const res = httpMocks.createResponse()
-      const postCheck = await mockCheckData()
+      const postCheck = await mockCheckData({create: reject})
       await postCheck(req, res)
       const data = JSON.parse(res._getData())
       expect(res.statusCode).toBe(500)
@@ -144,7 +151,6 @@ describe('Pupil Feedback controller', () => {
   describe('checkDataService error path', () => {
     let req, res
     beforeEach(() => {
-      isSuccessful = true
       jwtPromiseHelper.resolve(true)
       req = httpMocks.createRequest({
         method: 'POST',
@@ -153,6 +159,7 @@ describe('Pupil Feedback controller', () => {
       })
       res = httpMocks.createResponse()
     })
+
     it('returns a bad request when the checkCode is not found', async (done) => {
       findOneByCheckCodePromiseHelper.resolve(null)
       const setPupilFeedback = await mockCheckData()
@@ -163,7 +170,7 @@ describe('Pupil Feedback controller', () => {
       done()
     })
 
-    it('returns server error if the checkCodeDatasService throws an error', async (done) => {
+    it('returns server error if the checkCodeDataService throws an error', async (done) => {
       findOneByCheckCodePromiseHelper.reject(new Error('mock'))
       const setPupilFeedback = await mockCheckData()
       await setPupilFeedback(req, res)

admin/spec/service/data-access/pupil-feedback.data.service.spec.js

@@ -0,0 +1,36 @@
+'use strict'
+/* global describe, beforeEach, afterEach, it, expect */
+
+const proxyquire = require('proxyquire').noCallThru()
+const sinon = require('sinon')
+require('sinon-mongoose')
+
+const PupilFeedback = require('../../../models/pupil-feedback')
+// const pupilFeedback = require('../../mocks/pupil-feedback')
+
+describe('check.data.service', () => {
+  let service, sandbox
+
+  beforeEach(() => {
+    sandbox = sinon.sandbox.create()
+  })
+
+  afterEach(() => sandbox.restore())
+
+  describe('#create', () => {
+    let mock
+
+    beforeEach(() => {
+      mock = sandbox.mock(PupilFeedback.prototype).expects('save')
+      service = proxyquire('../../../services/data-access/pupil-feedback.data.service', {
+        '../../models/pupil-feedback': PupilFeedback
+      })
+    })
+
+    it('makes the expected calls', async (done) => {
+      await service.create({test: 'property'})
+      expect(mock.verify()).toBe(true)
+      done()
+    })
+  })
+})

admin/test/features/step_definitions/restarts_steps.rb

@@ -61,6 +61,7 @@
 
 When(/^I select a pupil for restarts$/) do
   pupil = restarts_page.pupil_list.rows.find {|row| row.has_no_selected?}
+  @pupil_name = pupil.name.text
   pupil.checkbox.click
 end
 
@@ -129,7 +130,7 @@
 
 Then(/^I should see pupil is added to the pupil restarts list with status '(.*)'$/) do|restart_status|
   hightlighted_row = restarts_page.restarts_pupil_list.rows.find{|row| row.has_highlighted_pupil?}
-  expect(hightlighted_row.text).to include("#{@details_hash[:last_name]}, #{@details_hash[:first_name]}")
+  expect(hightlighted_row.text).to include("#{@pupil_name}")
   expect(hightlighted_row.status.text).to include(restart_status)
 end