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

Feature/673 extend processpnboundmessagecommandhandler to allow for transaction creation #684

Conversation

ognjenkurtic
Copy link
Collaborator

Description

Introduces BpiMessage Transaction type and additional infrastructure that enables triggering of a command to create a BpiTransaction from a NATS incoming message which is properly formatted. Contains a number of TODOs that will be tackled as part of #669

Related Issue

#673
#669

Motivation and Context

Standard defines that a BpiTransaction can be triggered through a BpiMessage send via a messaging system utilized by the BPI.

How Has This Been Tested

Wrote new unit test and ran unit and e2e tests

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • [x ] New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Request to be added as a Code Owner/Maintainer

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I commit to abide by the Responsibilities of Code Owners/Maintainers.

Copy link
Collaborator

@biscuitdey biscuitdey left a comment

Choose a reason for hiding this comment

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

createNewBpiMessage() is used in two cases: 1. creating a new message & publishing it; 2. in the processInboundMessage command handler. In both cases, the function takes id as a param.

Case 1: Where is the uuid for the message (id) generated?

Case 2: In the processInboundMessage command handler, we are assuming that the other participant has NOT created a new BpiMessage before publishing and sending us the message. But in the createBpiMessage, a message can only be published after creating a new BpiMessage. Wouldn't this cause the processInboundMessage to exit in bpiMessageAlreadyExists?

In the processInboundMessage, I think we should check that the message we have received corresponds to an existing BpiMessage, and we are only handling the tasks or receiving the information of a message already created by some other participant.

public throwIfCreateTransactionInputInvalid() {
// TODO: This is a placeholder, we will add validation rules as we move forward with business logic implementation
return true;
}

public isCreateTransactionInputInvalid(): boolean {
Copy link
Collaborator

@Kasshern Kasshern Jun 1, 2023

Choose a reason for hiding this comment

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

We need to be able to throw if the input is invalid and determine if the input is invalid without throwing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, we throw when we work with REST API and we do not throw when inside a background task (i.e. NATS message processing).

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ognjenkurtic do you intend to use a DeadLetterQueue to collect and process errors? I presume we can set up a NATS queue for that which is then processed by an error handler service, no?

it('Should return no errors and create message dto when validating correct JSON raw message of TRANSACTION type', () => {
// Arrange
const rawMessage =
'{ "id": "0a3dd67c-c031-4b50-95df-0bc5fc1c78b5", "fromBpiSubjectId": "71302cec-0a38-469a-a4e5-f58bdfc4ab32", "toBpiSubjectId": "76cdd901-d87d-4c87-b572-155afe45c128", "content": { "testProp":"testValue" }, "signature": "xyz", "type": 1}';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps the enum type should be given string designations

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You mean for better readability? We can open a separate issue for that but i would not tackle it as a priority. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

not priority

Kasshern
Kasshern previously approved these changes Jun 1, 2023
Copy link
Collaborator

@Kasshern Kasshern left a comment

Choose a reason for hiding this comment

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

Minor comments - Looks good overall

skosito
skosito previously approved these changes Jun 10, 2023
Copy link
Collaborator

@skosito skosito left a comment

Choose a reason for hiding this comment

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

looks good, couple minor comments

!newBpiMessageCandidate.isTransactionMessage()
) {
errors.push(`type: ${newBpiMessageCandidate.type} is unknown`);
return [null, errors];
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a reason to not just throw err or antything else, this seems a bit odd for ts

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We want to collect all errors in one place and want to avoid throw as we are in a background task that should continue to execute

newBpiMessageCandidate.type,
),
);
if (newBpiMessageCandidate.isInfoMessage()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can this condition be checked in command handler since in both cases same command is called and that way we can isolate handling of different types on one place?

Copy link
Collaborator Author

@ognjenkurtic ognjenkurtic Jul 4, 2023

Choose a reason for hiding this comment

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

It is not the same command.

IsInfoMessage -> ProcessInboundBpiMessageCommand
isTransactionMessage -> ProcessInboundBpiTransactionCommand

): Promise<BpiSubjectAccount[]> {
const subjectAccounts: BpiSubjectAccount[] = [];

for (const subjectAccountId of subjectAccountIds) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit but shouldn't this be one query instead of one query per id?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense but i would do it as part of a separate issue. I just followed what was previously there, Chesterton's Fence style. I am going to open an issue to optimize this, not to affect approvals that already exits.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ognjenkurtic
Copy link
Collaborator Author

createNewBpiMessage() is used in two cases: 1. creating a new message & publishing it; 2. in the processInboundMessage command handler. In both cases, the function takes id as a param.

Case 1: Where is the uuid for the message (id) generated?

Case 2: In the processInboundMessage command handler, we are assuming that the other participant has NOT created a new BpiMessage before publishing and sending us the message. But in the createBpiMessage, a message can only be published after creating a new BpiMessage. Wouldn't this cause the processInboundMessage to exit in bpiMessageAlreadyExists?

In the processInboundMessage, I think we should check that the message we have received corresponds to an existing BpiMessage, and we are only handling the tasks or receiving the information of a message already created by some other participant.

Case 1:

Standard requirement is that the ID is produced by the sender. Sender generates one and passes it as part of the request DTO.
[R112]
A BPI message MUST contain a unique message identifier that is generated by the sender.
This allows unique identification of the message through its lifecycle.

Case 2:

processInboundMessageCommandHandler deals with the situation when the sender uses NATS to initiate message creation and publishing to the recipient

createBpiMessageCommandHandler deals with the situation when the senders does the same thing via the REST API.

The logic in both is mostly the same - accept and parse input, create new message, publish message. Not sure what issue are you refering to. Let's walk through the code together on our next sync.

@ognjenkurtic ognjenkurtic dismissed stale reviews from skosito and Kasshern via 91e7034 July 4, 2023 14:05
@ognjenkurtic ognjenkurtic force-pushed the feature/673-extend-processpnboundmessagecommandhandler-to-allow-for-transaction-creation branch from 54c52f3 to 91e7034 Compare July 4, 2023 14:05
Copy link
Collaborator

@biscuitdey biscuitdey left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation. LGTM.

Copy link
Collaborator

@Therecanbeonlyone1969 Therecanbeonlyone1969 left a comment

Choose a reason for hiding this comment

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

LGTM

it('Should return no errors and create message dto when validating correct JSON raw message of TRANSACTION type', () => {
// Arrange
const rawMessage =
'{ "id": "0a3dd67c-c031-4b50-95df-0bc5fc1c78b5", "fromBpiSubjectId": "71302cec-0a38-469a-a4e5-f58bdfc4ab32", "toBpiSubjectId": "76cdd901-d87d-4c87-b572-155afe45c128", "content": { "testProp":"testValue" }, "signature": "xyz", "type": 1}';
Copy link
Collaborator

Choose a reason for hiding this comment

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

not priority

@Therecanbeonlyone1969 Therecanbeonlyone1969 merged commit 0e40e2b into main Jul 7, 2023
@Therecanbeonlyone1969 Therecanbeonlyone1969 deleted the feature/673-extend-processpnboundmessagecommandhandler-to-allow-for-transaction-creation branch July 7, 2023 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants