Full init code snapshot for review - #7
Conversation
1d1f705 to
784c7db
Compare
| <parent> | ||
| <groupId>dev.vality</groupId> | ||
| <artifactId>service-parent-pom</artifactId> | ||
| <version>4.0.0-BETA-1</version> |
There was a problem hiding this comment.
Давай тут актуализируем, раз из "беты" конфиг вышел)
| | Дата | `yyyy-MM-dd` | | ||
| | Время | `HH:mm:ss` | | ||
| | Timezone | `CreateReportRequest.timezone`, по умолчанию UTC | | ||
| | Денежные значения | Decimal по exponent соответствующей валюты | |
There was a problem hiding this comment.
сразу вспоминаю "смотря какой fabric, смотря сколько details"
предлагаю "Decimal по экспоненте соответствующей валюты"
| | Денежные значения | Decimal по exponent соответствующей валюты | | ||
| | `exchange_rate_internal` | Decimal без экспоненциальной записи | | ||
|
|
||
| ### Payments |
There was a problem hiding this comment.
Может для наглядности мелкий тестовый пример отчета с корректным форматом? Для платежей и для выплат. Я вот так и не понял, как выглядит значение в поле exchange_rate_internal, пример бы помог.
| discovery: | ||
| enabled: false | ||
|
|
||
| ccr: |
There was a problem hiding this comment.
Может уберем этот верхний уровень вложенности? Тут все свойства для control-center-reporter'а, а он еще и в названиях классов зашит. Кажется избыточным и не очень удобным для восприятия.
| @@ -0,0 +1,69 @@ | |||
| package dev.vality.ccreporter.handler.support; | |||
There was a problem hiding this comment.
У нас обычно пакеты с утильными и вспомогательными классами как util называются, может в этом сервисе тоже так сделаем? Чтобы не гадать, что там. Я сначала подумал, что это какой-то специфичный для саппорта функционал.
| public static LocalDateTime toOptionalLocalDateTime(Instant value) { | ||
| return value == null ? null : LocalDateTime.ofInstant(value, ZoneOffset.UTC); | ||
| } |
There was a problem hiding this comment.
Предлагаю переименовать на toNullableLocalDateTime - иначе по названию кажется, что тут должен возвращаться Optional<LocalDateTime>
| PAYMENT_TXN_CURRENT.ID, | ||
| PAYMENT_TXN_CURRENT.INVOICE_ID, | ||
| PAYMENT_TXN_CURRENT.PAYMENT_ID, | ||
| PAYMENT_TXN_CURRENT.UPDATED_AT |
There was a problem hiding this comment.
UPDATED_AT вроде бы можно убрать из immutable. Он с одной стороны ничего не ломает, но немного сбивает с толку. Пришлось разбираться, почему UPDATED_AT записан в immutable, хотя кажется, что должен обновлятся после каждой записи (и он корректно обновляется)
| return Optional.empty(); | ||
| } | ||
| var withdrawal = change.getCreated().getWithdrawal(); | ||
| var body = withdrawal.getBody(); |
There was a problem hiding this comment.
У withdrawal еще появилось поле newBody с измененной суммой, надо бы его исползовать, если задано.
|
|
||
| @Component | ||
| @RequiredArgsConstructor | ||
| public class ReportingHandler implements ReportingSrv.Iface, ThriftLoggingHandler { |
There was a problem hiding this comment.
может его в пакет resource перенести? Еле нашел входную точку)
| public Report getReport(GetReportRequest request) { | ||
| return handleRequest( | ||
| "GetReport", | ||
| () -> ReportingHandlerLogSupport.summarizeGetReport(request), | ||
| () -> reportManagementService.getReport(request), | ||
| ReportingHandlerLogSupport::summarizeReport | ||
| ); | ||
| } | ||
|
|
||
| @Override | ||
| @SneakyThrows | ||
| public GetReportsResponse getReports(GetReportsRequest request) { | ||
| return handleRequest( | ||
| "GetReports", | ||
| () -> ReportingHandlerLogSupport.summarizeGetReports(request), | ||
| () -> reportManagementService.getReports(request), | ||
| ReportingHandlerLogSupport::summarizeGetReportsResponse | ||
| ); | ||
| } |
There was a problem hiding this comment.
Может в рамках этих методов тоже проверять внутри перед выдачей, что отчет не expired? А то можно в теории выдать просроченный отчет. Не знаю, чем грозит, но будто бы так по логике корректнее и меньше шансов, что на нас повлияет кривой cron.
76702ed to
e872515
Compare
e872515 to
0ef4586
Compare
3 participants
демо пр чтоб восстановить первое необходимое ревью фул проекта , но сами ветки указывают на этот основной пр #8
то есть тут все актуально но отображается "как новое" но мержить в мастер будем #8 а этот закроем
основной объем это файлы в тестах с возможными ивентами платежей
сопроводительная инфа есть в /docs - у нас условно 3 направления - запись из кафки данных, трифт апи для операций с отчетами (открыть закрыть получить) , и лайфцикл построения отчета . в лайфцикле отчетов есть некоторый согласованный обвес обсулиживания таймаутов\аварийныйх ситуаций , например если отчет строится больше чем 20мин мы убиваем воркер и резетаем дб коннект итп