From 23f9a9fe9675eab1d25c1983a08a7c76e0139d52 Mon Sep 17 00:00:00 2001 From: LoneRifle Date: Wed, 2 Jun 2021 15:06:21 +0800 Subject: [PATCH 1/2] test(betas): provide coverage In preparation to porting betas.client.factory to TypeScript, write a suite of unit tests to ensure functionality does not change during the migration. Restructure the AngularJS factory a bit to make code paths easy to reach. - Restructure `betas.client.factory` so that its functions are accessible both via module import and `angular.module().factory()` - Allow the functions to take in other beta feature fields for test purposes, defaulting to `BETA_FEATURES_FIELD` - Provide test coverage via jest, crowbar-ing a mock into the angular global at runtime, to minimise changes to the module under test --- .../forms/services/betas.client.factory.js | 86 +++++++++++-------- .../services/__tests__/BetaService.test.ts | 86 +++++++++++++++++++ 2 files changed, 138 insertions(+), 34 deletions(-) create mode 100644 src/public/services/__tests__/BetaService.test.ts diff --git a/src/public/modules/forms/services/betas.client.factory.js b/src/public/modules/forms/services/betas.client.factory.js index da580bae8e..2cdd7d08e2 100644 --- a/src/public/modules/forms/services/betas.client.factory.js +++ b/src/public/modules/forms/services/betas.client.factory.js @@ -2,49 +2,67 @@ const { get, forEach } = require('lodash') angular.module('forms').factory('Betas', [Betas]) -function Betas() { - const BETA_FEATURES_FIELD = { - // This is an example of how to add fields to this object - // featureName: { - // flag: 'betaFlagName', - // matches: (field) => - // field.fieldType === 'mobile' && - // (field.isVerifiable === true), - // }, - } +const BETA_FEATURES_FIELD = { + // This is an example of how to add fields to this object + // featureName: { + // flag: 'betaFlagName', + // matches: (field) => + // field.fieldType === 'mobile' && + // (field.isVerifiable === true), + // }, +} - const getBetaFeaturesForFields = (formFields) => { - let betaFeatures = new Set() - forEach(formFields, (field) => { - forEach(BETA_FEATURES_FIELD, (feature, name) => { - if (feature.matches(field)) betaFeatures.add(name) - }) +const getBetaFeaturesForFields = (formFields, betaFeaturesField) => { + let betaFeatures = new Set() + forEach(formFields, (field) => { + forEach(betaFeaturesField, (feature, name) => { + if (feature.matches(field)) betaFeatures.add(name) }) - return Array.from(betaFeatures) - } + }) + return Array.from(betaFeatures) +} - const getMissingFieldPermissions = (user, form) => { - const betaFeatures = getBetaFeaturesForFields(form.form_fields) - return betaFeatures.filter((name) => { - const flag = get(BETA_FEATURES_FIELD, [name, 'flag']) - return flag && !get(user, ['betaFlags', flag], false) - }) - } +const getMissingFieldPermissions = ( + user, + form, + betaFeaturesField = BETA_FEATURES_FIELD, +) => { + const betaFeatures = getBetaFeaturesForFields( + form.form_fields, + betaFeaturesField, + ) + return betaFeatures.filter((name) => { + const flag = get(betaFeaturesField, [name, 'flag']) + return flag && !get(user, ['betaFlags', flag], false) + }) +} - const isBetaField = (fieldType) => { - return BETA_FEATURES_FIELD[fieldType] - } +const isBetaField = (fieldType, betaFeaturesField = BETA_FEATURES_FIELD) => { + return betaFeaturesField[fieldType] +} - const userHasAccessToFieldType = (user, fieldType) => { - const flag = get(BETA_FEATURES_FIELD, [fieldType, 'flag']) - return ( - !isBetaField(fieldType) || (flag && get(user, ['betaFlags', flag], false)) - ) - } +const userHasAccessToFieldType = ( + user, + fieldType, + betaFeaturesField = BETA_FEATURES_FIELD, +) => { + const flag = get(betaFeaturesField, [fieldType, 'flag']) + return ( + !isBetaField(fieldType, betaFeaturesField) || + (flag && get(user, ['betaFlags', flag], false)) + ) +} +function Betas() { return { getMissingFieldPermissions, isBetaField, userHasAccessToFieldType, } } + +module.exports = { + getMissingFieldPermissions, + isBetaField, + userHasAccessToFieldType, +} diff --git a/src/public/services/__tests__/BetaService.test.ts b/src/public/services/__tests__/BetaService.test.ts new file mode 100644 index 0000000000..7c07bebe5e --- /dev/null +++ b/src/public/services/__tests__/BetaService.test.ts @@ -0,0 +1,86 @@ +import { IFieldSchema, IFormDocument } from '../../../types' + +window.angular = { + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + module: jest.fn(() => ({ + factory: jest.fn(), + })), +} + +// eslint-disable-next-line import/first +import * as BetaService from '../../modules/forms/services/betas.client.factory' + +describe('BetaService', () => { + const nonBetaFeature = 'nonBetaFeature' + const featureOne = 'featureOne' + const featureTwo = 'featureTwo' + const betaFeaturesField = { + [featureOne]: { + flag: 'betaFlagOne', + matches(field: IFieldSchema) { + return field.title === featureOne + }, + }, + [featureTwo]: { + flag: 'betaFlagTwo', + matches(field: IFieldSchema) { + return field.title === featureTwo + }, + }, + } + const userWithFeatureOne = { + betaFlags: { + [betaFeaturesField[featureOne].flag]: true, + }, + } + describe('isBetaField', () => { + it('returns truthy for defined beta fields', () => { + const result = BetaService.isBetaField(featureOne, betaFeaturesField) + expect(result).toBeTruthy() + }) + it('returns falsy for fields not defined as beta', () => { + const result = BetaService.isBetaField(nonBetaFeature, betaFeaturesField) + expect(result).toBeFalsy() + }) + }) + describe('userHasAccessToFieldType', () => { + it('returns true for features not defined as beta', () => { + const result = BetaService.userHasAccessToFieldType( + userWithFeatureOne, + nonBetaFeature, + betaFeaturesField, + ) + expect(result).toBeTrue() + }) + it('returns true for beta features that the user has access to', () => { + const result = BetaService.userHasAccessToFieldType( + userWithFeatureOne, + featureOne, + betaFeaturesField, + ) + expect(result).toBeTrue() + }) + it('returns false for beta features that the user lacks access to', () => { + const result = BetaService.userHasAccessToFieldType( + userWithFeatureOne, + featureTwo, + betaFeaturesField, + ) + expect(result).toBeFalse() + }) + }) + describe('getBetaFeaturesForFields', () => { + const form = { + form_fields: [{ title: featureTwo }], + } as IFormDocument + it('lists the beta features that the user lacks', () => { + const result = BetaService.getMissingFieldPermissions( + userWithFeatureOne, + form, + betaFeaturesField, + ) + expect(result).toStrictEqual([featureTwo]) + }) + }) +}) From 74a2ec8406f215eecaf787e24edd150962eb627d Mon Sep 17 00:00:00 2001 From: LoneRifle Date: Wed, 2 Jun 2021 16:04:44 +0800 Subject: [PATCH 2/2] refactor(beta): migrate to TypeScript --- src/public/main.js | 1 - .../edit-fields-modal.client.controller.js | 2 - .../list-forms.client.controller.js | 5 +- .../directives/edit-form.client.directive.js | 7 +- .../forms/services/betas.client.factory.js | 68 ----------------- .../examples-card.client.directive.js | 5 +- src/public/services/BetaService.ts | 75 +++++++++++++++++++ .../services/__tests__/BetaService.test.ts | 22 ++---- 8 files changed, 88 insertions(+), 97 deletions(-) delete mode 100644 src/public/modules/forms/services/betas.client.factory.js create mode 100644 src/public/services/BetaService.ts diff --git a/src/public/main.js b/src/public/main.js index 61c622bb0a..e29766efe4 100644 --- a/src/public/main.js +++ b/src/public/main.js @@ -264,7 +264,6 @@ require('./modules/forms/services/spcp-session.client.factory.js') require('./modules/forms/services/submissions.client.factory.js') require('./modules/forms/services/toastr.client.factory.js') require('./modules/forms/services/attachment.client.service.js') -require('./modules/forms/services/betas.client.factory.js') require('./modules/forms/services/captcha.client.service.js') require('./modules/forms/services/mailto.client.factory.js') diff --git a/src/public/modules/forms/admin/controllers/edit-fields-modal.client.controller.js b/src/public/modules/forms/admin/controllers/edit-fields-modal.client.controller.js index baea64ef3d..f576c7d167 100644 --- a/src/public/modules/forms/admin/controllers/edit-fields-modal.client.controller.js +++ b/src/public/modules/forms/admin/controllers/edit-fields-modal.client.controller.js @@ -28,7 +28,6 @@ angular 'Attachment', 'FormFields', '$q', - 'Betas', 'Auth', '$state', 'Toastr', @@ -43,7 +42,6 @@ function EditFieldsModalController( Attachment, FormFields, $q, - Betas, Auth, $state, Toastr, diff --git a/src/public/modules/forms/admin/controllers/list-forms.client.controller.js b/src/public/modules/forms/admin/controllers/list-forms.client.controller.js index 250e790582..5ad9d65bac 100644 --- a/src/public/modules/forms/admin/controllers/list-forms.client.controller.js +++ b/src/public/modules/forms/admin/controllers/list-forms.client.controller.js @@ -1,6 +1,7 @@ 'use strict' const get = require('lodash/get') +const BetaService = require('../../../../services/BetaService') // Forms controller angular @@ -15,7 +16,6 @@ angular '$timeout', '$window', 'Toastr', - 'Betas', ListFormsController, ]) @@ -29,7 +29,6 @@ function ListFormsController( $timeout, $window, Toastr, - Betas, ) { const vm = this @@ -172,7 +171,7 @@ function ListFormsController( } vm.duplicateForm = function (formIndex) { - const missingBetaPermissions = Betas.getMissingFieldPermissions( + const missingBetaPermissions = BetaService.getMissingFieldPermissions( vm.user, vm.myforms[formIndex], ) diff --git a/src/public/modules/forms/admin/directives/edit-form.client.directive.js b/src/public/modules/forms/admin/directives/edit-form.client.directive.js index 4ec0fd20ee..c7cf72af69 100644 --- a/src/public/modules/forms/admin/directives/edit-form.client.directive.js +++ b/src/public/modules/forms/admin/directives/edit-form.client.directive.js @@ -3,6 +3,7 @@ const { groupLogicUnitsByField } = require('shared/util/logic') const { reorder } = require('shared/util/immutable-array-fns') const FieldFactory = require('../../helpers/field-factory') const { UPDATE_FORM_TYPES } = require('../constants/update-form-types') +const BetaService = require('../../../../services/BetaService') const newFields = new Set() // Adding a fieldTypes will add a "new" label. @@ -35,7 +36,6 @@ function editFormDirective() { 'FormFields', '$uibModal', 'responseModeEnum', - 'Betas', '$window', 'Attachment', editFormController, @@ -48,7 +48,6 @@ function editFormController( FormFields, $uibModal, responseModeEnum, - Betas, $window, Attachment, ) { @@ -56,7 +55,7 @@ function editFormController( $scope.responseModeEnum = responseModeEnum $scope.isBetaField = function (fieldType) { - return Betas.isBetaField(fieldType) + return BetaService.isBetaField(fieldType) } $scope.isNewField = function (fieldType) { @@ -65,7 +64,7 @@ function editFormController( $scope.adminHasAccess = function (fieldType) { const user = $scope.myform.admin - return Betas.userHasAccessToFieldType(user, fieldType) + return BetaService.userHasAccessToFieldType(user, fieldType) } $scope.getFieldTitle = FormFields.getFieldTitle diff --git a/src/public/modules/forms/services/betas.client.factory.js b/src/public/modules/forms/services/betas.client.factory.js deleted file mode 100644 index 2cdd7d08e2..0000000000 --- a/src/public/modules/forms/services/betas.client.factory.js +++ /dev/null @@ -1,68 +0,0 @@ -const { get, forEach } = require('lodash') - -angular.module('forms').factory('Betas', [Betas]) - -const BETA_FEATURES_FIELD = { - // This is an example of how to add fields to this object - // featureName: { - // flag: 'betaFlagName', - // matches: (field) => - // field.fieldType === 'mobile' && - // (field.isVerifiable === true), - // }, -} - -const getBetaFeaturesForFields = (formFields, betaFeaturesField) => { - let betaFeatures = new Set() - forEach(formFields, (field) => { - forEach(betaFeaturesField, (feature, name) => { - if (feature.matches(field)) betaFeatures.add(name) - }) - }) - return Array.from(betaFeatures) -} - -const getMissingFieldPermissions = ( - user, - form, - betaFeaturesField = BETA_FEATURES_FIELD, -) => { - const betaFeatures = getBetaFeaturesForFields( - form.form_fields, - betaFeaturesField, - ) - return betaFeatures.filter((name) => { - const flag = get(betaFeaturesField, [name, 'flag']) - return flag && !get(user, ['betaFlags', flag], false) - }) -} - -const isBetaField = (fieldType, betaFeaturesField = BETA_FEATURES_FIELD) => { - return betaFeaturesField[fieldType] -} - -const userHasAccessToFieldType = ( - user, - fieldType, - betaFeaturesField = BETA_FEATURES_FIELD, -) => { - const flag = get(betaFeaturesField, [fieldType, 'flag']) - return ( - !isBetaField(fieldType, betaFeaturesField) || - (flag && get(user, ['betaFlags', flag], false)) - ) -} - -function Betas() { - return { - getMissingFieldPermissions, - isBetaField, - userHasAccessToFieldType, - } -} - -module.exports = { - getMissingFieldPermissions, - isBetaField, - userHasAccessToFieldType, -} diff --git a/src/public/modules/users/controllers/examples-card.client.directive.js b/src/public/modules/users/controllers/examples-card.client.directive.js index 7b57c7b823..2331969a89 100644 --- a/src/public/modules/users/controllers/examples-card.client.directive.js +++ b/src/public/modules/users/controllers/examples-card.client.directive.js @@ -1,4 +1,5 @@ 'use strict' +const BetaService = require('../../../services/BetaService') angular.module('users').directive('examplesCard', [examplesCard]) @@ -21,7 +22,6 @@ function examplesCard() { 'GTag', 'Auth', '$location', - 'Betas', 'Toastr', examplesCardController, ], @@ -37,7 +37,6 @@ function examplesCardController( GTag, Auth, $location, - Betas, Toastr, ) { $scope.user = Auth.getUser() @@ -79,7 +78,7 @@ function examplesCardController( */ $scope.useTemplate = function () { - const missingBetaPermissions = Betas.getMissingFieldPermissions( + const missingBetaPermissions = BetaService.getMissingFieldPermissions( $scope.user, $scope.form, ) diff --git a/src/public/services/BetaService.ts b/src/public/services/BetaService.ts new file mode 100644 index 0000000000..b7cdf24928 --- /dev/null +++ b/src/public/services/BetaService.ts @@ -0,0 +1,75 @@ +import { get } from 'lodash' + +// Re-use the backend types for now so that we have some type safety. +// Given that this is used mostly by JavaScript modules, the lack of +// mongo-specific types should not present a problem. +// Change the types to frontend equivalents as and when available. +import { IField, IForm, IUser } from '../../types' + +type BetaFeature = { + flag: string + matches: (field: IField) => boolean +} + +type BetaFeaturesDictionary = { [featureName: string]: BetaFeature } + +const BETA_FEATURES_FIELD: BetaFeaturesDictionary = { + // This is an example of how to add fields to this object + // featureName: { + // flag: 'betaFlagName', + // matches: (field) => + // field.fieldType === 'mobile' && + // (field.isVerifiable === true), + // }, +} + +const getBetaFeaturesForFields = ( + formFields: IField[] | undefined, + betaFeaturesField: BetaFeaturesDictionary, +): string[] => { + const betaFeatures = new Set() + if (formFields) { + for (const field of formFields) { + for (const [name, feature] of Object.entries(betaFeaturesField)) { + if (feature.matches(field)) { + betaFeatures.add(name) + } + } + } + } + return Array.from(betaFeatures) +} + +export const getMissingFieldPermissions = ( + user: IUser, + form: IForm, + betaFeaturesField = BETA_FEATURES_FIELD, +): string[] => { + const betaFeatures = getBetaFeaturesForFields( + form.form_fields, + betaFeaturesField, + ) + return betaFeatures.filter((name) => { + const flag = get(betaFeaturesField, [name, 'flag']) + return flag && !get(user, ['betaFlags', flag], false) + }) +} + +export const isBetaField = ( + fieldType: string, + betaFeaturesField = BETA_FEATURES_FIELD, +): BetaFeature | undefined => { + return betaFeaturesField[fieldType] +} + +export const userHasAccessToFieldType = ( + user: IUser, + fieldType: string, + betaFeaturesField = BETA_FEATURES_FIELD, +): boolean => { + const flag = get(betaFeaturesField, [fieldType, 'flag']) + return ( + !isBetaField(fieldType, betaFeaturesField) || + (Boolean(flag) && get(user, ['betaFlags', flag], false)) + ) +} diff --git a/src/public/services/__tests__/BetaService.test.ts b/src/public/services/__tests__/BetaService.test.ts index 7c07bebe5e..e404f1d1e0 100644 --- a/src/public/services/__tests__/BetaService.test.ts +++ b/src/public/services/__tests__/BetaService.test.ts @@ -1,15 +1,5 @@ -import { IFieldSchema, IFormDocument } from '../../../types' - -window.angular = { - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - module: jest.fn(() => ({ - factory: jest.fn(), - })), -} - -// eslint-disable-next-line import/first -import * as BetaService from '../../modules/forms/services/betas.client.factory' +import { IField, IForm, IUser } from '../../../types' +import * as BetaService from '../BetaService' describe('BetaService', () => { const nonBetaFeature = 'nonBetaFeature' @@ -18,13 +8,13 @@ describe('BetaService', () => { const betaFeaturesField = { [featureOne]: { flag: 'betaFlagOne', - matches(field: IFieldSchema) { + matches(field: IField) { return field.title === featureOne }, }, [featureTwo]: { flag: 'betaFlagTwo', - matches(field: IFieldSchema) { + matches(field: IField) { return field.title === featureTwo }, }, @@ -33,7 +23,7 @@ describe('BetaService', () => { betaFlags: { [betaFeaturesField[featureOne].flag]: true, }, - } + } as IUser describe('isBetaField', () => { it('returns truthy for defined beta fields', () => { const result = BetaService.isBetaField(featureOne, betaFeaturesField) @@ -73,7 +63,7 @@ describe('BetaService', () => { describe('getBetaFeaturesForFields', () => { const form = { form_fields: [{ title: featureTwo }], - } as IFormDocument + } as IForm it('lists the beta features that the user lacks', () => { const result = BetaService.getMissingFieldPermissions( userWithFeatureOne,