From 415a76b6bfeff16cee47a795a71db94b5cf5de6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=B9=AF=E6=9C=AC=20=E9=96=8B?= Date: Mon, 14 Aug 2023 06:52:13 +0000 Subject: [PATCH] =?UTF-8?q?Merged=20PR=20325:=20=E3=83=86=E3=82=B9?= =?UTF-8?q?=E3=83=88=E3=82=92=E6=9C=80=E6=96=B0=E5=8C=96=EF=BC=88=E3=82=A2?= =?UTF-8?q?=E3=82=AB=E3=82=A6=E3=83=B3=E3=83=88=E7=99=BB=E9=8C=B2=EF=BC=89?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 概要 [Task2397: テストを最新化（アカウント登録）](https://paruru.nds-tyo.co.jp:8443/tfs/ReciproCollection/fa4924a4-d079-4fab-9fb5-a9a11eb205f0/_workitems/edit/2397) - アカウント登録のテストを最新化（元が無かったので実質新規追加） - テスト時、外部サービスへの依存をMockに差し替える際のモデルケースを追加 - `Object.defineProperty` を使って強引にメンバ変数をMock用オブジェクトへ上書きする方式 - migrationとEntityの定義内容が食い違っていた部分をmigration側に合わせるよう修正 - 実質的にEntityの属性指定は通常実行時には使われないと思われたため、部分的にはテスト用に修正（※意図はコメント済） ## レビューポイント - テスト時、AdB2cServiceやSendGridService等のDB以外のサービスをMockに差し替えて動かしているが、この方式に問題や懸念点はなさそうか - 例えば、`/app/dictation_server/src/features/users/test/utility.ts` に定義されている `makeTestingModuleWithAdb2c` のように構築時にoverrideする形式が既にあるが、そちらの方式でやるべきでは？ 等 - 上記方式ではなく `Object.defineProperty` で良いのでは？という提案を行っているのは、以下の理由 - モジュール構築時に指定する方法だと依存してるサービス数によっては構築時の指定パラメータが膨大になってしまう懸念がある（3~4個のサービスと連携している場合、`makeTestingModuleWithXXX(...)` の引数指定がえらいことになりそう） - `Object.defineProperty` 形式だとテストケース内で必要なサービスを1個1個上書きしていく事が可能なので、汎用関数として切り出して各Serviceで利用できる - 現状のテストケースとして妥当か - 例: SendGridの送信に失敗した場合は最終的にはリカバリを行う実装が入るが、現段階ではリカバリ処理がないためテスト記述なし - Entityの属性を修正したが問題なさそうか、特にテスト用に指定した箇所があるが問題ないか ## 動作確認状況 - npm run testで成功 --- dictation_server/src/common/test/overrides.ts | 63 +++++++ .../accounts/accounts.service.spec.ts | 176 +++++++++++++++++- .../src/features/accounts/accounts.service.ts | 6 +- .../src/features/accounts/test/utility.ts | 44 +++++ .../accounts/entity/account.entity.ts | 18 +- .../repositories/users/entity/user.entity.ts | 18 +- 6 files changed, 301 insertions(+), 24 deletions(-) create mode 100644 dictation_server/src/common/test/overrides.ts diff --git a/dictation_server/src/common/test/overrides.ts b/dictation_server/src/common/test/overrides.ts new file mode 100644 index 0000000..054573e --- /dev/null +++ b/dictation_server/src/common/test/overrides.ts @@ -0,0 +1,63 @@ +import { ConflictError } from '../../gateways/adb2c/adb2c.service'; + +/** + * adB2cServiceのモックを作成して、TServiceが依存するサービス(adB2cService)の参照を上書きする + * ※ serviceに指定するオブジェクトは`adB2cService: AdB2cService`メンバ変数を持つ必要がある + * @param service 上書きしたいTService + * @param overrides adB2cServiceの各種メソッドのモックが返す値（省略した場合は既定のダミーの値） + */ +export const overrideAdB2cService = ( + service: TService, + overrides: { + createUser?: ( + email: string, + password: string, + username: string, + ) => Promise<{ sub: string } | ConflictError>; + }, +): void => { + const { createUser } = overrides; + Object.defineProperty(service, 'adB2cService', { + value: { + createUser: createUser ?? jest.fn().mockResolvedValue({ sub: 'dummy' }), + }, + }); +}; + +/** + * sendgridServiceのモックを作成して、TServiceが依存するサービス(sendgridService)の参照を上書きする + * ※ serviceに指定するオブジェクトは`sendgridService: SendgridService`メンバ変数を持つ必要がある + * @param service 上書きしたいTService + * @param overrides sendgridServiceの各種メソッドのモックが返す値（省略した場合は既定のダミーの値） + */ +export const overrideSendgridService = ( + service: TService, + overrides: { + createMailContentFromEmailConfirm?: ( + accountId: number, + userId: number, + email: string, + ) => Promise<{ subject: string; text: string; html: string }>; + sendMail?: ( + to: string, + from: string, + subject: string, + text: string, + html: string, + ) => Promise; + }, +): void => { + const { createMailContentFromEmailConfirm, sendMail } = overrides; + Object.defineProperty(service, 'sendgridService', { + value: { + createMailContentFromEmailConfirm: + createMailContentFromEmailConfirm ?? + jest.fn().mockResolvedValue({ + subject: 'dummySubject', + text: 'dummyText', + html: 'dummyHtml', + }), + sendMail: sendMail ?? jest.fn().mockResolvedValue(undefined), + }, + }); +}; diff --git a/dictation_server/src/features/accounts/accounts.service.spec.ts b/dictation_server/src/features/accounts/accounts.service.spec.ts index 4c523e8..f068766 100644 --- a/dictation_server/src/features/accounts/accounts.service.spec.ts +++ b/dictation_server/src/features/accounts/accounts.service.spec.ts @@ -16,6 +16,10 @@ import { createLicenseOrder, createUser, createLicenseSetExpiryDateAndStatus, + getAccount, + getUser, + getAccounts, + getUsers, } from './test/utility'; import { DataSource } from 'typeorm'; import { makeTestingModule } from '../../common/test/modules'; @@ -23,6 +27,176 @@ import { AccountsService } from './accounts.service'; import { makeContext } from '../../common/log'; import { TIERS } from '../../constants'; import { License } from '../../repositories/licenses/entity/license.entity'; +import { + overrideAdB2cService, + overrideSendgridService, +} from '../../common/test/overrides'; + +describe('createAccount', () => { + let source: DataSource = null; + beforeEach(async () => { + source = new DataSource({ + type: 'sqlite', + database: ':memory:', + logging: false, + entities: [__dirname + '/../../**/*.entity{.ts,.js}'], + synchronize: true, // trueにすると自動的にmigrationが行われるため注意 + }); + return source.initialize(); + }); + + afterEach(async () => { + await source.destroy(); + source = null; + }); + + it('アカウントを作成できる', async () => { + const module = await makeTestingModule(source); + const service = module.get(AccountsService); + + const externalId = 'test_external_id'; + const companyName = 'test_company_name'; + const country = 'US'; + const dealerAccountId = 1; + const email = 'dummy@dummy.dummy'; + const password = 'dummy_password'; + const username = 'dummy_username'; + const role = 'admin none'; + const acceptedTermsVersion = '1.0.0'; + + overrideAdB2cService(service, { + createUser: async () => { + return { sub: externalId }; + }, + }); + overrideSendgridService(service, {}); + + const { accountId, externalUserId, userId } = await service.createAccount( + companyName, + country, + dealerAccountId, + email, + password, + username, + role, + acceptedTermsVersion, + ); + // 作成したアカウントのIDが返ってくるか確認 + expect(accountId).toBe(1); + expect(externalUserId).toBe(externalId); + expect(userId).toBe(1); + + // DB内が想定通りになっているか確認 + const account = await getAccount(source, accountId); + const user = await getUser(source, externalUserId); + expect(account.company_name).toBe(companyName); + expect(account.country).toBe(country); + expect(account.parent_account_id).toBe(dealerAccountId); + expect(account.tier).toBe(TIERS.TIER5); + expect(account.primary_admin_user_id).toBe(user.id); + expect(account.secondary_admin_user_id).toBe(null); + expect(user.accepted_terms_version).toBe(acceptedTermsVersion); + expect(user.account_id).toBe(accountId); + expect(user.role).toBe(role); + }); + + it('アカウントを作成がAzure AD B2Cへの通信失敗によって失敗すると500エラーが発生する', async () => { + const module = await makeTestingModule(source); + const service = module.get(AccountsService); + + overrideAdB2cService(service, { + createUser: async () => { + throw new Error(); + }, + }); + + overrideSendgridService(service, {}); + + const companyName = 'test_company_name'; + const country = 'US'; + const dealerAccountId = 1; + const email = 'dummy@dummy.dummy'; + const password = 'dummy_password'; + const username = 'dummy_username'; + const role = 'admin none'; + const acceptedTermsVersion = '1.0.0'; + + try { + await service.createAccount( + companyName, + country, + dealerAccountId, + email, + password, + username, + role, + acceptedTermsVersion, + ); + } catch (e) { + if (e instanceof HttpException) { + expect(e.getStatus()).toBe(HttpStatus.INTERNAL_SERVER_ERROR); + expect(e.getResponse()).toEqual(makeErrorResponse('E009999')); + } else { + expect(true).toBe(false); // ここには来てはいけない + } + } + + // DB内が想定通りになっているか確認 + const accounts = await getAccounts(source); + expect(accounts.length).toBe(0); + const users = await getUsers(source); + expect(users.length).toBe(0); + }); + + it('アカウントを作成がメールアドレス重複によって失敗すると400エラーが発生する', async () => { + const module = await makeTestingModule(source); + const service = module.get(AccountsService); + + overrideAdB2cService(service, { + createUser: async () => { + // EmailのConflictエラーを返す + return { reason: 'email', message: 'dummy' }; + }, + }); + + overrideSendgridService(service, {}); + + const companyName = 'test_company_name'; + const country = 'US'; + const dealerAccountId = 1; + const email = 'dummy@dummy.dummy'; + const password = 'dummy_password'; + const username = 'dummy_username'; + const role = 'admin none'; + const acceptedTermsVersion = '1.0.0'; + + try { + await service.createAccount( + companyName, + country, + dealerAccountId, + email, + password, + username, + role, + acceptedTermsVersion, + ); + } catch (e) { + if (e instanceof HttpException) { + expect(e.getStatus()).toBe(HttpStatus.BAD_REQUEST); + expect(e.getResponse()).toEqual(makeErrorResponse('E010301')); + } else { + expect(true).toBe(false); // ここには来てはいけない + } + } + + // DB内が想定通りになっているか確認 + const accounts = await getAccounts(source); + expect(accounts.length).toBe(0); + const users = await getUsers(source); + expect(users.length).toBe(0); + }); +}); describe('AccountsService', () => { it('アカウントに紐づくライセンス情報を取得する', async () => { @@ -505,7 +679,7 @@ describe('getPartnerAccount', () => { ); // 有効期限が14日後のライセンスを追加（5ライセンス） - let expiryDate = new Date(); + const expiryDate = new Date(); expiryDate.setDate(expiryDate.getDate() + 14); expiryDate.setHours(23, 59, 59, 999); for (let i = 0; i < 5; i++) { diff --git a/dictation_server/src/features/accounts/accounts.service.ts b/dictation_server/src/features/accounts/accounts.service.ts index 5d08225..bbe0e39 100644 --- a/dictation_server/src/features/accounts/accounts.service.ts +++ b/dictation_server/src/features/accounts/accounts.service.ts @@ -10,11 +10,7 @@ import { } from '../../gateways/adb2c/adb2c.service'; import { Account } from '../../repositories/accounts/entity/account.entity'; import { User } from '../../repositories/users/entity/user.entity'; -import { - LICENSE_EXPIRATION_THRESHOLD_DAYS, - TIERS, - USER_ROLES, -} from '../../constants'; +import { TIERS, USER_ROLES } from '../../constants'; import { makeErrorResponse } from '../../common/error/makeErrorResponse'; import { TypistGroup, diff --git a/dictation_server/src/features/accounts/test/utility.ts b/dictation_server/src/features/accounts/test/utility.ts index 31f4237..e66bff0 100644 --- a/dictation_server/src/features/accounts/test/utility.ts +++ b/dictation_server/src/features/accounts/test/utility.ts @@ -30,6 +30,50 @@ export const createAccount = async ( return { accountId: account.id }; }; +/** + * テスト ユーティリティ: 指定IDのアカウントを取得する + * @param dataSource データソース + * @param id アカウントID + * @returns 該当アカウント + */ +export const getAccount = async (dataSource: DataSource, id: number) => { + return await dataSource.getRepository(Account).findOne({ + where: { id: id }, + }); +}; + +/** + * テスト ユーティリティ: すべてのアカウントを取得する + * @param dataSource データソース + * @returns　該当アカウント一覧 + */ +export const getAccounts = async ( + dataSource: DataSource, +): Promise => { + return await dataSource.getRepository(Account).find(); +}; + +/** + * テスト ユーティリティ: 指定ExternalIdのアカウントを取得する + * @param dataSource データソース + * @param externalId 外部ID + * @returns 該当ユーザー + */ +export const getUser = async (dataSource: DataSource, externalId: string) => { + return await dataSource.getRepository(User).findOne({ + where: { external_id: externalId }, + }); +}; + +/** + * テスト ユーティリティ: すべてのユーザーを取得する + * @param dataSource データソース + * @returns 該当ユーザー一覧 + */ +export const getUsers = async (dataSource: DataSource): Promise => { + return await dataSource.getRepository(User).find(); +}; + export const createLicense = async ( datasource: DataSource, accountId: number, diff --git a/dictation_server/src/repositories/accounts/entity/account.entity.ts b/dictation_server/src/repositories/accounts/entity/account.entity.ts index 82d57ea..ad65437 100644 --- a/dictation_server/src/repositories/accounts/entity/account.entity.ts +++ b/dictation_server/src/repositories/accounts/entity/account.entity.ts @@ -22,16 +22,16 @@ export class Account { @Column() country: string; - @Column() + @Column({ default: false }) delegation_permission: boolean; - @Column() + @Column({ default: false }) locked: boolean; @Column() company_name: string; - @Column() + @Column({ default: false }) verified: boolean; @Column({ nullable: true }) @@ -43,16 +43,16 @@ export class Account { @Column({ nullable: true }) deleted_at?: Date; - @Column() - created_by: string; + @Column({ nullable: true }) + created_by?: string; - @CreateDateColumn() + @CreateDateColumn({ default: () => "datetime('now', 'localtime')" }) // defaultはSQLite用設定値.本番用は別途migrationで設定 created_at: Date; - @Column() - updated_by: string; + @Column({ nullable: true }) + updated_by?: string; - @UpdateDateColumn() + @UpdateDateColumn({ default: () => "datetime('now', 'localtime')" }) // defaultはSQLite用設定値.本番用は別途migrationで設定 updated_at: Date; @OneToMany(() => User, (user) => user.id) diff --git a/dictation_server/src/repositories/users/entity/user.entity.ts b/dictation_server/src/repositories/users/entity/user.entity.ts index 1e01643..7ed3f88 100644 --- a/dictation_server/src/repositories/users/entity/user.entity.ts +++ b/dictation_server/src/repositories/users/entity/user.entity.ts @@ -33,16 +33,16 @@ export class User { @Column({ nullable: true }) accepted_terms_version?: string; - @Column() + @Column({ default: false }) email_verified: boolean; - @Column() + @Column({ default: true }) auto_renew: boolean; - @Column() + @Column({ default: true }) license_alert: boolean; - @Column() + @Column({ default: true }) notification: boolean; @Column({ nullable: true }) @@ -57,16 +57,16 @@ export class User { @Column({ nullable: true }) deleted_at?: Date; - @Column() + @Column({ nullable: true }) created_by: string; - @CreateDateColumn() + @CreateDateColumn({ default: () => "datetime('now', 'localtime')" }) // defaultはSQLite用設定値.本番用は別途migrationで設定 created_at: Date; - @Column() - updated_by: string; + @Column({ nullable: true }) + updated_by?: string; - @UpdateDateColumn() + @UpdateDateColumn({ default: () => "datetime('now', 'localtime')" }) // defaultはSQLite用設定値.本番用は別途migrationで設定 updated_at: Date; @ManyToOne(() => Account, (account) => account.user)