[MODFISTO-460] Removed the old transaction API #408
Conversation
| @@ -366,7 +361,7 @@ | |||
| { | |||
| "tableName": "fund_distribution", | |||
| "fromModuleVersion": "mod-finance-storage-4.2.1", | |||
| "mode": "delete", | |||
| "mode": "DELETE", | |||
There was a problem hiding this comment.
What do you think about delete declaration completely instead of using "mode"" : "DELETE" ? It should work in the same way but schema.json will be simpler
There was a problem hiding this comment.
Are you sure the tables would get deleted ?
If yes, I feel like this can still be useful for documentation purpose (for instance if we hear a report that some customer using an old version has some data in a particular table and that table is no longer used, we will figure it out faster when seeing that table with the DELETE mode in the schema), but we could probably remove deleted tables from 4 versions back or more.
There was a problem hiding this comment.
yes it should be deleted but I am ok alsot remain as is
There was a problem hiding this comment.
Now that I think about it, I see another reason to keep this code: it helps to make sure we don't create a new table with the same name as an old table, which could potentially cause issues when upgrading FOLIO.
src/test/java/org/folio/service/transactions/cancel/CancelPaymentCreditServiceTest.java
Show resolved
Hide resolved
| Error error = new Error() | ||
| .withCode(ErrorCodes.GENERIC_ERROR_CODE.getCode()) | ||
| .withMessage(message) | ||
| .withParameters(singletonList(causeParam)); |
There was a problem hiding this comment.
Can we use List.of(), As far as I know singletonList will allow null
There was a problem hiding this comment.
I was prefering singletonList for single element lists, but I will switch to List.of.
| import static io.vertx.core.Future.succeededFuture; | ||
| import static org.folio.dao.ledger.LedgerPostgresDAO.LEDGER_TABLE; | ||
| import static org.folio.dao.transactions.BatchTransactionDAO.TRANSACTIONS_TABLE; |
There was a problem hiding this comment.
some parts import static is the beginning and some parts is in last part, we should use one approach for import style. Old folio documenation says static should be in the beginning
There was a problem hiding this comment.
Makes sense to me. I was just letting my editor figuring it.
There was a problem hiding this comment.
related bug (for google style)
There was a problem hiding this comment.
Actually, it looks like the default in Intellij is to put static imports last. So I prefer not to change that at this point (if we wanted to, we would have to let all developers know that they have to change their settings).
There was a problem hiding this comment.
Also we should probably change that in a separate ticket if we want to do it. The current order is inconsistent, and this PR is already hard to read.
There was a problem hiding this comment.
I personally prefer intellij idea default, but to be consitent I am using folio import style. I favour of moving towards to default intellij idea import style. In that way, no one have to change its import settings.
|
Purpose
MODFISTO-460 - Remove the old transaction API (storage)
Approach
See ticket
mod-finance PR to update the version
acq-models PR to merge after this one
Pre-Merge Checklist:
Before merging this PR, please go through the following list and take appropriate actions.
If there are breaking changes, please STOP and consider the following:
Ideally, all the PRs involved in breaking changes would be merged on the same day
to avoid breaking the folio-testing environment.
Communication is paramount if that is to be achieved,
especially as the number of inter-module and inter-team dependencies increase.
While it's helpful for reviewers to help identify potential problems,
ensuring that it's safe to merge is ultimately the responsibility of the PR assignee.