keycloak-user-migration icon indicating copy to clipboard operation
keycloak-user-migration copied to clipboard

feat: add property when group add as uuid

Open abdurrahmanekr opened this issue 11 months ago • 2 comments

I didn't notice the #135 that include same context. But if @petrisorcibian accept this pr I'm okay with that.

abdurrahmanekr avatar Mar 24 '24 14:03 abdurrahmanekr

Quality Gate Failed Quality Gate failed

Failed conditions
3 New Code Smells (required ≤ 0)
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

sonarqubecloud[bot] avatar Mar 25 '24 16:03 sonarqubecloud[bot]

Hello, thanks for contributing!

Some feedback:

  • Please provide tests for your functionality
  • It's not clear at a glance what is happening in the code and why. The code seems to use try-catch only to check if the groupName is a UUID - the try-catch should be extracted to a single isUuid method, which will make it both more readable and safer (the code won't accidentally catch an exception from another line of code)
  • Consider using a static regex Pattern instead of catching exceptions - I think it would be more performant
  • Any new functionality should be documented in README.md
  • From a client's perspective, it's unclear why we should use getGroupById if the groupName is UUID. How about making it a configurable option (whether to treat groups from the API as names or IDs) or just add groupIds to the API?

daniel-frak avatar Mar 25 '24 16:03 daniel-frak