Łukasz Lewczyński

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

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?

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 DigestConfiguration entity
OLMIS-6016: Added DigestConfiguration entity
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

renamed

renamed

delegate the work to JpaExcetutor. When an entity is retrieved it is also removed from the table

delegate the work to JpaExcetutor. When an entity is retrieved it is also removed from the table

added check and loggers

added check and loggers

I modified the way how we retrieve entities by delegate the work to JpaExecutor class from Spring Integration library. From now, when an entity will be retrieved from the database it will be also r...

I modified the way how we retrieve entities by delegate the work to JpaExecutor class from Spring Integration library. From now, when an entity will be retrieved from the database it will be also removed from it so there is only one chance to send a notification to a user. In the future, we could ability to resend notifications but this is out of the scope of this review (and ticket).

I used other solution to know which notification should be sent. Instead of setting a flag (and have issues related to that thing) I create a new entity (PendingNotification) which informs what not...

I used other solution to know which notification should be sent. Instead of setting a flag (and have issues related to that thing) I create a new entity (PendingNotification) which informs what notification by using what channel should be sent.

renamed

renamed

changes have been reverted

changes have been reverted

removed

removed

Definitely no, I will remove this in the next commit

Definitely no, I will remove this in the next commit