feat: batchCreateWallet & batchTransferWallet service and controller #492
Conversation
| destination: './uploads', | ||
| filename: (req, file, callback) => { | ||
| const uniqueSuffix = | ||
| Date.now() + '-' + Math.round(Math.random() * 1e9); |
There was a problem hiding this comment.
Another option could be to use a uuid
| }), | ||
| ) | ||
| async batchCreateWallet( | ||
| @Body('sender_wallet') senderWallet: string, |
There was a problem hiding this comment.
Create a dto folder and a dto file that matches the body. So we don't have to have @Body three times, we can just have one
| csvJson, | ||
| file.path, | ||
| ); | ||
| } catch (error) { |
There was a problem hiding this comment.
Let's have the try catch in the service file
src/modules/wallet/wallet.service.ts
Outdated
|
|
||
| return { | ||
| message: 'Batch transfer successful', | ||
| }; | ||
| } catch (e) { | ||
| await fs.unlink(filePath); | ||
| if (!process.env.JEST_WORKER_ID) { |
There was a problem hiding this comment.
What's the reason for this?
There was a problem hiding this comment.
@Kpoke to skip fs.unlink(filePath) if the wallet service was called in tests. because deleting a file seems unnecessary while in test as we were justing mocking it. please lmk if there's a better way to handle this! tysm.
There was a problem hiding this comment.
We would also be mocking the fs module for unit tests, so there's no need to check for that. And for integration tests we would be using a mock file to ensure it all works as it should
|
|
yunchipang
changed the title
| token_transfer_amount_overwrite?: number; | ||
| } | ||
|
|
||
| export class BatchTransferWalletDto { |
There was a problem hiding this comment.
Also, can you crosscheck and confirm the validations? If any field is required, use IsNotEmpty; if not, use IsOptional decorators. Also, verify if there are any other types of validations applied.
treetracker-wallet-api/server/handlers/walletHandler/schemas.js
Lines 63 to 100 in bf235b0
For instance, wallet_name should be unique in the csvJson array. And there is no corresponding csvJsonArray validation for BatchCreateWallet
|
|
||
| import { Type } from 'class-transformer'; | ||
|
|
||
| // Custom validator to check for unique `wallet_name` values in the csvJson array |
There was a problem hiding this comment.
@Kpoke validations are added here.
| expect(walletService.getByName).toHaveBeenCalledWith(senderWallet); | ||
| expect(walletService.createWallet).not.toHaveBeenCalled(); | ||
| expect(transferService.transferBundle).not.toHaveBeenCalled(); | ||
| expect(fs.promises.unlink).toHaveBeenCalledWith(filePath); |
There was a problem hiding this comment.
@Kpoke i've removed the check for jest ID from service logic, and added a test for deleting file here.
| } | ||
| } | ||
|
|
||
| export class CsvItemDto { |
There was a problem hiding this comment.
@Kpoke i've added this CsvItemDto for validation purposes for both batchCreateWallet and batchTransferWallet. let me know if this is what you're looking for. thanks!!
|
@Kpoke i made some adjustments and successfully posted to the batch-create-wallet endpoint. batch-transfer has not been tested. just leaving the checkpoint here. thanks! test 1: when wallet_name is empty test 2: when fields are ideal |
|
@Kpoke |
| }[], | ||
| filePath: string, | ||
| ): Promise<{ message: string }> { | ||
| const queryRunner = this.dataSource.createQueryRunner(); |
There was a problem hiding this comment.
@Kpoke batchCreateWallet and batchTransferWallet are made database transactions.
Description
completes #491
What kind of change(s) does this PR introduce?
Please check if the PR fulfils these requirements
Issue
What is the current behavior?
What is the new behavior?
Breaking change
Does this PR introduce a breaking change?
Other useful information