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

Initiate payment request use case based #35

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

martiBpl
Copy link

No description provided.

@martiBpl martiBpl force-pushed the cr/initiate-payment-request-usecase-based branch from 639fc9e to b284fb8 Compare October 21, 2022 10:33
@Mantas-Ragauskas
Copy link

@martiBpl Greetings and thanks for raising a PR!
Would you mind sparing some time and putting down some explanations on the actual problem this PR attempts to resolve?

From what I can see right now - both InitiateBankPaymentRequest & InitiateCardPaymentRequest provide just a wrapper around InitiatePaymentRequest's bankPaymentMethod & cardPaymentMethod, and that hardly seems like the best potential solution for the problem

On top of that - overriding equals & hashCode does make sense based on field types, as long as you encounter comparing the InitiatePaymentRequest objects (was that the original reason for the PR?)
I must note, though, that solving this problem via static builder functions & keeping data class type, would have saved some lines of code as well as keeping the toString method

@martiBpl
Copy link
Author

martiBpl commented Oct 26, 2022

Hi @Mantas-Ragauskas,
The main point of this PR was to have non nullable fields (mostly bankPaymentMethod) where we know exactly that we're initiating a bank payment. Based on that concrete model we wouldn't have check again if the bankPaymentMethod is null and provide dummy default values if its not - and we are sure that it's not.
Given this the builder pattern wouldn't help as it would still be consisting nullable fields.
To be honest the equals and hashCode methods are autogenerated by the IDE, those where by default provided by the kotlin data class which I needed to get rid of, because of the inheritance.

One other solution would be to create a separate model class (non inherited) which would have a method mapToInitiatePaymentRequest.

Side question: from the docs I've read that when creating a card payment you have to always provide a bankPaymentMethod

Initiate payment by calling the /pis/payment endpoint with both bankPaymentMethod and cardPaymentMethod objects, and set paymentMethodPreferred to card

Does this mean that the bankPaymentMethod is required at all times?

@saltanovas
Copy link
Member

saltanovas commented Oct 27, 2022

Does this mean that the bankPaymentMethod is required at all times?

It should be. It would be way easier to just mark it as mandatory, even if that is only a business rule that is not implemented on the API yet.

Will talk with the Platform team. They consider to implement such business rule on the API as well.

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.

3 participants