Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PP-13312 Settings for card types get controller #4376

Merged
merged 7 commits into from
Dec 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,7 +1,20 @@
const { response } = require('@utils/response')
const { formatCardTypesForTemplate } = require('@utils/simplified-account/format/format-card-types')
const { getAllCardTypes, getAcceptedCardTypesForServiceAndAccountType } = require('@services/card-types.service')

function get (req, res) {
response(req, res, 'simplified-account/settings/card-types/index', { })
async function get (req, res, next) {
const serviceId = req.service.externalId
const accountType = req.account.type
const isAdminUser = req.user.isAdminUserForService(serviceId)
try {
const { card_types: allCards } = await getAllCardTypes()
const { card_types: acceptedCards } = await getAcceptedCardTypesForServiceAndAccountType(serviceId, accountType)
const cardTypes = formatCardTypesForTemplate(allCards, acceptedCards, req.account, isAdminUser)
response(req, res, 'simplified-account/settings/card-types/index',
{ cardTypes, isAdminUser })
} catch (err) {
next(err)
}
}

module.exports = {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
const sinon = require('sinon')
const ControllerTestBuilder = require('@test/test-helpers/simplified-account/controllers/ControllerTestBuilder.class')
const { expect } = require('chai')
const User = require('@models/User.class')
const userFixtures = require('@test/fixtures/user.fixtures')

const ACCOUNT_TYPE = 'live'
const SERVICE_ID = 'service-id-123abc'

const adminUser = new User(userFixtures.validUserResponse({
external_id: 'user-id-for-admin-user',
service_roles: {
service: {
service: { external_id: SERVICE_ID },
role: { name: 'admin' }
}
}
}))

const viewOnlyUser = new User(userFixtures.validUserResponse(
{
external_id: 'user-id-for-view-only-user',
service_roles: {
service:
{
service: { external_id: SERVICE_ID },
role: { name: 'view-only' }
}
}
}))

const allCardTypes = [{
id: 'id-001',
brand: 'visa',
label: 'Visa',
type: 'DEBIT',
requires3ds: false
},
{
id: 'id-002',
brand: 'visa',
label: 'Visa',
type: 'CREDIT',
requires3ds: false
}]
const acceptedCardTypes = [allCardTypes[0]]

const mockResponse = sinon.spy()
const mockGetAllCardTypes = sinon.stub().resolves({ card_types: allCardTypes })
const mockGetAcceptedCardTypesForServiceAndAccountType = sinon.stub().resolves({ card_types: acceptedCardTypes })

const { res, nextRequest, call } = new ControllerTestBuilder('@controllers/simplified-account/settings/card-types/card-types.controller')
.withServiceExternalId(SERVICE_ID)
.withAccountType(ACCOUNT_TYPE)
.withStubs({
'@utils/response': { response: mockResponse },
'@services/card-types.service': {
getAllCardTypes: mockGetAllCardTypes,
getAcceptedCardTypesForServiceAndAccountType: mockGetAcceptedCardTypesForServiceAndAccountType
}
})
.build()

describe('Controller: settings/card-types', () => {
describe('get for admin user', () => {
before(() => {
nextRequest({
user: adminUser
})
call('get')
})

it('should call the response method', () => {
expect(mockResponse.called).to.be.true // eslint-disable-line
})

it('should pass req, res and template path to the response method', () => {
expect(mockResponse.args[0][0].user).to.deep.equal(adminUser)
expect(mockResponse.args[0][1]).to.deep.equal(res)
expect(mockResponse.args[0][2]).to.equal('simplified-account/settings/card-types/index')
})

it('should pass context data to the response method', () => {
expect(mockResponse.args[0][3]).to.have.property('cardTypes').to.have.property('debitCards').length(1)
expect(mockResponse.args[0][3].cardTypes.debitCards[0]).to.deep.include({ text: 'Visa debit', checked: true })
expect(mockResponse.args[0][3]).to.have.property('cardTypes').to.have.property('creditCards').length(1)
expect(mockResponse.args[0][3].cardTypes.creditCards[0]).to.deep.include({ text: 'Visa credit', checked: false })
expect(mockResponse.args[0][3]).to.have.property('isAdminUser').to.equal(true)
})
})

describe('get for non-admin user', () => {
before(() => {
nextRequest({
user: viewOnlyUser
})
call('get')
})

it('should call the response method', () => {
expect(mockResponse.called).to.be.true // eslint-disable-line
})

it('should pass req, res and template path to the response method', () => {
expect(mockResponse.args[0][0].user).to.deep.equal(viewOnlyUser)
expect(mockResponse.args[0][1]).to.deep.equal(res)
expect(mockResponse.args[0][2]).to.equal('simplified-account/settings/card-types/index')
})

it('should pass context data to the response method', () => {
expect(mockResponse.args[0][3]).to.have.property('cardTypes').to.have.property('Enabled debit cards').to.have.length(1).to.include('Visa debit')
expect(mockResponse.args[0][3].cardTypes).to.have.property('Not enabled debit cards').to.have.length(0)
expect(mockResponse.args[0][3].cardTypes).to.have.property('Enabled credit cards').to.have.length(0)
expect(mockResponse.args[0][3].cardTypes).to.have.property('Not enabled credit cards').to.have.length(1).to.include('Visa credit')
expect(mockResponse.args[0][3]).to.have.property('isAdminUser').to.equal(false)
})
})
})
17 changes: 17 additions & 0 deletions app/services/card-types.service.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
'use strict'

const ConnectorClient = require('./clients/connector.client.js').ConnectorClient
const connectorClient = new ConnectorClient(process.env.CONNECTOR_URL)

async function getAllCardTypes () {
return connectorClient.getAllCardTypes()
}

async function getAcceptedCardTypesForServiceAndAccountType (serviceId, accountType) {
return connectorClient.getAcceptedCardsForServiceAndAccountType(serviceId, accountType)
}

module.exports = {
getAllCardTypes,
getAcceptedCardTypesForServiceAndAccountType
}
15 changes: 15 additions & 0 deletions app/services/clients/connector.client.js
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,21 @@ ConnectorClient.prototype = {
return response.data
},

/**
* Retrieves the accepted card Types for the given external service external id and account type
* @param serviceId (required)
* @param accountType (required)
* @returns {Promise<Object>}
*/
getAcceptedCardsForServiceAndAccountType: async function (serviceId, accountType) {
const url = `${this.connectorUrl}/v1/frontend/service/{serviceId}/account/{accountType}/card-types`
.replace('{serviceId}', encodeURIComponent(serviceId))
.replace('{accountType}', encodeURIComponent(accountType))
configureClient(client, url)
const response = await client.get(url, 'get accepted card types for account')
return response.data
},

/**
* Updates the accepted card Types for to the given gateway account
* @param gatewayAccountId (required)
Expand Down
3 changes: 3 additions & 0 deletions app/simplified-account-routes.js
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,9 @@ simplifiedAccount.get(paths.simplifiedAccount.settings.worldpayDetails.index, pe
simplifiedAccount.get(paths.simplifiedAccount.settings.worldpayDetails.credentials, permission('gateway-credentials:update'), serviceSettingsController.worldpayDetails.worldpayCredentials.get)
simplifiedAccount.post(paths.simplifiedAccount.settings.worldpayDetails.credentials, permission('gateway-credentials:update'), serviceSettingsController.worldpayDetails.worldpayCredentials.post)

// card types
simplifiedAccount.get(paths.simplifiedAccount.settings.cardTypes.index, permission('transactions:read'), serviceSettingsController.cardTypes.get)

// stripe details
const stripeDetailsPath = paths.simplifiedAccount.settings.stripeDetails
const stripeDetailsRouter = new Router({ mergeParams: true })
Expand Down
80 changes: 80 additions & 0 deletions app/utils/simplified-account/format/format-card-types.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
const cardsThatNeedToBeEnabledOnWorldpay = ['American Express', 'Union Pay']

const formatLabel = (card) => {
if (card.brand === 'visa' || card.brand === 'master-card') {
return `${card.label} ${card.type.toLowerCase()}`
}
return card.brand === 'jcb' ? card.label.toUpperCase() : card.label
}

const createCardTypeChecklistItem = (card, acceptedCards) => {
return {
value: card.id,
text: formatLabel(card),
checked: acceptedCards.filter(accepted => accepted.id === card.id).length !== 0,
requires3ds: card.requires3ds
}
}

const disableCheckboxIf3dsRequiredButNotEnabled = (cardTypeChecklistItem, accountType, accountRequires3ds) => {
if (cardTypeChecklistItem.requires3ds && !accountRequires3ds) {
return {
...cardTypeChecklistItem,
disabled: true,
hint: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are these hints shown in the UI? For example, when the mouse hovers over the label?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are always displayed as additional text underneath the card type if the conditions are met. The text under 'Maestro' in the first screenshot is an example of this.

html: accountType === 'test' ? `${cardTypeChecklistItem.text} is not available on test accounts` : `${cardTypeChecklistItem.text} cannot be used because 3D Secure is not available. Please contact support`
}
}
}
return cardTypeChecklistItem
}

const addHintForAmexAndUnionpayIfWorldpay = (cardTypeChecklistItem, paymentProvider) => {
if (cardsThatNeedToBeEnabledOnWorldpay.includes(cardTypeChecklistItem.text) && paymentProvider === 'worldpay') {
return {
...cardTypeChecklistItem,
hint: {
html: 'You must have already enabled this with Worldpay'
}
}
}
return cardTypeChecklistItem
}

const formatCardTypesForAdminTemplate = (allCards, acceptedCards, account) => {
const debitCardChecklistItems = allCards.filter(card => card.type === 'DEBIT')
.map(card => createCardTypeChecklistItem(card, acceptedCards))
.map(cardTypeChecklistItem => disableCheckboxIf3dsRequiredButNotEnabled(cardTypeChecklistItem, account.type, account.requires3ds))

const creditCardChecklistItems = allCards.filter(card => card.type === 'CREDIT')
.map(card => createCardTypeChecklistItem(card, acceptedCards))
.map(cardTypeChecklistItem => addHintForAmexAndUnionpayIfWorldpay(cardTypeChecklistItem, account.paymentProvider))

return { debitCards: debitCardChecklistItems, creditCards: creditCardChecklistItems }
}

const formatCardTypesForNonAdminTemplate = (allCards, acceptedCards) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the best I could come up with for refactoring this is something like this, but I'm not convinced it's an improvement so feel free to ignore it and stick with this implementation

Suggested change
const formatCardTypesForNonAdminTemplate = (allCards, acceptedCards) => {
const isDisabled = (card, account) => {
return card.requires3ds && !account.requires3ds
}
const hintText = (card, account) => {
const cardLabel = formatLabel(card)
const disabled = isDisabled(card, account)
if (disabled && account.type === 'test') {
return {
text: `${cardLabel} is not available on test accounts`
}
} else if (disabled) {
return {
text: `${cardLabel} cannot be used because 3D Secure is not available. Please contact support`
}
}
if (card.type !== 'CREDIT' || account.paymentProvider !== 'worldpay') {
return undefined
}
if (['American Express', 'Union Pay'].includes(cardLabel)) {
return {
html: 'You must have already enabled this with Worldpay'
}
}
return undefined
}
const formatCard = (card, accepted, account) => {
const disabled = isDisabled(card, account)
return {
value: card.id,
text: formatLabel(card),
checked: !disabled && accepted,
requires3ds: card.requires3ds,
disabled,
hint: hintText(card, account)
}
}
const formatCardTypesForNonAdminTemplate = (allCards, acceptedCards) => {
const items = allCards.reduce((acc, card) => {
if (!acc[card.type]) {
acc[card.type] = []
}
const accepted = acceptedCards.filter(c => c.type === card.type && c.id === card.id).length > 0
acc[card.type].push(formatCard(card, accepted, account))
return acc
}, {})
return {
debitCards: Object.values(items.DEBIT),
creditCards: Object.values(items.CREDIT)
}
}

const acceptedCardTypeIds = acceptedCards.map(card => card.id)
const formattedCardTypes = {
'Enabled debit cards': [],
'Not enabled debit cards': [],
'Enabled credit cards': [],
'Not enabled credit cards': []
}
allCards.forEach(card => {
const cardIsEnabled = acceptedCardTypeIds.includes(card.id) ? 'Enabled' : 'Not enabled'
formattedCardTypes[`${cardIsEnabled} ${card.type.toLowerCase()} cards`].push(formatLabel(card))
})
return formattedCardTypes
}
Comment on lines +58 to +69
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a slight change I might suggest would be something like

Suggested change
const formattedCardTypes = {
'Enabled debit cards': [],
'Not enabled debit cards': [],
'Enabled credit cards': [],
'Not enabled credit cards': []
}
allCards.forEach(card => {
const cardIsEnabled = acceptedCardTypeIds.includes(card.id) ? 'Enabled' : 'Not enabled'
formattedCardTypes[`${cardIsEnabled} ${card.type.toLowerCase()} cards`].push(formatLabel(card))
})
return formattedCardTypes
}
const formattedCardTypes = {
'debit/enabled': {
cards: [],
heading: 'Enabled debit cards'
}
...
}
allCards.forEach(card => {
const cardIsEnabled = acceptedCardTypeIds.includes(card.id) ? 'enabled' : 'disabled'
formattedCardTypes[`${card.type.toLowerCase()}/${cardIsEnabled}`].push(formatLabel(card))
})

just so the heading isn't the object key


const formatCardTypesForTemplate = (allCards, acceptedCards, account, isAdminUser) => {
if (isAdminUser) {
return formatCardTypesForAdminTemplate(allCards, acceptedCards, account)
}
return formatCardTypesForNonAdminTemplate(allCards, acceptedCards)
}

module.exports = {
formatCardTypesForTemplate
}
116 changes: 116 additions & 0 deletions app/utils/simplified-account/format/format-card-types.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
const { expect } = require('chai')
const { formatCardTypesForTemplate } = require('@utils/simplified-account/format/format-card-types')

const allCards = [
{
id: 'id-001',
brand: 'visa',
label: 'Visa',
type: 'DEBIT',
requires3ds: false
},
{
id: 'id-002',
brand: 'visa',
label: 'Visa',
type: 'CREDIT',
requires3ds: false
},
{
id: 'id-003',
brand: 'master-card',
label: 'Mastercard',
type: 'DEBIT',
requires3ds: false
},
{
id: 'id-004',
brand: 'american-express',
label: 'American Express',
type: 'CREDIT',
requires3ds: false
},
{
id: 'id-005',
brand: 'jcb',
label: 'Jcb',
type: 'CREDIT',
requires3ds: false
},
{
id: 'id-006',
brand: 'maestro',
label: 'Maestro',
type: 'DEBIT',
requires3ds: true
}
]
describe('format-card-types for template', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing for these hints in this test would be good too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point - this is now done.

describe('present checkboxes for admin user', () => {
it('should return all card types with checked boxes if they are all accepted', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can improve these tests, something like:

const expectedDebitCards = ['Visa debit', 'Mastercard debit', 'Maestro']
const expectedCreditCards = ['Visa credit', 'American Express', 'JCB']

expect(cards.debitCards).to.have.length(expectedDebitCards.length)
expect(cards.creditCards).to.have.length(expectedCreditCards.length)

cards.debitCards.forEach((card, idx) => {
  expect(card).to.deep.include({
    text: expectedDebitCards[idx],
    checked: true
  })
})

cards.creditCards.forEach((card, idx) => {
  expect(card).to.deep.include({
    text: expectedCreditCards[idx],
    checked: true
  })
})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree that using to.deep.include reduces repetition so I've updated this test and the next one. I'm not sure that using iterable brings a significant benefit with this small number of items and as far as I can see it isn't something that can be reused elsewhere in these tests. Happy to go with it though if you prefer.

const acceptedCards = [...allCards]
const account = { requires3ds: true }
const cards = formatCardTypesForTemplate(allCards, acceptedCards, account, true)
expect(cards).to.have.property('debitCards').to.have.length(3)
expect(cards.debitCards[0]).to.deep.include({ text: 'Visa debit', checked: true })
expect(cards.debitCards[1]).to.deep.include({ text: 'Mastercard debit', checked: true })
expect(cards.debitCards[2]).to.deep.include({ text: 'Maestro', checked: true })
expect(cards).to.have.property('creditCards').to.have.length(3)
expect(cards.creditCards[0]).to.deep.include({ text: 'Visa credit', checked: true })
expect(cards.creditCards[1]).to.deep.include({ text: 'American Express', checked: true })
expect(cards.creditCards[2]).to.deep.include({ text: 'JCB', checked: true })
})

it('should return unchecked boxes for not accepted card types', () => {
const acceptedCards = [...allCards].filter(card => card.id !== 'id-001' && card.id !== 'id-002')
const account = { requires3ds: true }
const cards = formatCardTypesForTemplate(allCards, acceptedCards, account, true)
expect(cards).to.have.property('debitCards').to.have.length(3)
expect(cards.debitCards[0]).to.deep.include({ text: 'Visa debit', checked: false })
expect(cards.debitCards[1]).to.deep.include({ text: 'Mastercard debit', checked: true })
expect(cards.debitCards[2]).to.deep.include({ text: 'Maestro', checked: true })
expect(cards).to.have.property('creditCards').to.have.length(3)
expect(cards.creditCards[0]).to.deep.include({ text: 'Visa credit', checked: false })
expect(cards.creditCards[1]).to.deep.include({ text: 'American Express', checked: true })
expect(cards.creditCards[2]).to.deep.include({ text: 'JCB', checked: true })
})

it('should set checkbox to disabled for requires3ds card types if 3ds not enabled for test account', () => {
const acceptedCards = [...allCards]
const account = { requires3ds: false, type: 'test' }
const cards = formatCardTypesForTemplate(allCards, acceptedCards, account, true)
expect(cards.debitCards.filter(card => card.text === 'Maestro')[0]).to.have.property('disabled').to.be.true // eslint-disable-line no-unused-expressions
expect(cards.debitCards.filter(card => card.text === 'Maestro')[0]).to.have.property('hint')
.to.deep.equal({ html: 'Maestro is not available on test accounts' })
})

it('should set checkbox to disabled for requires3ds card types if 3ds not enabled for live account', () => {
const acceptedCards = [...allCards]
const account = { requires3ds: false, type: 'live' }
const cards = formatCardTypesForTemplate(allCards, acceptedCards, account, true)
expect(cards.debitCards.filter(card => card.text === 'Maestro')[0]).to.have.property('disabled').to.be.true // eslint-disable-line no-unused-expressions
expect(cards.debitCards.filter(card => card.text === 'Maestro')[0]).to.have.property('hint')
.to.deep.equal({ html: 'Maestro cannot be used because 3D Secure is not available. Please contact support' })
})

it('should add hint to American Express if payment provider is Worldpay and account is live', () => {
const acceptedCards = [...allCards]
const account = { requires3ds: true, paymentProvider: 'worldpay', type: 'live' }
const cards = formatCardTypesForTemplate(allCards, acceptedCards, account, true)
expect(cards.creditCards.filter(card => card.text === 'American Express')[0])
.to.have.property('hint').to.deep.equal({ html: 'You must have already enabled this with Worldpay' })
})
})

describe('present read-only list for non-admin user', () => {
it('should return all card types arranged by type and whether accepted or not', () => {
const acceptedCards = [...allCards].filter(card => card.id !== 'id-001' && card.id !== 'id-002')
const account = { requires3ds: true }
const cards = formatCardTypesForTemplate(allCards, acceptedCards, account, false)
expect(cards['Enabled debit cards']).to.have.length(2).to.deep.equal(['Mastercard debit', 'Maestro'])
expect(cards['Not enabled debit cards']).to.have.length(1).to.deep.equal(['Visa debit'])
expect(cards['Enabled credit cards']).to.have.length(2).to.deep.equal(['American Express', 'JCB'])
expect(cards['Not enabled credit cards']).to.have.length(1).to.deep.equal(['Visa credit'])
})
})
})
Loading
Loading