Task #2993
closed
LRT notification for new contracts/accounts
100%
Description
It's similar task to LastContractEndNotificationTask but only for creation
LRT params:- Number of days before contract is valid 0=now, -X=afte valid contract X=before valid contract
- Recipient, by role(selectbox), manager(checkbox), directly to the user(checkbox)
- name of topic
- Select box with systems - if no system is selected all accounts will be shown
- checkbox if you want only contracts - no accounts, only contracts will be shown
- Type of user - form projection
params for notification same as in LastContractEndNotificationTask
Updated by Roman Kučera about 3 years ago
- % Done changed from 0 to 60
implementation https://github.com/bcvsolutions/czechidm-extras/tree/2993-lrt-contract-start-notif
implemented all params for lrt and for template
did some basic testing
TODO
unit test
more testing
doc
Updated by Roman Kučera about 3 years ago
- % Done changed from 60 to 80
test, docs and testing is done
https://wiki.czechidm.com/tutorial/adm/notification_-_identity_s_contract_is_starting
Updated by Roman Kučera about 3 years ago
- Status changed from In Progress to Needs feedback
- Assignee changed from Roman Kučera to Tomáš Doischer
@doischert Could you please do review of this?
https://github.com/bcvsolutions/czechidm-extras/compare/2993-lrt-contract-start-notif?expand=1
Updated by Tomáš Doischer about 3 years ago
- Status changed from Needs feedback to In Progress
- Assignee changed from Tomáš Doischer to Roman Kučera
Thank you for this feature. This review is also for #2612.
I really like the use of negative numbers in configuration!
- are you sure removing the contract from the queue is desirable in ContractStartNotificationTask? I know that validFrom of contract can potentially change but is it really necessary to send the notification again if that happens? It is needed in LastContractEndNotificationTask but it does have performance impact.
- in both tasks, I would expect that tree node selection is used recursively, i. e., I will see users with contract on selected position and on those below it
- unused imports in ContractStartNotificationTask and LastContractEndNotificationTask
- Sonar has some issues (using boolean instead of Boolean)
- null pointer on line ContractStartNotificationTask:290 - validFrom can be null when called from getContractItemFromQueue()
Otherwise it looks great, I tested a few basic UCs and it works well.
Updated by Roman Kučera about 3 years ago
Tomáš Doischer wrote in #note-5:
- are you sure removing the contract from the queue is desirable in ContractStartNotificationTask? I know that validFrom of contract can potentially change but is it really necessary to send the notification again if that happens? It is needed in LastContractEndNotificationTask but it does have performance impact.
The removing was implemented for use case if user will start working in company (notification is send) then he leave the company and he starts working there again. So we want to send the notification again. But when I am thinking about it now, I am not sure if he will have still the same contract. Because if if get a new contract then notification will be send anyway.
- in both tasks, I would expect that tree node selection is used recursively, i. e., I will see users with contract on selected position and on those below it
Yeah I thought that to. I will confirm this behavior with customer, or maybe some additional switch would be nice to choose if you want recursion or no.
- unused imports in ContractStartNotificationTask and LastContractEndNotificationTask
- Sonar has some issues (using boolean instead of Boolean)
- null pointer on line ContractStartNotificationTask:290 - validFrom can be null when called from getContractItemFromQueue()
I'll fix it thx.
Updated by Roman Kučera about 3 years ago
Unused imports, sonar and null pointer is already fixed.
Updated by Tomáš Doischer about 3 years ago
Fixes look great, thanks.
The other points I will leave up to you to decide whether to implement them or not.
Updated by Roman Kučera about 3 years ago
@doischert Could you please look at this last commit https://github.com/bcvsolutions/czechidm-extras/commit/cae885a7de060d8acfb660ea44ab1f4d2c4ad4e2 ?
I added down recursion by default, so the behavior by tree node will be similar as in other LRT.
I also changed the name of two template properties to correct names.
Updated by Roman Kučera about 3 years ago
- Status changed from In Progress to Closed
- Target version set to 3.3.0
- % Done changed from 80 to 100