Łukasz Lewczyński

OLMIS-6029: Added CONSOLIDATE_NOTIFICATIONS feature flag

1. The review is quite big because for all new classes I had to add tests to avoid sonar issues. But the main changes are in the service package when a bunch of new classes has been created based o...

1. The review is quite big because for all new classes I had to add tests to avoid sonar issues. But the main changes are in the service package when a bunch of new classes has been created based on the diagram in the ticket. Also because of comments from the previous versions of the review I had to provide additional changes.
2. Each component of spring integration has two channels: input and output. Only start and end of flow has one channel: output and input, respectively. Everything starts in NotificationToSendRetriever and ends in EmailNotificationChannelHandler. As I said before it is easier to see what happens when you open the diagram from the ticket.

In the end, I am not sure if there is an easy way to remove unnecessary changes. If you still have an issue to understand changes you could try to do this together with the diagram from the ticket.

done

done

changed

changed

changed

changed

OLMIS-5971: fixed broken test

OLMIS-5971: Update test for NotificationTransformer (FEOLMIS-3638)

OLMIS-5971: Updated log level and message (FEOLMIS-3638)

Yes, currently this class does nothing but we change that in OLMIS-6029

Yes, currently this class does nothing but we change that in OLMIS-6029

OLMIS-6017: Added ability to subscribe to get digest notifications
OLMIS-6017: Added ability to subscribe to get digest notifications
Nikodem Graczewski I am not sure if this is a correct type for this field. Could you bring more details about what values should be accepted by the field?

Nikodem Graczewski I am not sure if this is a correct type for this field. Could you bring more details about what values should be accepted by the field?

OLMIS-6017: Added ability to subscribe to get digest notifications

  1. … 8 more files in changeset.
Is it okay that we use commit id (oefdc844...) instead of master branch in the URL?

Is it okay that we use commit id (oefdc844...) instead of master branch in the URL?

OLMIS-6016: Added missing tests

OLMIS-6016: Update changelog

OLMIS-6016: Added DigestConfiguration entity
OLMIS-6016: Added DigestConfiguration entity
OLMIS-6016: Added DigestConfiguration entity

OLMIS-5971: fixed sonar issue

I don't know if there was any discussion about the approach. The first thought was to use the flag to know which notification has been sent. Issues that I found was that I need direct access to an ...

I don't know if there was any discussion about the approach. The first thought was to use the flag to know which notification has been sent. Issues that I found was that I need direct access to an aggregate resource (NotificationMessage) and this breaks DDD (because I should always go thought Notification class). Also, it is easier to simply remove an entity from a table when it is not required that wait and set the flag.

1. renamed 2. the issue is that we have only one value in ENUM and it is hard to check against other values. I would like to avoid adding UNKNOWN or something like that. SMS is also incorrect becau...

1. renamed
2. the issue is that we have only one value in ENUM and it is hard to check against other values. I would like to avoid adding UNKNOWN or something like that. SMS is also incorrect because in near future we will add support for the channel.
3. Right now I would say it is impossible to have a null value for the header, the case is only in the test because there are no other values in enum class. Currently, we have only EMAIL channel and because of constraint on the database layer, it is hard to verify other path but because the code goes through the list to find a correct message for a channel there could be a situation where the notification will not exist but this is more related to the stream works. In the end, if the test is meaningless I could simply remove it right now.

added

added