PP-2696 Reinstate get transactions with refunds #462
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rauligs
force-pushed
the
PP-2696-Reinstate-Get-Transactions-With-Refunds
branch
from
f1cf830 to
7dacbb4
Compare
- Add _@Transactional_ annotation to TransactionDao since this is
causing issues, so PP-2515 can be added back:
google/guice#730
- Reinstante PP-2515 Implementation of `TransactionDao`
The implementation of `TransactionDao` is meant to be used a as
drop-in replacement for `ChargeDao` when we want to take refunds into
consideration while searching.
- A transaction can be of two types `charge` or `refund`. Depending
on whether we want to search for only charges or refunds, an
optional payment type can be specified in the filtering
criteria. The absence of a payment type in the search criteria
implies all types are considered during the search.
- The filtering criteria has been augmented to allow searching by
refund status or charge status or any combination of both.
- Technical note: Performance being an overriding concern when
searching for transactions, the underlying query required to be
hand-optimized in order to get a decent execution plan out of a
SQL UNION with pagination. Since JPA tend to get in the way
because of its lack of support for UNION query, we were forced to
resort to using a native query constructed dynamically with the
support of jOOQ's query DSL API.
https://www.jooq.org/doc/3.9/manual/getting-started/use-cases/jooq-as-a-standalone-sql-builder/
- Reinstante PP-2515 Mechanism to switch between fetching strategies
Introduce strategies for fetching transactions:
- `TransactionSearchStrategy` fetches both charges and refunds
- `ChargeSearchStrategy` fetches charges only, using the original implementation
- Infer transaction_type from states (payments or refunds)
The strategy is selected according to the `REFUNDS_IN_TX_LIST` feature flag.
Testing:
- `TransactionDaoITest` exercises the query operations of the TransactionDao
- `ChargesApiResourceITest` exercises all layers from resource down, with `REFUNDS_IN_TX_LIST` disabled
- `TransactionsApiResourceITest` exercises all layers from resource down, with `REFUNDS_IN_TX_LIST` enabled
The strategy and switching via feature flag are temporary. We intend to
replace the `TransactionSearchStrategy` implementation after refactoring the
data model to store charges and refunds in the same database table.
Reinstante Clarify escaping of like clause:
- Having a function called `escape` immediately sets off alarm bells when
building SQL code. Why are we needing to explicitly escape a string? In fact we
are quoting the special symbols `%` and `_` which can appear in a like clause.
rauligs
force-pushed
the
PP-2696-Reinstate-Get-Transactions-With-Refunds
branch
from
7dacbb4 to
c7eb98c
Compare
kakumara
reviewed
Oct 2, 2017
| @@ -176,6 +176,11 @@ | |||
| <artifactId>commons-collections</artifactId> | |||
| <version>3.2.2</version> | |||
| </dependency> | |||
| <dependency> | |||
| <groupId>org.jooq</groupId> | |||
There was a problem hiding this comment.
I don't really see the benefits of using this. SQL is perfectly readable/manageble to me. 😐
There was a problem hiding this comment.
-
This code is temporary. Connector refactoring to use concept of transactions is in spike at the moment. See https://payments-platform.atlassian.net/browse/PP-2544
-
This will remove this complexity and jooq can be removed.
| @@ -94,20 +90,58 @@ public ChargeResponse buildChargeResponse(UriInfo uriInfo, ChargeEntity chargeEn | |||
| .orElseGet(Optional::empty); | |||
| } | |||
|
|
|||
| public <T extends AbstractChargeResponseBuilder<T, R>, R> AbstractChargeResponseBuilder<T, R> populateResponseBuilderWith(AbstractChargeResponseBuilder<T, R> responseBuilder, UriInfo uriInfo, ChargeEntity chargeEntity) { | |||
There was a problem hiding this comment.
😬 <T extends AbstractChargeResponseBuilder<T, R>, R> AbstractChargeResponseBuilder<T, R>
|
|
||
| import static uk.gov.pay.connector.model.ChargeResponse.aChargeResponseBuilder; | ||
|
|
||
| public class ChargeSearchStrategy extends AbstractSearchStrategy<ChargeEntity> implements SearchStrategy { |
There was a problem hiding this comment.
Do you really need to implements SearchStrategy ? AbstractSearchStrategy already does that
| import static uk.gov.pay.connector.resources.ApiPaths.CHARGE_API_PATH; | ||
| import static uk.gov.pay.connector.resources.ApiPaths.REFUNDS_API_PATH; | ||
|
|
||
| public class TransactionSearchStrategy extends AbstractSearchStrategy<Transaction> implements SearchStrategy { |
There was a problem hiding this comment.
Same as above.AbstractSearchStrategy already implement it
kakumara
approved these changes
Oct 2, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
WHAT
Reinstate get transactions with refunds adding the fix to prevent Automatically started UnitOfWork is never ended google/guice#730 by annotating
TransactionDaowith @transactional annotation.Reinstate story: https://payments-platform.atlassian.net/browse/PP-2515
Original PR: PP-2515 Get payments including refunds #439
Introduced mechanism to switch between fetching transactions:
TransactionSearchStrategyfetches both charges and refundsChargeSearchStrategyfetches charges only, using the original implementationThe strategy is selected according to the
REFUNDS_IN_TX_LISTfeature flag.The strategy and switching via feature flag are temporary. We intend to replace the
TransactionSearchStrategyimplementation after refactoring the data model to store charges and refunds in the same database table.The implementation of
TransactionDaois meant to be used a as drop-in replacement forChargeDaowhen we want to take refunds into consideration while searching.A transaction can be of two types
chargeorrefund. Depending on whether we want to search for only charges or refunds, an optional payment type can be specified in the filtering criteria. The absence of a payment type in the search criteria implies all types are considered during the search.The filtering criteria has been augmented to allow searching by refund status or charge status or any combination of both.