(#154) Set Groups and Case Sensitivity to optional Parameters #155
Conversation
|
Report: Report: clientLibrary - scala:2.12.17
|
…y-for-token-retrieval-in-client-lib-as-optional
dk1844
left a comment
dk1844
left a comment
There was a problem hiding this comment.
The code is nice, but with a library we should meet additional standards (see comments below)
| * @param caseSensitiveGroups A boolean indicating whether the group prefixes should be treated as case sensitive. | ||
| * @return An AccessToken object representing the retrieved access token (JWT) from the login service. | ||
| */ | ||
| def fetchAccessToken( |
There was a problem hiding this comment.
This being a library, we should not just remove API as if it was never there between versions.
The existing API should stay in place with @deprecated annotation (ideally internally calling the new methods and thus be ready for removal in the future).
| */ | ||
| def fetchRefreshToken(username: String, password: String): RefreshToken = { | ||
| fetchAccessAndRefreshToken(username, password, List.empty, false)._2 | ||
| def fetchRefreshToken(authMethod: AuthMethod): RefreshToken = { |
There was a problem hiding this comment.
Neither the new method nor the existing (which should be present for some time as described above) seem to be covered with unit tests.
That is unfortunate, because the appropriate class TokenRetrievalClientTest already exists and it would be nice to have it covered. I see two options:
- You may need to mock the creation of
RestTemplate&KerberosRestTemplate(could be done by moving the creation to a method that you override in the test) to write the unit test properly. - A half-measure would be just to mock the
fetchTokenmethods in the class to check if those are receiving expected parameters. Not a complete unit test, but even this would be better than nothing if the option 1 cannot be reasonably done (I believe it should be).
Release Notes:
Closes: #154