Dashboard

OLMIS-6031: added performance tests for supply lines
OLMIS-6031: added performance tests for supply lines
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

Łukasz Lewczyński, thanks for sending this. I have two main comments: 1. The size of this review is too large and unfocused. It's difficult to grasp what is supposed to be evident and what's a WI...

Łukasz Lewczyński, thanks for sending this.

I have two main comments:

1. The size of this review is too large and unfocused. It's difficult to grasp what is supposed to be evident and what's a WIP. Next time try much smaller reviews chained together that build upon one another. Less than a dozen files, no more than 1k LOC.

2. It's not really clear what's happening in the service layer with regards to what should be a filter, what's a router, and where we'd see an aggregator. This is likely a continuation of the above.

Could you clean this up?

Is this class a placeholder and a WIP? I'm confused how this filter supports a digest operation? Wouldn't we want an aggregator over a filter anyhow?

Is this class a placeholder and a WIP? I'm confused how this filter supports a digest operation? Wouldn't we want an aggregator over a filter anyhow?

I am curious about the decision for some log statements to be errors? Some of them seem like warnings to me. Like in NotificationTransformer or NotificationChannelRouter.

I am curious about the decision for some log statements to be errors? Some of them seem like warnings to me. Like in NotificationTransformer or NotificationChannelRouter.

We may have SMS support in the future, but we can still test that nothing is returned if an SMS channel is specified and there is no SMS message in the notification.

We may have SMS support in the future, but we can still test that nothing is returned if an SMS channel is specified and there is no SMS message in the notification.

I think you mean cannot extract notification message, as this component does not send a notification.

I think you mean cannot extract notification message, as this component does not send a notification.

I meant setting a variable in the given section of each test, as opposed to creating constants.

I meant setting a variable in the given section of each test, as opposed to creating constants.

OLMIS-5531 added functional test for requisition workflow validations
OLMIS-5531 added functional test for requisition workflow validations
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-5989: Updated info about building documentation
OLMIS-5989: Updated info about building documentation
I was added intentionally because it can be moved to another directory or updated and the link would be invalid but I can change it to master if you really want.

I was added intentionally because it can be moved to another directory or updated and the link would be invalid but I can change it to master if you really want.

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?

Worth linking to the Object Reference Expander as it does all of the job and not all the services have it implemented yet.

Worth linking to the Object Reference Expander as it does all of the job and not all the services have it implemented yet.