Add reminder emails functionality #279
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #279 +/- ##
============================================
+ Coverage 79.94% 80.00% +0.05%
- Complexity 1006 1020 +14
============================================
Files 136 136
Lines 3017 3095 +78
Branches 163 170 +7
============================================
+ Hits 2412 2476 +64
- Misses 505 516 +11
- Partials 100 103 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
src/main/java/eu/bbmri_eric/negotiator/service/UserNotificationServiceImpl.java
Outdated
Show resolved
Hide resolved
jungwire
force-pushed
the
feat/reminder-emails
branch
from
fe0c9ed
to
4dff818
Compare
|
|
||
| private static LocalDateTime thresholdTime; | ||
|
|
||
| private static Collection<NegotiationResourceState> activeNegotiationResourceStates = |
There was a problem hiding this comment.
why not do it the other way? nonActiveResource states are just Unavailable and Made available
|
|
||
| @Value("${negotiator.frontend-url}") | ||
| private String frontendUrl; | ||
|
|
||
| @Value("${reminder.trigger-duration-days:P14D}") |
There was a problem hiding this comment.
thinking about it now, change to 7 days
| @@ -51,6 +75,29 @@ private static Set<Person> getRepresentativesForNegotiation(Negotiation negotiat | |||
| .collect(Collectors.toSet()); | |||
| } | |||
|
|
|||
| private static Set<Person> getRepresentativesForStaleNegotiation(Negotiation negotiation) { | |||
| Set<Person> representatives = | |||
There was a problem hiding this comment.
first you should just get resources involved in the Negotiation that did not have a new record in the last week/2 week. Then simply get their reps
|
|
||
| @Test | ||
| @WithMockNegotiatorUser(id = 109L, roles = "ADMIN") | ||
| public void testSendReminderEmails() { |
There was a problem hiding this comment.
Not sure one test is enough for this. You should check multiple scenarios: for example set trigger duration to 0ms and then check if correct notifications were made
| @@ -31,6 +31,10 @@ public class NegotiationResourceLifecycleRecord extends AuditEntity { | |||
| @JoinColumn(name = "resource_id", referencedColumnName = "id") | |||
| private Resource resource; | |||
|
|
|||
| @ManyToOne | |||
There was a problem hiding this comment.
not sure this is needed. Just go through the parent entity, if this is needed you need a migration script no?
jungwire
force-pushed
the
feat/reminder-emails
branch
from
ce7ce5e
to
171e2fc
Compare
|
technically I think it is implemented correctly, @konradlang do you think it matches the desired functionality? |
Negotiator pull request:
Description:
Introduces some changes to the notification email service by adding a functionality to send reminder emails for pending (negotiations that are under review) and stale (negotiations without ) to admins and representatives, respectively. The goal is to increase the responsiveness of biobanks/admins.
Checklist:
Make sure you tick all the boxes bellow if they are true or do not apply: