Task #2541
closed
LRT CheckExpiredOrMissingManager
Added by Roman Kubica about 4 years ago. Updated about 4 years ago.
100%
Description
- identity that has assigned manager with expired contract (manager is already expired)
- identity that has no manager assigned
- identity that has assigned manager and manager's contract is expiring in X days.
The final email notification will include information about identities specified in scenarios (LRT options / checkbox)
Files
| table.png (65.2 KB) table.png | Roman Kučera, 11/19/2020 09:32 AM | ||
| table_update.png (61.1 KB) table_update.png | Roman Kučera, 11/25/2020 07:09 AM | ||
| multiple_guarantees_table.png (64.6 KB) multiple_guarantees_table.png | Roman Kučera, 11/25/2020 07:10 AM |
Updated by Roman Kubica about 4 years ago
- % Done changed from 0 to 80
Added LRT option for selecting TreeNode. This will search only identities with this workPosition. If option excluded, it will search for all identities.
Updated by Roman Kubica about 4 years ago
I am linking my branch for the new LRT: https://github.com/bcvsolutions/czechidm-extras/tree/rkubica/LRT-checkExpiredOrMisssingManager
Updated by Tomáš Doischer about 4 years ago
Please can you make it possible that if you select a tree node, the task will check users who are not only on the selected tree node but also on any tree node below it (maybe as a new option)?
Maybe it works already but I'm not sure and it is important.
Updated by Roman Kubica about 4 years ago
- % Done changed from 80 to 90
@doischert treenode search is down/below recursive by default.
Added documentation: https://wiki.czechidm.com/tutorial/adm/notification_-_expired_managers
Updated by Roman Kučera about 4 years ago
- code style is different, in some place you have space before and after =,{,} etc. sometimes not. I'll suggest to use some auto format in IDE to be consistent.
- Once you use if without {} once with for one command. I'll suggest to use it everytime
- CheckExpiredOrMissingManagerTask::141 get content from Page and use simple foreach would be simpler and more readable I would say. Didn't see any benefit for using iterator and "classic" for loop
- CheckExpiredOrMissingManagerTask::167 managers can't be null at this place
- CheckExpiredOrMissingManagerTask::230-260 Just check if some contract from list will expire in X days if so add manager to Map. Second option to get data from DB.
- CheckExpiredOrMissingManagerTask::230-260 Key in map managersExpiritingXDays is based on indentity so if user has more managers you will not show all of them
- CheckExpiredOrMissingManagerTask::279 insted of recipientEmail==null || recipientEmail.trim().isEmpty() use StringUtils.isBlank() that should do exactly what you want.
- Insted of new method getUsersByRoleId just use identityService.findAllByRole(role) that's same as far as I checked your code
Next I'll test it
Updated by Roman Kučera about 4 years ago
- It would be nice to show the date even for already expired guarantees, now it is displayed only for guarantees who will expire in X days
- For guarantees who will expire soon there is different spacing? see image
- When you run report for example only for already expired guarantees, you will still see all tables in notification only with info "no users found" this can be misleading. If the report will be send to some other user who doesn't know how LRT was configured he can be confused and he can think that everything is ok. I would suggest to hide the specific block in notification if the report is not running for that use case. Or at least replace the massage no users for this case with something like "Report did not check this option".
I test various use cases and options and didn't found any other issues then the one which are already mention in BE review.
Updated by Roman Kubica about 4 years ago
- https://github.com/bcvsolutions/czechidm-extras/commit/e3013efa28814dc2a373376c65f47fd4a35fa70b#diff-eaae27a4c7e447bc9a06bdbb31a83c5969771ebfbb33cbb9ceaf2c56845849ee
- https://github.com/bcvsolutions/czechidm-extras/commit/c9f10b24ed4ca87998421a9e1d1bbd58ffc89ccf#diff-eaae27a4c7e447bc9a06bdbb31a83c5969771ebfbb33cbb9ceaf2c56845849ee
Updated by Roman Kučera about 4 years ago
- File table_update.png table_update.png added
- File multiple_guarantees_table.png multiple_guarantees_table.png added
Thx for fixes. Code looks good to me.I tested it again and the style of notification still behave strange, see image attached. "Garant" row is not spaced evenly for all tables. And if you have multiple guarantees the spacing is even weirder(second image).
The point from my previous testing
When you run report for example only for already expired guarantees, you will still see all tables in notification only with info "no users found" this can be misleading. If the report will be send to some other user who doesn't know how LRT was configured he can be confused and he can think that everything is ok. I would suggest to hide the specific block in notification if the report is not running for that use case. Or at least replace the massage no users for this case with something like "Report did not check this option".
is still there but it's not big issue so if just mention this in documentation or if this behavior is not issue for the project where you will use it, it's ok by me.
Sorry I forgot to update template manually. Template is nice now :)
Updated by Roman Kubica about 4 years ago
- Assignee changed from Roman Kubica to Peter Štrunc
Thank you Roman for code review.
Hi Peter, can we please include it in version 2.7.0?
Updated by Peter Štrunc about 4 years ago
- Status changed from In Progress to Closed
- % Done changed from 90 to 100
Merged to develop, thanks for the feature.