Skip to content

Commit

Permalink
Stop adding link on due returns for unauth users (#1378)
Browse files Browse the repository at this point in the history
https://eaflood.atlassian.net/browse/WATER-4700

We have built a replacement view licence page, which includes displaying return logs to all users.

All users should be able to see return logs. However, when we reviewed the legacy code, we overlooked that only permitted users should see a link on 'due/overdue' returns that allows them to edit them.

The issue was reported on behalf of users with basic access. When they clicked the link, they saw an error. We checked what the legacy page does, and it simply doesn't display a link.

So, this change updates our version to replicate this behaviour.
  • Loading branch information
Cruikshanks authored Oct 3, 2024
1 parent 0ed0731 commit 1c286ba
Show file tree
Hide file tree
Showing 5 changed files with 92 additions and 29 deletions.
18 changes: 12 additions & 6 deletions app/presenters/licences/view-licence-returns.presenter.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,13 @@ const { formatLongDate } = require('../base.presenter.js')
*
* @param {module:ReturnLogModel[]} returnLogs - The results from `FetchLicenceReturnsService` to be formatted
* @param {boolean} hasRequirements - True if the licence has return requirements else false
* @param {object} auth - The auth object taken from `request.auth` containing user details
*
* @returns {object} The data formatted for the view template
*/
function go (returnLogs, hasRequirements) {
const returns = _returns(returnLogs)
function go (returnLogs, hasRequirements, auth) {
const canManageReturns = auth.credentials.scope.includes('returns')
const returns = _returns(returnLogs, canManageReturns)

const hasReturns = returns.length > 0

Expand All @@ -26,12 +28,16 @@ function go (returnLogs, hasRequirements) {
}
}

function _link (status, returnLogId) {
function _link (status, returnLogId, canManageReturns) {
if (['completed', 'void'].includes(status)) {
return `/returns/return?id=${returnLogId}`
}

return `/return/internal?returnId=${returnLogId}`
if (canManageReturns) {
return `/return/internal?returnId=${returnLogId}`
}

return null
}

function _noReturnsMessage (hasReturns, hasRequirements) {
Expand All @@ -52,15 +58,15 @@ function _purpose (purpose) {
return firstPurpose.alias ? firstPurpose.alias : firstPurpose.tertiary.description
}

function _returns (returns) {
function _returns (returns, canManageReturns) {
return returns.map((returnLog) => {
const { endDate, dueDate, id: returnLogId, metadata, returnReference, startDate, status } = returnLog

return {
dates: `${formatLongDate(new Date(startDate))} to ${formatLongDate(new Date(endDate))}`,
description: metadata.description,
dueDate: formatLongDate(new Date(dueDate)),
link: _link(status, returnLogId),
link: _link(status, returnLogId, canManageReturns),
purpose: _purpose(metadata.purposes),
reference: returnReference,
returnLogId,
Expand Down
2 changes: 1 addition & 1 deletion app/services/licences/view-licence-returns.service.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ async function go (licenceId, auth, page) {
const hasRequirements = await DetermineLicenceHasReturnVersionsService.go(licenceId)

const returnsData = await FetchLicenceReturnsService.go(licenceId, page)
const pageData = ViewLicenceReturnsPresenter.go(returnsData.returns, hasRequirements)
const pageData = ViewLicenceReturnsPresenter.go(returnsData.returns, hasRequirements, auth)

const pagination = PaginatorPresenter.go(returnsData.pagination.total, Number(page), `/system/licences/${licenceId}/returns`)

Expand Down
7 changes: 6 additions & 1 deletion app/views/licences/tabs/returns.njk
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,12 @@
{% from "macros/return-status-tag.njk" import statusTag %}

{% macro referenceColumn(return) %}
<a href="{{ return.link }}" class="govuk-link">{{ return.reference }} </a>
{% if return.link %}
<a href="{{ return.link }}" class="govuk-link">{{ return.reference }} </a>
{% else %}
<div>{{ return.reference }}</div>
{% endif %}

<p class="govuk-body-s">{{ return.dates }}</p>
{% endmacro %}

Expand Down
80 changes: 60 additions & 20 deletions test/presenters/licences/view-licence-returns.presenter.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,29 @@ const { expect } = Code
const ViewLicenceReturnsPresenter = require('../../../app/presenters/licences/view-licence-returns.presenter.js')

describe('View Licence returns presenter', () => {
let auth
let returnLogs
let hasRequirements

beforeEach(() => {
auth = {
isValid: true,
credentials: {
user: { id: 123 },
roles: ['returns'],
groups: [],
scope: ['returns'],
permissions: { abstractionReform: false, billRuns: true, manage: true }
}
}

hasRequirements = true
returnLogs = _returnLogs()
})

describe('when provided with returns data', () => {
it('correctly presents the data', () => {
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements)
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements, auth)

expect(result).to.equal({
noReturnsMessage: null,
Expand Down Expand Up @@ -52,7 +64,7 @@ describe('View Licence returns presenter', () => {

describe('the "dates" property', () => {
it('returns the start and end date in long format (2 January 2020 to 1 February 2020)', () => {
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements)
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements, auth)

expect(result.returns[0].dates).to.equal('2 January 2020 to 1 February 2020')
})
Expand All @@ -61,17 +73,31 @@ describe('View Licence returns presenter', () => {
describe('the "link" property', () => {
describe('when the return log has a status of "completed"', () => {
it('returns a link to the view return log page', () => {
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements)
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements, auth)

expect(result.returns[0].link).to.equal('/returns/return?id=v1:1:01/123:10046821:2020-01-02:2020-02-01')
})
})

describe('when the return log has a status of "due"', () => {
it('returns a link to the edit return log page', () => {
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements)
describe('and the user is permitted to submit and edit returns', () => {
it('returns a link to the edit return log page', () => {
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements, auth)

expect(result.returns[1].link).to.equal('/return/internal?returnId=v1:1:01/123:10046820:2020-01-02:2020-02-01')
})
})

describe('and the user is not permitted to submit and edit returns', () => {
beforeEach(() => {
auth.credentials.scope = []
})

it('returns null', () => {
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements, auth)

expect(result.returns[1].link).to.equal('/return/internal?returnId=v1:1:01/123:10046820:2020-01-02:2020-02-01')
expect(result.returns[1].link).to.be.null()
})
})
})

Expand All @@ -80,10 +106,24 @@ describe('View Licence returns presenter', () => {
returnLogs[1].status = 'received'
})

it('returns a link to the edit return log page', () => {
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements)
describe('and the user is permitted to submit and edit returns', () => {
it('returns a link to the edit return log page', () => {
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements, auth)

expect(result.returns[1].link).to.equal('/return/internal?returnId=v1:1:01/123:10046820:2020-01-02:2020-02-01')
expect(result.returns[1].link).to.equal('/return/internal?returnId=v1:1:01/123:10046820:2020-01-02:2020-02-01')
})
})

describe('and the user is not permitted to submit and edit returns', () => {
beforeEach(() => {
auth.credentials.scope = []
})

it('returns null', () => {
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements, auth)

expect(result.returns[1].link).to.be.null()
})
})
})

Expand All @@ -93,7 +133,7 @@ describe('View Licence returns presenter', () => {
})

it('returns a link to the view return log page', () => {
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements)
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements, auth)

expect(result.returns[1].link).to.equal('/returns/return?id=v1:1:01/123:10046820:2020-01-02:2020-02-01')
})
Expand All @@ -103,7 +143,7 @@ describe('View Licence returns presenter', () => {
describe('the "purpose" property', () => {
describe("when the first purpose in the return log's metadata does not have an alias", () => {
it("returns the purpose's tertiary description", () => {
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements)
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements, auth)

expect(result.returns[1].purpose).to.equal('SPRAY IRRIGATION')
})
Expand All @@ -115,7 +155,7 @@ describe('View Licence returns presenter', () => {
})

it("returns the purpose's alias", () => {
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements)
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements, auth)

expect(result.returns[1].purpose).to.equal('Spray irrigation - top field only')
})
Expand All @@ -125,7 +165,7 @@ describe('View Licence returns presenter', () => {
describe('the "status" property', () => {
describe('when the return log has a status of "completed"', () => {
it('returns "complete"', () => {
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements)
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements, auth)

expect(result.returns[0].status).to.equal('complete')
})
Expand All @@ -134,7 +174,7 @@ describe('View Licence returns presenter', () => {
describe('when the return log has a status of "due"', () => {
describe('and the due date is less than today', () => {
it('returns "overdue"', () => {
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements)
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements, auth)

expect(result.returns[1].status).to.equal('overdue')
})
Expand All @@ -147,7 +187,7 @@ describe('View Licence returns presenter', () => {
})

it('returns "due"', () => {
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements)
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements, auth)

expect(result.returns[1].status).to.equal('due')
})
Expand All @@ -160,7 +200,7 @@ describe('View Licence returns presenter', () => {
})

it('returns "received"', () => {
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements)
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements, auth)

expect(result.returns[1].status).to.equal('received')
})
Expand All @@ -172,7 +212,7 @@ describe('View Licence returns presenter', () => {
})

it('returns "void"', () => {
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements)
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements, auth)

expect(result.returns[1].status).to.equal('void')
})
Expand All @@ -183,7 +223,7 @@ describe('View Licence returns presenter', () => {
describe('the "noReturnsMessage" property', () => {
describe('when a licence has returns and requirements', () => {
it('returns null', () => {
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements)
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements, auth)

expect(result.noReturnsMessage).to.be.null()
})
Expand All @@ -196,7 +236,7 @@ describe('View Licence returns presenter', () => {
})

it('returns the message "No requirements for returns have been set up for this licence."', () => {
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements)
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements, auth)

expect(result.noReturnsMessage).to.equal('No requirements for returns have been set up for this licence.')
})
Expand All @@ -208,7 +248,7 @@ describe('View Licence returns presenter', () => {
})

it('returns the message "No returns for this licence."', () => {
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements)
const result = ViewLicenceReturnsPresenter.go(returnLogs, hasRequirements, auth)

expect(result.noReturnsMessage).to.equal('No returns for this licence.')
})
Expand Down
14 changes: 13 additions & 1 deletion test/services/licences/view-licence-returns.service.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,21 @@ const ViewLicenceReturnsService = require('../../../app/services/licences/view-l
describe('View Licence Returns service', () => {
const testId = '2c80bd22-a005-4cf4-a2a2-73812a9861de'
const page = 1
const auth = {}

let auth

beforeEach(async () => {
auth = {
isValid: true,
credentials: {
user: { id: 123 },
roles: ['returns'],
groups: [],
scope: ['returns'],
permissions: { abstractionReform: false, billRuns: true, manage: true }
}
}

Sinon.stub(DetermineLicenceHasReturnVersionsService, 'go').returns(true)

Sinon.stub(FetchLicenceReturnsService, 'go').resolves({
Expand Down

0 comments on commit 1c286ba

Please sign in to comment.