IdStory IdM modules » extras

Ondřej Kopr wrote:

I merged develop into your branch, resolved conflicts and then push you branch to develop.

There is review:

• local and jenkins tests doesn't pass maybe some problem with reused identity
  

Solved by changing identity username.

• missing author

There was missing author in test file, so I completed it.

• missing localization for error messages

- Don't know how this works

• javadoc for some public method missing (not required)

Filled for method getOneValue() in ImportCSVUserContractRolesTaskExecutor

• get eav value for contract is called twice on same place (performance problem), ImportCSVUserContractRolesTaskExecutor#205 and ImportCSVUserContractRolesTaskExecutor#206

- Fixed but needs testing

• EAVs is get only from default definition is this problem?

- Not sure what this mean, but it should probably check all contracts

• some small typo error in log messages (taskCompleted("Asfsigned roles: " + ge),

I did file search in all projects but I can't find this typo.. probably created during review.

• add more tests, for example ImportCSVUserContractRolesTaskExecutor.java has only 0.9% coverage (tests didn't pass), your whole branch has 6.1% test coverage,

- Have to find how to display this coverage

• fix your bugs from sonar (for example issues with Booleans)

- I have to install Sonarqube and check how this works

• unused imports

- I can't see any unused imports in ImportCSVUserContractRolesTaskExecutor and ImportCSVUserContractRolesTaskExecutorTest, but maybe Eclipse doesn't show it right

• useless get query for contract ImportCSVUserContractRolesTaskExecutor#173 (performance problem), (identityContractService.get(contractId))

- Not sure how to do it, cause I can't log this item to LRT without existing contract

• if more contracts has same EAV value, you return random of them ImportCSVUserContractRolesTaskExecutor#208 is this problem?

- Thats a good point, I will try to fix this.

• you create useless empty role request while run this task again and again, please add checking for empty concepts and then create role request.

- I will fix this.

I tested you task and it works correctly without problem. Roles are assigned correctly, Good job! Please try fix tests and check coverage.

Ondřej Kopr wrote:

Petr Hanák wrote:

There was missing author in test file, so I completed it.

Class CSVToIdM.java isn't in tests.

OK.

Add localizations for errors

added localizations for COLUMN_NOT_FOUND, EMPTY_ATTACHMENT_ID.

- Not sure what this mean, but it should probably check all contracts

You search contracts EAV only by default form definition. When you have some another definition with your EAV, you didn't find the contract. But is this problem?

No, from HR we always sync. values to main contract definition. I've added this info to wiki for this LRT.

Test coverage

Now almost everything is covered (except form attributes definition)

fix your bugs from sonar (for example issues with Booleans)

Fixed bugs.

if more contracts has same EAV value, you return random of them ImportCSVUserContractRolesTaskExecutor#208 is this problem?

After consulting with Alena, role is now assigned to all contracts with matching eav

you create useless empty role request while run this task again and again, please add checking for empty concepts and then create role request.

Fixed.