Review #1327
Review #1327
Conversation
|
@matejonnet @michalszynkiewicz review, please |
| @@ -39,7 +39,7 @@ public JAASLoggedInUser(HttpServletRequest httpServletRequest) { | |||
|
|
|||
| @Override | |||
| public String getEmail() { | |||
| return "Email N/A (" + getUserName() + ")"; | |||
| return "unknown@unknown.com"; | |||
There was a problem hiding this comment.
it's always better to use example.com :)
|
looks ok to me |
| @@ -39,7 +39,7 @@ public JAASLoggedInUser(HttpServletRequest httpServletRequest) { | |||
|
|
|||
| @Override | |||
| public String getEmail() { | |||
| return "Email N/A (" + getUserName() + ")"; | |||
| return "example@example.com"; | |||
There was a problem hiding this comment.
Although this class is currently not used in our production deployment it can be (generally looking, it is not for test only). The previous return string is better fit than hard-coded example.com.
You probably changed it due to email validation constraint. I see two options here:
a) update the class to read the email and other missing data from the User entity, in this case it would mean, if you use JAAS you should have another way to register new users (For tests we can use demo-data).
b) remove the constraint
There was a problem hiding this comment.
Yes, I changed it due to the constraint and removing it is not the right way, I think.
I don't understand the A):
How can I read it from the User entity, when the entity is meant to be created based on what's obtained from the JAASAuthenticator?
Currently there is no e-mail written to the DB, so I can change it to smth like:
getUserName() + "@example.com"
or
getUserName() + "@unknown.com"
to preserve the information there and to have the correct e-mail address format.
There was a problem hiding this comment.
User entity inserted only when the user does not exists. We have "auto-registration" for authenticated users. You can insert a user with proper email and matching username, before the tests.
There was a problem hiding this comment.
OK. That could be done in the test probably, but why not to change it to return a valid e-mail?
|
@matejonnet Changes done |
No description provided.