IdStory IdM modules » Business card

Documentation shoulb be finished.
Started with tests

test finnished.
Did some code cleanup to. Removed unused code.

Test coverage for whole module in ideas is 81%.

Next step is to put it on nexus and sonar. #2396

It looks great, I found no issues with the code, just a minor detail:

• FOPProcessor:66 - Double Brace Initialization is not often used and can lead to memory leaks (though that seems unlikely here)

I also tested it a little bit and have some suggestions:

• The tree node EAV businessCardName is required - this should at least be mentioned in the documentation (it also has to be of the type TEXT, not SHORTTEXT).
• If you remove the date when creating a card by clicking the cross button next to selection, a part of the form just reloads until you select a date. Maybe mark the date as required?
• The button "Editovat oddělení pro všechny" is a nice touch but the title is confusing. Maybe something like "Přejít na detail oddělení" would be better?

In the future, if someone else wants to use this module, it would be nice to make it more easily configurable. I you wanted different attributes, you have to override getFormInstance. Using a form definition which can be edited in GUI seems better. (I didn't look too much into it but it seems possible.) For more significant changes, you would probably still have to overried the getFormInstance method.

Also for the future, some default ready-to-use templates would be nice.

Anyway, it looks great, thanks!

Thx for review. I fixed it and below are some comments.

Tomáš Doischer wrote:

It looks great, I found no issues with the code, just a minor detail:

• FOPProcessor:66 - Double Brace Initialization is not often used and can lead to memory leaks (though that seems unlikely here) fixed

I also tested it a little bit and have some suggestions:

• The tree node EAV businessCardName is required - this should at least be mentioned in the documentation (it also has to be of the type TEXT, not SHORTTEXT). Added more info into documentation under configuration section
• If you remove the date when creating a card by clicking the cross button next to selection, a part of the form just reloads until you select a date. Maybe mark the date as required? only required will not fix this issue. It's fixed in the way that if you clear date picker the selectbox with contracts turn into readonly and all other attributes are hidden. when you select some date, attributes will appear again. So user will notice that he does not pick any date.
• The button "Editovat oddělení pro všechny" is a nice touch but the title is confusing. Maybe something like "Přejít na detail oddělení" would be better? the label should tell the user that if he want to change the department text for all users he need to edit it in diffrent place. I am afraid that if I put label "Přejít na detail oddělení" it wouldn't be understand correctly what is does. So maybe use something like "Upravit oddělení pro všechny uživatele" ?

In the future, if someone else wants to use this module, it would be nice to make it more easily configurable. I you wanted different attributes, you have to override getFormInstance. Using a form definition which can be edited in GUI seems better. (I didn't look too much into it but it seems possible.) For more significant changes, you would probably still have to overried the getFormInstance method. this is good point and that was the original plan, but because basically all attributes are filled with some value you still need to override the method to tell which values will be filled into which field. At that point I didn't see many benefits to configure attributes in FE.

Also for the future, some default ready-to-use templates would be nice. There is one template, it's shown in documentation, or you mean something different by this?

Anyway, it looks great, thanks!

All fixes in commit https://github.com/bcvsolutions/czechidm-business-card/commit/c50ed526f216b0540aebec5733718cbf537505e5

Together with it I also fixed one small bug which was found during testing with more restrict permissions. Bulk action required ADMIN permission for IdmIdentity but only read is required. So now this is fixed to.

Thank you for the changes, all looks good.

• The solution with date picking is great!
• I understand the issue with localization, you can keep it as is. I don't really have a good alternative, maybe just adding a 'help' explaining what the user is about to do? I don't know myself, you can keep it like this.
• As for the configuration, this was not really a part of code-review but rather a suggestion for the future when this module is used by other clients. But I imagine you could use attribute codes to identify attributes in form definitions and thus make the form configurable from FE. This is not needed now, I just couldn't help myself.