OBPIH-5984 Fix lazyInitialization issue when building approval emails

[drodzewicz]

In the end, the issue was with LazyInitializationException which occurred when building the approver emails.
The interesting part was that it didn't occur every time and at first, I had trouble reproducing this bug locally.

To be more specific there is a case where we access some values like requisitions.events inside of the GSP templates and extract status and comment from it, and that's the place where the lazyInitializationException is being thrown.
To fix this issue I decided to just fetch the Requisition again in the event handler instead of relying on the requisition passed in the RequisitionStatusTransitionEvent.

Wrapping the method with Requisition.withSession didn't help btw.

Requisition.withSession {
        if (requisition.shouldSendApprovalNotification()) {
            notificationService.publishRequisitionStatusTransitionNotifications(requisition)
        }
}

[kchelstowski]

My conscience wouldn't allow me to leave this without a comment even though we agreed on a similar solution somewhere, so:

1. If the problem with the LazyInitializationException happens here, it should be enough to add @Transactional annotation for this service
2. If it happens in the view (which I assume happens), I'd prefer to create a data service method that would eagerly fetch the events using @Join annotation instead of fetching the requisition twice.

[drodzewicz]

yes, I agree a comment should be added.
By the way this is grails 1 so @Join services are probably not available to us here

[kchelstowski]

@drodzewicz

yes, I agree a comment should be added.

I didn't mean to add a comment in the code, I just meant that my conscience wouldn't allow me to leave this PR without the comment 😆

By the way this is grails 1 so @Join services are probably not available to us here

Oh, is it?..... then I apologize, but I'd consider adding a TODO to change it to the data service method I proposed in Grails 3, if everyone agrees on my preposition

[alannadolny]

I think we should add the TODO for Grails 3

[jmiranda]

I think we should always pass the ID rather than the object to these events (see RefreshProductAvailabilityEvent) because there's no guarantee the event handler will be executed synchronously. We should treat them similar to Quartz jobs.

[kchelstowski]

PS. I'm fine if you all agree on pushing this solution through, just wanted to make sure I mention my personal opinion on that.

[jmiranda]

I think we should always pass the ID rather than the object to these events (see RefreshProductAvailabilityEvent) because there's no guarantee the event handler will be executed synchronously. We should treat them similar to Quartz jobs.

[jmiranda]

Great job on the investigation, @drodzewicz.

[awalkowiak]

As agreed on the standup this one can be merged