Project

General

Profile

Actions
Copy link

Task #2541

closed

LRT CheckExpiredOrMissingManager

Added by Roman Kubica about 4 years ago. Updated about 4 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Peter Štrunc
Target version:
2.7.0
Start date:
10/29/2020
Due date:
% Done:

100%

Estimated time:
Owner:

Description

Long Running Task (LRT) will send email to contacts specified by role or address about identities, who are affected with these scenarios.
  • 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

Download all 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
Actions
Copy link
 #1

Updated by Roman Kubica about 4 years ago

  • Description updated (diff)
Actions
Copy link
 #2

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.

Actions
Copy link
 #3

Updated by Roman Kubica about 4 years ago

Actions
Copy link
 #4

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.

Actions
Copy link
 #5

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

Actions
Copy link
 #6

Updated by Roman Kučera about 4 years ago

Code review of BE:
  • 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

Actions
Copy link
 #7

Updated by Roman Kučera about 4 years ago

I'll test it, and I have some points:
  • 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.

Actions
Copy link
 #9

Updated by Roman Kučera about 4 years ago

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 :)

Actions
Copy link
 #10

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?

Actions
Copy link
 #11

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.

Actions
Copy link

Also available in: Atom PDF