From cfb45637921973ce000ce1a1085937e36b83b09e Mon Sep 17 00:00:00 2001 From: wanlingt <56983748+wanlingt@users.noreply.github.com> Date: Tue, 17 Oct 2023 09:36:36 +0800 Subject: [PATCH] feat: add more myinfo sgid fields (#6766) * feat: use fe app URL for local dev redirect * feat: add new myinfo sgid fields new fields: sex, race, nationality, housingtype, hdbtype * feat: add registered address * feat: log if missing myinfo field is from sgid or singpass login * feat: use isDataReadOnly property * feat: add growthbook * feat: add more sgid fields * chore: promote -sgid config to use env site name (cherry picked from commit e9d9288943baf7acbe0bf0735fe9d92464b3f085) * fix: fix linting errors * feat: change formatVehicles input type * fix: set agency shortname as growthbook attribute * fix: catch errors when parsing vehicles * feat: account for blank fields * feat: account for NA in missing field values check * fix: clean up code * fix: remove comment * fix: include SGID_SUPPORTED_V1 in SGID_SUPPORTED_V1 * fix: include more details in comment * feat: add logging to _isDataReadOnly for Singpass and SGID MyInfo * fix: remove console.log statement * feat: update mobile_number_with_country_code scope * fix: return empty string when fieldValue is NA --------- Co-authored-by: Ken --- .ebextensions/env-file-creation.config | 2 +- .../field-panels/MyInfoPanel.tsx | 81 +++++++++---- shared/constants/feature-flags.ts | 1 + src/app/modules/myinfo/myinfo.adapter.ts | 11 ++ src/app/modules/myinfo/myinfo.service.ts | 1 + src/app/modules/myinfo/myinfo.util.ts | 13 +- src/app/modules/sgid/sgid.adapter.ts | 114 ++++++++++++++++-- src/app/modules/sgid/sgid.constants.ts | 15 ++- src/app/modules/sgid/sgid.controller.ts | 11 +- src/app/modules/sgid/sgid.format.ts | 43 +++++++ 10 files changed, 258 insertions(+), 34 deletions(-) create mode 100644 src/app/modules/sgid/sgid.format.ts diff --git a/.ebextensions/env-file-creation.config b/.ebextensions/env-file-creation.config index bf59ee6d6b..2d6b460e14 100644 --- a/.ebextensions/env-file-creation.config +++ b/.ebextensions/env-file-creation.config @@ -44,9 +44,9 @@ files: aws ssm get-parameter --name "${ENV_TYPE}-sentry" --with-decryption --region $AWS_REGION | jq -r '.Parameter.Value' >> $TARGET_DIR/.env aws ssm get-parameter --name "${ENV_TYPE}-sms" --with-decryption --region $AWS_REGION | jq -r '.Parameter.Value' >> $TARGET_DIR/.env aws ssm get-parameter --name "${ENV_TYPE}-ndi" --with-decryption --region $AWS_REGION | jq -r '.Parameter.Value' >> $TARGET_DIR/.env - aws ssm get-parameter --name "${ENV_TYPE}-sgid" --with-decryption --region $AWS_REGION | jq -r '.Parameter.Value' >> $TARGET_DIR/.env aws ssm get-parameter --name "${ENV_TYPE}-verified-fields" --with-decryption --region $AWS_REGION | jq -r '.Parameter.Value' >> $TARGET_DIR/.env aws ssm get-parameter --name "${ENV_TYPE}-webhook-verified-content" --with-decryption --region $AWS_REGION | jq -r '.Parameter.Value' >> $TARGET_DIR/.env + aws ssm get-parameter --name "${ENV_SITE_NAME}-sgid" --with-decryption --region $AWS_REGION | jq -r '.Parameter.Value' >> $TARGET_DIR/.env aws ssm get-parameter --name "${ENV_SITE_NAME}-payment" --with-decryption --region $AWS_REGION | jq -r '.Parameter.Value' >> $TARGET_DIR/.env aws ssm get-parameter --name "${ENV_SITE_NAME}-cron-payment" --with-decryption --region $AWS_REGION | jq -r '.Parameter.Value' >> $TARGET_DIR/.env diff --git a/frontend/src/features/admin-form/create/builder-and-design/BuilderAndDesignDrawer/FieldListDrawer/field-panels/MyInfoPanel.tsx b/frontend/src/features/admin-form/create/builder-and-design/BuilderAndDesignDrawer/FieldListDrawer/field-panels/MyInfoPanel.tsx index abc879eab1..90f2afa81a 100644 --- a/frontend/src/features/admin-form/create/builder-and-design/BuilderAndDesignDrawer/FieldListDrawer/field-panels/MyInfoPanel.tsx +++ b/frontend/src/features/admin-form/create/builder-and-design/BuilderAndDesignDrawer/FieldListDrawer/field-panels/MyInfoPanel.tsx @@ -1,8 +1,10 @@ -import { useCallback, useMemo } from 'react' +import { useCallback, useEffect, useMemo } from 'react' import { Droppable } from 'react-beautiful-dnd' import { Link as ReactLink } from 'react-router-dom' import { Box, Text } from '@chakra-ui/react' +import { useFeatureIsOn, useGrowthBook } from '@growthbook/growthbook-react' +import { featureFlags } from '~shared/constants' import { AdminFormDto, FormAuthType, @@ -35,7 +37,7 @@ import { DraggableMyInfoFieldListOption } from '../FieldListOption' import { FieldSection } from './FieldSection' -const SGID_SUPPORTED: Set = new Set([ +const SGID_SUPPORTED_V1 = [ MyInfoAttribute.Name, MyInfoAttribute.DateOfBirth, MyInfoAttribute.PassportNumber, @@ -44,26 +46,64 @@ const SGID_SUPPORTED: Set = new Set([ // phone number formats. // MyInfo phone numbers support country code, while sgID-MyInfo does not. // MyInfoAttribute.MobileNo, - - // FRM-1189: This is disabled due to slight different formatting. - // We format the Myinfo response by separates lines in addresses with comma - // Whereas sgID separates each line with newline. - // This should be enabled in future work - // MyInfoAttribute.RegisteredAddress, -]) - -/** - * If sgID is used, checks if the corresponding - * MyInfo field is supported by sgID. - */ -const sgIDUnSupported = ( - form: AdminFormDto | undefined, - fieldType: MyInfoAttribute, -): boolean => - form?.authType === FormAuthType.SGID_MyInfo && !SGID_SUPPORTED.has(fieldType) +] +const SGID_SUPPORTED_V2 = [ + ...SGID_SUPPORTED_V1, + MyInfoAttribute.Sex, + MyInfoAttribute.Race, + MyInfoAttribute.Nationality, + MyInfoAttribute.HousingType, + MyInfoAttribute.HdbType, + MyInfoAttribute.RegisteredAddress, + MyInfoAttribute.BirthCountry, + MyInfoAttribute.VehicleNo, + MyInfoAttribute.Employment, + MyInfoAttribute.WorkpassStatus, + MyInfoAttribute.Marital, + MyInfoAttribute.MobileNo, +] export const MyInfoFieldPanel = () => { const { data: form, isLoading } = useCreateTabForm() + + const { user } = useUser() + + // FRM-1444: Remove once rollout is 100% and stable + const growthbook = useGrowthBook() + + useEffect(() => { + if (growthbook) { + growthbook.setAttributes({ + // Only update the `adminEmail` and `adminAgency` attributes, keep the rest the same + ...growthbook.getAttributes(), + adminEmail: user?.email, + adminAgency: user?.agency.shortName, + }) + } + }, [growthbook, user]) + + const showSgidMyInfoV2 = useFeatureIsOn(featureFlags.myinfoSgid) + + const sgidSupportedFinal = useMemo(() => { + return showSgidMyInfoV2 ? SGID_SUPPORTED_V2 : SGID_SUPPORTED_V1 + }, [showSgidMyInfoV2]) + + /** + * If sgID is used, checks if the corresponding + * MyInfo field is supported by sgID. + */ + const sgIDUnSupported = useCallback( + (form: AdminFormDto | undefined, fieldType: MyInfoAttribute): boolean => { + const sgidSupported: Set = new Set(sgidSupportedFinal) + + return ( + form?.authType === FormAuthType.SGID_MyInfo && + !sgidSupported.has(fieldType) + ) + }, + [sgidSupportedFinal], + ) + // myInfo should be disabled if // 1. form response mode is not email mode // 2. form auth type is not myInfo @@ -83,9 +123,8 @@ export const MyInfoFieldPanel = () => { (fieldType: MyInfoAttribute): boolean => { return isDisabled || sgIDUnSupported(form, fieldType) }, - [form, isDisabled], + [form, isDisabled, sgIDUnSupported], ) - const { user } = useUser() return ( <> diff --git a/shared/constants/feature-flags.ts b/shared/constants/feature-flags.ts index f1c57d7bbc..de9158b6a1 100644 --- a/shared/constants/feature-flags.ts +++ b/shared/constants/feature-flags.ts @@ -8,4 +8,5 @@ export const featureFlags = { 'encryption-boundary-shift-hard-validation' as const, encryptionBoundaryShiftVirusScanner: 'encryption-boundary-shift-virus-scanner' as const, + myinfoSgid: 'myinfo-sgid' as const, } diff --git a/src/app/modules/myinfo/myinfo.adapter.ts b/src/app/modules/myinfo/myinfo.adapter.ts index 18e569f303..27d12b298b 100644 --- a/src/app/modules/myinfo/myinfo.adapter.ts +++ b/src/app/modules/myinfo/myinfo.adapter.ts @@ -14,6 +14,7 @@ import { MyInfoChildVaxxStatus, MyInfoDataTransformer, } from '../../../../shared/types' +import { createLoggerWithLabel } from '../../config/logger' import { formatAddress, @@ -26,6 +27,8 @@ import { } from './myinfo.format' import { isMyInfoChildrenBirthRecords } from './myinfo.util' +const logger = createLoggerWithLabel(module) + /** * Converts an internal MyInfo attribute used in FormSG to a scope * which can be used to retrieve data from MyInfo. @@ -422,6 +425,14 @@ export class MyInfoData // Above cases should be exhaustive for all attributes supported by Form. // Fall back to leaving field editable as data shape is unknown. default: + logger.error({ + message: 'Unknown attribute found in Singpass MyInfo field', + meta: { + action: '_isDataReadOnly', + myInfoValue, + attr, + }, + }) return false } } diff --git a/src/app/modules/myinfo/myinfo.service.ts b/src/app/modules/myinfo/myinfo.service.ts index 3a8f0f8047..61a3217768 100644 --- a/src/app/modules/myinfo/myinfo.service.ts +++ b/src/app/modules/myinfo/myinfo.service.ts @@ -281,6 +281,7 @@ export class MyInfoServiceClass { fieldValue, myInfoAttr, myInfoConstantsList, + myInfoData, ) } } diff --git a/src/app/modules/myinfo/myinfo.util.ts b/src/app/modules/myinfo/myinfo.util.ts index 533d18a26b..c29b22f78f 100644 --- a/src/app/modules/myinfo/myinfo.util.ts +++ b/src/app/modules/myinfo/myinfo.util.ts @@ -44,12 +44,14 @@ import { FormAuthNoEsrvcIdError, FormNotFoundError, } from '../form/form.errors' +import { SGIDMyInfoData } from '../sgid/sgid.adapter' import { SGID_MYINFO_LOGIN_COOKIE_NAME } from '../sgid/sgid.constants' import { ProcessedChildrenResponse, ProcessedFieldResponse, } from '../submission/submission.types' +import { MyInfoData } from './myinfo.adapter' import { MYINFO_LOGIN_COOKIE_NAME } from './myinfo.constants' import { MyInfoCookieStateError, @@ -542,7 +544,7 @@ export const handleMyInfoChildHashResponse = ( */ export const getMyInfoAttributeConstantsList = ( myInfoAttr: string | string[], -) => { +): string[] | undefined => { switch (myInfoAttr) { case MyInfoAttribute.Occupation: return myInfoOccupations @@ -576,15 +578,22 @@ export const logIfFieldValueNotInMyinfoList = ( fieldValue: string, myInfoAttr: string | string[], myInfoList: string[], + myInfoData: MyInfoData | SGIDMyInfoData, ) => { const isFieldValueInMyinfoList = myInfoList.includes(fieldValue) - if (!isFieldValueInMyinfoList) { + const myInfoSource = + myInfoData instanceof MyInfoData ? 'Singpass MyInfo' : 'SGID MyInfo' + // SGID returns NA instead of empty field values, we don't need this to be logged + // as this is expected behaviour + const isNAFromSgid = myInfoAttr === 'SGID MyInfo' && fieldValue === 'NA' + if (!isNAFromSgid || !isFieldValueInMyinfoList) { logger.error({ message: 'Myinfo field value not found in existing Myinfo constants list', meta: { action: 'prefillAndSaveMyInfoFields', myInfoFieldValue: fieldValue, myInfoAttr, + myInfoSource, }, }) } diff --git a/src/app/modules/sgid/sgid.adapter.ts b/src/app/modules/sgid/sgid.adapter.ts index 1885c239f7..fa5dc4eb4a 100644 --- a/src/app/modules/sgid/sgid.adapter.ts +++ b/src/app/modules/sgid/sgid.adapter.ts @@ -2,27 +2,53 @@ import { MyInfoAttribute as InternalAttr, MyInfoDataTransformer, } from '../../../../shared/types' +import { createLoggerWithLabel } from '../../config/logger' import { SGID_MYINFO_NRIC_NUMBER_SCOPE, SGIDScope as ExternalAttr, } from './sgid.constants' +import { formatAddress, formatVehicles } from './sgid.format' import { SGIDScopeToValue } from './sgid.types' +const logger = createLoggerWithLabel(module) + export const internalAttrToScope = (attr: InternalAttr): ExternalAttr => { switch (attr) { case InternalAttr.Name: return ExternalAttr.Name + case InternalAttr.Sex: + return ExternalAttr.Sex case InternalAttr.DateOfBirth: return ExternalAttr.DateOfBirth + case InternalAttr.Race: + return ExternalAttr.Race + case InternalAttr.Nationality: + return ExternalAttr.Nationality + case InternalAttr.HousingType: + return ExternalAttr.HousingType + case InternalAttr.HdbType: + return ExternalAttr.HdbType case InternalAttr.PassportNumber: return ExternalAttr.PassportNumber case InternalAttr.PassportExpiryDate: return ExternalAttr.PassportExpiryDate - case InternalAttr.MobileNo: - return ExternalAttr.MobileNumber case InternalAttr.RegisteredAddress: return ExternalAttr.RegisteredAddress + case InternalAttr.BirthCountry: + return ExternalAttr.BirthCountry + case InternalAttr.VehicleNo: + return ExternalAttr.VehicleNo + case InternalAttr.Employment: + return ExternalAttr.Employment + case InternalAttr.WorkpassStatus: + return ExternalAttr.WorkpassStatus + case InternalAttr.WorkpassExpiryDate: + return ExternalAttr.WorkpassExpiryDate + case InternalAttr.Marital: + return ExternalAttr.MaritalStatus + case InternalAttr.MobileNo: + return ExternalAttr.MobileNoWithCountryCode default: // This should be removed once sgID reaches parity with MyInfo. // For now, the returned value will be automatically filtered @@ -44,16 +70,38 @@ const internalAttrToSGIDExternal = ( switch (attr) { case InternalAttr.Name: return ExternalAttr.Name + case InternalAttr.Sex: + return ExternalAttr.Sex case InternalAttr.DateOfBirth: return ExternalAttr.DateOfBirth + case InternalAttr.Race: + return ExternalAttr.Race + case InternalAttr.Nationality: + return ExternalAttr.Nationality + case InternalAttr.HousingType: + return ExternalAttr.HousingType + case InternalAttr.HdbType: + return ExternalAttr.HdbType case InternalAttr.PassportNumber: return ExternalAttr.PassportNumber case InternalAttr.PassportExpiryDate: return ExternalAttr.PassportExpiryDate case InternalAttr.MobileNo: - return ExternalAttr.MobileNumber + return ExternalAttr.MobileNoWithCountryCode case InternalAttr.RegisteredAddress: return ExternalAttr.RegisteredAddress + case InternalAttr.BirthCountry: + return ExternalAttr.BirthCountry + case InternalAttr.VehicleNo: + return ExternalAttr.VehicleNo + case InternalAttr.Employment: + return ExternalAttr.Employment + case InternalAttr.WorkpassStatus: + return ExternalAttr.WorkpassStatus + case InternalAttr.WorkpassExpiryDate: + return ExternalAttr.WorkpassExpiryDate + case InternalAttr.Marital: + return ExternalAttr.MaritalStatus default: return undefined } @@ -79,16 +127,37 @@ export class SGIDMyInfoData /** * Placeholder. For now, there are not enough public fields in * sgID's MyInfo catalog to require significant formatting. + * SGID returns 'NA' for field values that do not exist (vs empty string returned by Singpass MyInfo) * @param attr sgID's MyInfo OAuth scope. * @returns the formatted field. */ _formatFieldValue(attr: ExternalAttr): string | undefined { - return this.#payload[attr] + const fieldValue = this.#payload[attr] + switch (attr) { + case ExternalAttr.RegisteredAddress: + return formatAddress(fieldValue) + case ExternalAttr.VehicleNo: + return formatVehicles(fieldValue) + default: + // SGID returns NA if they do not have the value. We want to return an empty string instead, + // so that this can be processed by our frontend in the same way as Singpass MyInfo + return fieldValue === 'NA' ? '' : fieldValue + } } /** - * Refer to the myInfo data catalogue to see which fields should be read-only - * and which fields should be editable by the user. + * SGID only returns verified MyInfo fields, unless the field contains marriage-related information + * (decision by SNDGO & MSF due to overseas unregistered marriages). + * An empty myInfo field will always evaluate + * to false so that the field can be filled by form-filler. + * + * The affected marriage fields are: + * - marital + * - marriagedate + * - divorcedate + * - countryofmarriage + * - marriagecertno + * * @param attr sgID MyInfo OAuth scope. * @param fieldValue FormSG field value. * @returns Whether the data/field should be readonly. @@ -98,15 +167,36 @@ export class SGIDMyInfoData if (!data || !fieldValue) return false switch (attr) { - case ExternalAttr.MobileNumber: + case ExternalAttr.MobileNoWithCountryCode: case ExternalAttr.RegisteredAddress: case ExternalAttr.Name: case ExternalAttr.PassportNumber: case ExternalAttr.DateOfBirth: case ExternalAttr.PassportExpiryDate: + case ExternalAttr.Sex: + case ExternalAttr.Race: + case ExternalAttr.Nationality: + case ExternalAttr.HousingType: + case ExternalAttr.HdbType: + case ExternalAttr.BirthCountry: + case ExternalAttr.VehicleNo: + case ExternalAttr.Employment: + case ExternalAttr.WorkpassStatus: + case ExternalAttr.WorkpassExpiryDate: return !!data + // Fields required to always be editable according to MyInfo docs + case ExternalAttr.MaritalStatus: + return false // Fall back to leaving field editable as data shape is unknown. default: + logger.error({ + message: 'Unknown attribute found in SGID MyInfo field', + meta: { + action: '_isDataReadOnly', + fieldValue, + attr, + }, + }) return false } } @@ -127,9 +217,17 @@ export class SGIDMyInfoData } } const fieldValue = this._formatFieldValue(externalAttr) + logger.info({ + message: 'get field value', + meta: { + action: 'getFieldValueForAttr', + fieldValue, + externalAttr, + }, + }) return { fieldValue, - isReadOnly: true, + isReadOnly: this._isDataReadOnly(externalAttr, fieldValue), } } } diff --git a/src/app/modules/sgid/sgid.constants.ts b/src/app/modules/sgid/sgid.constants.ts index 57ccc8855a..c8884b2e06 100644 --- a/src/app/modules/sgid/sgid.constants.ts +++ b/src/app/modules/sgid/sgid.constants.ts @@ -33,7 +33,20 @@ export enum SGIDScope { DateOfBirth = 'myinfo.date_of_birth', PassportNumber = 'myinfo.passport_number', PassportExpiryDate = 'myinfo.passport_expiry_date', - MobileNumber = 'myinfo.mobile_number', Email = 'myinfo.email', RegisteredAddress = 'myinfo.registered_address', + Sex = 'myinfo.sex', + Race = 'myinfo.race', + Nationality = 'myinfo.nationality', + HousingType = 'myinfo.housingtype', + HdbType = 'myinfo.hdbtype', + BirthCountry = 'myinfo.birth_country', + VehicleNo = 'myinfo.vehicles', + Employment = 'myinfo.name_of_employer', + WorkpassStatus = 'myinfo.workpass_status', + WorkpassExpiryDate = 'myinfo.workpass_expiry_date', + MaritalStatus = 'myinfo.marital_status', + // SGID also has another myinfo.mobile_number field that does not contain the country code prefix. + // We use the one that contains prefix, as this matches our mobile number form field. + MobileNoWithCountryCode = 'myinfo.mobile_number_with_country_code', } diff --git a/src/app/modules/sgid/sgid.controller.ts b/src/app/modules/sgid/sgid.controller.ts index 8223649dae..966671c401 100644 --- a/src/app/modules/sgid/sgid.controller.ts +++ b/src/app/modules/sgid/sgid.controller.ts @@ -2,6 +2,7 @@ import { ParamsDictionary } from 'express-serve-static-core' import { StatusCodes } from 'http-status-codes' import { FormAuthType } from '../../../../shared/types' +import { Environment } from '../../../types' import config from '../../config/config' import { createLoggerWithLabel } from '../../config/logger' import { ControllerHandler } from '../core/core.types' @@ -38,7 +39,15 @@ export const handleLogin: ControllerHandler< } const { formId, rememberMe, decodedQuery } = parsedState.value - const target = decodedQuery ? `/${formId}${decodedQuery}` : `/${formId}` + + // For local dev, we need to specify the frontend app URL as this is different from the backend's app URL + const redirectTargetRaw = + process.env.NODE_ENV === Environment.Dev + ? `${config.app.feAppUrl}/${formId}` + : `/${formId}` + const target = decodedQuery + ? `${redirectTargetRaw}${decodedQuery}` + : `${redirectTargetRaw}` const formResult = await FormService.retrieveFullFormById(formId) if (formResult.isErr()) { diff --git a/src/app/modules/sgid/sgid.format.ts b/src/app/modules/sgid/sgid.format.ts new file mode 100644 index 0000000000..73d93dcc4d --- /dev/null +++ b/src/app/modules/sgid/sgid.format.ts @@ -0,0 +1,43 @@ +import { createLoggerWithLabel } from '../../config/logger' + +const logger = createLoggerWithLabel(module) + +/** + * Formats SGID MyInfo attribute as address. + * SGID MyInfo multi-line address are newline-separated, while MyInfo multi-line addresses are comma-separated + * @param addr The address to format. + * @example '29 ROCHDALE ROAD\n\nSINGAPORE 535842' + * @returns Formatted address is comma separated, same as the output of formatAddress in myinfo.format.ts + */ +export const formatAddress = (addr: string): string => { + const formattedAddress = addr.replace(/(\n)+/g, ', ') + return formattedAddress +} + +/** + * Formats SGID vehicle types. + * SGID vehicles are a stringified array of objects, we want to output this as a string + * @param vehicles The vehicles to format. + * @example '[{"vehicle_number":"CB6171D"},{"vehicle_number":"SJQ7247B"}]' + * @returns Formatted address is comma separated, same as the output of formatAddress in myinfo.format.ts + */ +export const formatVehicles = (vehicles: string): string => { + if (vehicles && vehicles !== '[]') { + try { + const vehiclesObject = JSON.parse(vehicles) + const vehicleNos = vehiclesObject + .map((vehicle: { vehicle_number: string }) => vehicle['vehicle_number']) + .join(', ') + return vehicleNos + } catch (error) { + logger.error({ + message: 'Failed to parse vehicles', + meta: { action: 'formatVehicles', vehicles }, + error, + }) + return '' + } + } else { + return '' + } +}