IdStory IdM modules » Property

I did test and code review. Module basically works, it's really complex and code has nice structure (evaluators, processors etc.), good job! I found one critical issue, which is related to design itself - without it module will not work properly from my point of view (can be reproduced with one automatic role for 15 users => 5 properties are assigned only), see notes bellow.

Review notes (!! critical, ! major, minor):
-- !! Architecture: Synchronized blocks or an asynchronous property requests processing (for add / remove) has to be added. Role request are executed asynchronously simultaneously => when more role requests is processed in the same time (e.g. when new automatic role is created for more users), when two role requests override the same property => it leeds to inconsistency, when e.g. only 5 properties are asigned, but 16 users has automatic role properly assigned.
-- ! BE: copy-fe-sources plugin missing in pom.xml (https://wiki.czechidm.com/priv/vlastnik_modulu#doporucene_nastaveni_modulu).
-- BE: organization and developer missing in pom.xml.
-- BE: javadoc, athor is missing (DefaultPrpPropertyService).
-- BE: Assert.notNull or check null pointers is missing in public service methods (e.g. DefaultPrpPropertyService#canPropertyFromSetBeAssigned).
-- ! FE: SetGuarantorInfo#getLink - undefined entityId variable is used (same in SetGuarantorRoleInfo)
-- FE: a lot of warnings is on FE sources - formating, duplicate component property usage, long rows, undefined variables usage (same above) etc.
-- BE: notification templates are not valid by xsd schema.
-- FE: set detail has different icon in header than menu (target system icon) - the same for set items on set detail etc.
-- ! FE: set detail - 404 is shown, when close detail button is used (url /prp/set => /prp/sets). The same for property detail. The same, when detail (both set and property too) is saved and close by split button.
-- ! FE: Set table - filter by text doesn't work. The same for Property table + unassigned filter doesn't work (BE: textPredicates are not set into result predicates).
-- FE: Backend bulk action could be provided. Frontend bulk action usage is obsolete for newly added agenda (will be removed in future).
-- FE: Bulk actions could be hidden on identity detail - assigned property cannot be removed anyway (and other bulk action are not provided now).
-- FE: Unassigned filter can be hidden on identity detail (assigned user is jidden already).
-- ! FE: Localization is missing for success (and error) messages, after more information (eav) is saved (in prp:content.property.eav section).
-- ! FE: Localization is missing for success message, when property guarantee (both) are saved.
-- ! FE: Localization parameter is missing for success message, when property guarantee (both) are deleted.
-- FE: Assigned user can be shown on property item detail. Assigned user is shown in table - popover face can be used in info components for user and set.
-- FE: Assigned users can be shown on set detail in new tab - this can be used even for show "waiting" users. Information can be found in notification now, but this will provide summary for set guarantee.
-- ! BE: Private modifiers is missing for fields in PrpPropertyController, PrpSetController constructor etc.
-- FE / BE: Url for module can be unified under module prefix e.g. /set-roles => /prp/set-roles or /prp-sets => /prp/sets.
-- ! BE: Message is missing in logs for all result codes (e.g. when assigned property is deleted, then code only is set without additional message, what happened). See CoreResultCode.
-- BE: Url to set detail can be added to notification, when property id not available.
-- BE: PrpInitModuleProcessor can be removed - is provided by archetype, but is covered by other InitModuleProcessor (for provide default roles).
-- ! BE: Referential integrity is missing, when role is deleted, then set roles and set guarantees are broken (add cascade delete or reuse force property @since 11.1.0).
-- ! BE: Referential integrity is missing, when identity is deleted, then set guarantees are broken (add cascade delete or reuse force property @since 11.1.0).
-- ! FE: cosole.log is forgotten on FE (e.g. PropertyTable)
-- BE: Camel case should be used (ImportPropertyFromCSVExecutor etc.)
-- ! BE: Security: when Property set guarantor (setGuarantorRole) role is assigned - server error occurs on set detail (java.lang.IllegalArgumentException: Parameter value [fac81c33-b074-4245-a606-4abc9be55380] did not match expected type [eu.bcvsolutions.idm.core.model.entity.IdmIdentity (n/a)])
-- ! FE: Security: link to set detail is shown for user without set read permission (can be solved by popover face)
-- BE: Upper case should be used for final fields ~ constants (e.g. ImportSetFromCSVExecutor)
-- FE: SetSelect readme - FormProjectionSelect is forgotten
-- ! FE: IdentityProperties#getNavigationKey - duplicate method (remove method with getRequestNavigationKey usage)
-- BE: I'm not sure about PrpGroupPermission, where SET_xxx persmission is used. Is maybe to general and can be abused in different custom module ... mayve some prefix can be added just for sure (e.g. PRPSET_xxx etc.).
-- ! BE: LookupService#lookupEmbeddedDto can be used to get related dtos an prevent to execute additional db select (e.g. IdentityRoleRemovePropertyProcessor#56)
-- ! BE: IdmRoleRequestService#startRequest should be used - asynchronous request should be started. Synchronous requests are used now (by IdmRoleRequestService#executeRequest) => provisioning queue order will be broken.
-- ! BE / FE: disabled property cannot be created - disable flag is removed on background and user is not informed about it - disable field on FE, when property is created or remove it from BE.
-- ! BE: "superAdminRole" is hardcoded in PrpUtils - use RoleConfiguration if needed instead
-- ! BE: PrpUtils#getRecipientsForSetNotification can return all identities, if e.g. userRole is configured - use pageable property and limit recipients e.g. to 100. The same with DefaultPrpSetGuarantorRoleService#findGuarantorRoleIdentitiesBySet.
-- BE: I don't like PrpUtils at all - each method can be moved to concrete service (e.q. getRecipientsForSetNotification => setService, getSubordinateRoleIds => not needed at all ~ simple .stream + .map, etc.)
-- ! BE: xxxService.deleteInternal should be used just on one place - in delete xxxDeleteProcessor - for delete the one concrete entity. Use xxxService.delete (mainly in SetDeleteProcessor). Add force delete property to skip check assigned property if needed.
-- ! BE: toFilter method has to be implemented in all Controllers (=> DataFilter is used => properties will not be set properly by default)
-- ! BE: null pointers are not checked in both import LRTs. When identity, role or set is not found, then import should end with result code exception. And LookupService could be used instead to support codeable identifiers (uuid or code).
-- BE: Both imports could be executed in transaction - when import fails, then skeleton records are created now (e.g. set without guarantees).
-- ! BE: @Transactional annotation is missing for methods, which changes data (e.g. DefaultPrpPropertyService#generateProperties)
-- ! BE: rest / service layer is mixed - e.g. DefaultPrpPropertyService#getFormDefinitionResponseEntityForProperty should be in controller.
-- ! BE: PrpPropertyService methods with String propertyId shold be refactored to UUID usage and security is missing here - e.g. PrpPropertyService#getFormDefinitionResponseEntityForProperty is called directly from controller without permission is evaluated.
-- BE: DefaultPrpPropertyService#getAvailablePropertyFromSet load 100 records and returns first.
-- BE: DefaultPrpSetGuarantorService#findGuarantorIdentitiesBySet - use lookupService for dege embedded dto to prevent a lot of unnecessary selects in cycle. Use this pattern on all places (DefaultPrpSetRoleService#findRolesAssigningSet etc.)
-- ! BE: PrpSetService#getSetIdByCode - remove method - use lookupservcei instead (~ reuse getByCode from CodeableService API)
-- ! FE: Middle click on propery main menu - 404 is shown.

Thank you very much for the feedback.

I fixed all the major and some of the minor issues:

-- ! BE: copy-fe-sources plugin missing in pom.xml (https://wiki.czechidm.com/priv/vlastnik_modulu#doporucene_nastaveni_modulu).
-- BE: organization and developer missing in pom.xml.
-- BE: javadoc, athor is missing (DefaultPrpPropertyService).
-- ! FE: SetGuarantorInfo#getLink - undefined entityId variable is used (same in SetGuarantorRoleInfo)
-- BE: notification templates are not valid by xsd schema.
-- ! FE: set detail - 404 is shown, when close detail button is used (url /prp/set => /prp/sets). The same for property detail. The same, when detail (both set and property too) is saved and close by split button.
-- ! FE: Set table - filter by text doesn't work. The same for Property table + unassigned filter doesn't work (BE: textPredicates are not set into result predicates).
-- ! FE: Localization is missing for success (and error) messages, after more information (eav) is saved (in prp:content.property.eav section).
-- ! FE: Localization is missing for success message, when property guarantee (both) are saved.
-- ! FE: Localization parameter is missing for success message, when property guarantee (both) are deleted.
-- ! BE: Private modifiers is missing for fields in PrpPropertyController, PrpSetController constructor etc.
-- ! BE: Message is missing in logs for all result codes (e.g. when assigned property is deleted, then code only is set without additional message, what happened). See CoreResultCode.
-- BE: PrpInitModuleProcessor can be removed - is provided by archetype, but is covered by other InitModuleProcessor (for provide default roles).
-- ! BE: Referential integrity is missing, when role is deleted, then set roles and set guarantees are broken (add cascade delete or reuse force property @since 11.1.0).
-- ! BE: Referential integrity is missing, when identity is deleted, then set guarantees are broken (add cascade delete or reuse force property @since 11.1.0).
-- ! FE: cosole.log is forgotten on FE (e.g. PropertyTable)
--! BE: Security: when Property set guarantor (setGuarantorRole) role is assigned - server error occurs on set detail (java.lang.IllegalArgumentException: Parameter value [fac81c33-b074-4245-a606-4abc9be55380] did not match expected type [eu.bcvsolutions.idm.core.model.entity.IdmIdentity (n/a)]) (not sure that it is fixed but I was not able to replicate the issue)
-- ! FE: Security: link to set detail is shown for user without set read permission (can be solved by popover face)
-- BE: Upper case should be used for final fields ~ constants (e.g. ImportSetFromCSVExecutor)
-- ! FE: IdentityProperties#getNavigationKey - duplicate method (remove method with getRequestNavigationKey usage)
-- ! BE: LookupService#lookupEmbeddedDto can be used to get related dtos an prevent to execute additional db select (e.g. IdentityRoleRemovePropertyProcessor#56)
-- ! BE: IdmRoleRequestService#startRequest should be used - asynchronous request should be started. Synchronous requests are used now (by IdmRoleRequestService#executeRequest) => provisioning queue order will be broken.
-- ! BE / FE: disabled property cannot be created - disable flag is removed on background and user is not informed about it - disable field on FE, when property is created or remove it from BE.
-- ! BE: "superAdminRole" is hardcoded in PrpUtils - use RoleConfiguration if needed instead
-- ! BE: PrpUtils#getRecipientsForSetNotification can return all identities, if e.g. userRole is configured - use pageable property and limit recipients e.g. to 100. The same with DefaultPrpSetGuarantorRoleService#findGuarantorRoleIdentitiesBySet.
-- ! BE: xxxService.deleteInternal should be used just on one place - in delete xxxDeleteProcessor - for delete the one concrete entity. Use xxxService.delete (mainly in SetDeleteProcessor). Add force delete property to skip check assigned property if needed.
-- ! BE: toFilter method has to be implemented in all Controllers (=> DataFilter is used => properties will not be set properly by default)
-- ! BE: null pointers are not checked in both import LRTs. When identity, role or set is not found, then import should end with result code exception. And LookupService could be used instead to support codeable identifiers (uuid or code).
-- ! BE: @Transactional annotation is missing for methods, which changes data (e.g. DefaultPrpPropertyService#generateProperties)
-- ! BE: rest / service layer is mixed - e.g. DefaultPrpPropertyService#getFormDefinitionResponseEntityForProperty should be in controller.
-- ! BE: PrpPropertyService methods with String propertyId shold be refactored to UUID usage and security is missing here - e.g. PrpPropertyService#getFormDefinitionResponseEntityForProperty is called directly from controller without permission is evaluated.
-- BE: DefaultPrpPropertyService#getAvailablePropertyFromSet load 100 records and returns first.
-- ! BE: PrpSetService#getSetIdByCode - remove method - use lookupservcei instead (~ reuse getByCode from CodeableService API)
-- ! FE: Middle click on propery main menu - 404 is shown

Commit: https://git.bcvsolutions.eu/modules/property/-/commit/79816b8e5131d3e22dcacaa97d0161c732d604ad

Can you take a look please?

I did test and code review again, a lot of notes were fixed, issue with architecture mainly, good job.

Review notes (!! critical, ! major, minor):
-- !! BE: DefaultPrpPropertyService#getAvailablePropertyFromSet - pageable should be removed - cache is used now for filter properties and null property could be falsely returned - avaliable property is not returned when e.g. 1000 user is in automatic role => 80 properties are correctly assigned on my environment only.
-- !! BE:newly added procesors for ensuring referential integrity didn't work. Exception is thrown, when identity is deleted. Processors should have order less than zero => clean up related entities before identity or role is deleted.
-- !! BE: EXECUTED#assignRole - exception is thrown - role request with state "EXECUTED" cannot be started => unassigned property cannot be assigned, when property is added.
-- !! FE: property cannot be created - blank page is shown (!_.includes(columns, 'set') should not be used on detail)
-- ! IdmBasePermission.READ is hardcoded in DefaultPrpPropertyService#getFormDefinitionForProperty - permission shold be given as parameter (see other services method) and given from controller only (~ permissions will not be evaluated from service layer => will be quicker)
-- BE: toFilter method - Data filter set singular properties automatically (e.g. filter.setSerialNumber(getParameterConverter().toString(parameters, PrpPropertyFilter.PARAMETER_SERIAL_NUMBER));
can be removed). Codeable properties should remain only (e.g. PrpPropertyFilter.PARAMETER_SET_ID).
-- ! BE: Security is still missing on controller layer - e.g. PrpPropertyController#generateProperties - use propertyService.get(id, persminsions) or check permission to create after super.getDto(id) (this is better, because codeable support is solved in super class). The same for getFormDefinitionResponseEntityForProperty method - permission parameter (same as above) could be added to service or super.getDto(id) can be used. Please check all methods used in controlloer to evaluate permissions.
-- BE: DefaultPrpSetService#getRecipientsForSetNotification - return 10 recipent even underlying service find 100 recipient.
-- FE: PropertTable - info component could be used for assigned identity
-- FE: Set table - filter by unassigned works now, nice (I'm not sure about double negation "Unassigned - No" will lead to user confusion, but we can discuss it later :))
-- ! FE: Localization parameter is missing for success message (and confirm dialog), when property guarantee (both) are deleted.
-- ! BE: Private modifiers is missing for fields in DefaultPrpPropertyService etc.
-- ! BE: Security: when Property set guarantor (setGuarantorRole) role is assigned - server error occurs on set detail (java.lang.IllegalArgumentException: Parameter value [fac81c33-b074-4245-a606-4abc9be55380] did not match expected type [eu.bcvsolutions.idm.core.model.entity.IdmIdentity (n/a)]) - issue is in DefaultPrpSetGuarantorService#67 where wrong predicate is defined - root.get(PrpSetGuarantor_.guarantor).get(id) is missing.
-- BE: LookupService#lookupEmbeddedDto can be used to get related dtos an prevent to execute additional db select (e.g. DefaultPrpPropertyService#getFormDefinitionForProperty)
-- ! BE: "superAdminRole" (by RoleConfiguration#DEFAULT_ADMIN_ROLE) is hardcoded in DefaultPrpSetService- use RoleConfiguration#getAdminRoleId or getAdminRoleCode instead
-- FE: showLoading is missing on generate more properties modal window - when e.g. 1000 properties is generated, user don't know about processing

Thank you for the review, consultation and fixes. I fixed all the critical and major issues:

-- !! BE: DefaultPrpPropertyService#getAvailablePropertyFromSet - pageable should be removed - cache is used now for filter properties and null property could be falsely returned - avaliable property is not returned when e.g. 1000 user is in automatic role => 80 properties are correctly assigned on my environment only.
-- !! BE:newly added procesors for ensuring referential integrity didn't work. Exception is thrown, when identity is deleted. Processors should have order less than zero => clean up related entities before identity or role is deleted.
-- !! BE: EXECUTED#assignRole - exception is thrown - role request with state "EXECUTED" cannot be started => unassigned property cannot be assigned, when property is added. (Only role request with CONCEPT or EXCEPTION or DUPLICATED state can be started!)
-- !! FE: property cannot be created - blank page is shown (!_.includes(columns, 'set') should not be used on detail)
-- ! IdmBasePermission.READ is hardcoded in DefaultPrpPropertyService#getFormDefinitionForProperty - permission shold be given as parameter (see other services method) and given from controller only (~ permissions will not be evaluated from service layer => will be quicker)
-- ! BE: Security is still missing on controller layer - e.g. PrpPropertyController#generateProperties - use propertyService.get(id, persminsions) or check permission to create after super.getDto(id) (this is better, because codeable support is solved in super class). The same for getFormDefinitionResponseEntityForProperty method - permission parameter (same as above) could be added to service or super.getDto(id) can be used. Please check all methods used in controlloer to evaluate permissions.
-- FE: PropertTable - info component could be used for assigned identity
-- ! FE: Localization parameter is missing for success message (and confirm dialog), when property guarantee (both) are deleted. (you fixed it, thank you!)
-- ! BE: Private modifiers is missing for fields in DefaultPrpPropertyService etc.
-- ! BE: Security: when Property set guarantor (setGuarantorRole) role is assigned - server error occurs on set detail (java.lang.IllegalArgumentException: Parameter value [fac81c33-b074-4245-a606-4abc9be55380] did not match expected type [eu.bcvsolutions.idm.core.model.entity.IdmIdentity (n/a)]) - issue is in DefaultPrpSetGuarantorService#67 where wrong predicate is defined - root.get(PrpSetGuarantor_.guarantor).get(id) is missing.
-- ! BE: "superAdminRole" (by RoleConfiguration#DEFAULT_ADMIN_ROLE) is hardcoded in DefaultPrpSetService- use RoleConfiguration#getAdminRoleId or getAdminRoleCode instead

Commit: https://git.bcvsolutions.eu/modules/property/-/commit/7a854679e09c6079519f2ed13146bd5a584b6afb

Can you take a look at it again please?

Assing and remove properties mechanism works smoothly now, awesome! All notes above were fixed, thx.

Review notes (minor only):
-- BE: role cannot be deleted, when role assign some sets and a property is already assigned - but referential integrity is not broken => can be solved in future with force delete usage.
-- BE: when available property id not found, then notification is send - when a lot of properties is missing, then a lot of notification is sent. Notifications can be reduced (~ merged into one) in future.
-- FE: As i wrote before, showLoading is missing on generate more properties modal window - it's more dangerous, when unassigned properties exists and they are processed together with creating new property - user can click on generate again and again. I implemented showLoading for sure, commit: https://git.bcvsolutions.eu/modules/property/-/commit/a6803450f61851d3a26c87fe0439f0d1d3fe0647

Module can be released, it work properly, good job. All unimplemented minor notes can be moved into standalone tickets to next version.