Forgot password #643
Forgot password #643
Conversation
Reformat unit tests.
…sswordControllerAction.
There was a problem hiding this comment.
I think there are 5 main issues:
- Configuration following the new standard (reference.conf + configuration class)
- Output parameter of RecoverPasswordController
- Email sending in German
- Handling of form errors
- Tests readability
The rest are make-up issues :P
But anyway great job! You clearly put a lot of effort on the tests.
app/EmailSenderProvider.java
Outdated
| final String security = configuration.getString("application.smtp.security"); | ||
| timeoutMs = configuration.getInt("application.smtp.timeoutMs", 10 * 1000); | ||
| smtpConfiguration = | ||
| new SmtpConfiguration(host, port, SmtpConfiguration.TransportSecurity.valueOf(security), username, password); |
There was a problem hiding this comment.
I'll create a configuration class for it.
app/EmailSenderProvider.java
Outdated
| final String username = configuration.getString("application.smtp.username"); | ||
| final String password = configuration.getString("application.smtp.password"); | ||
| final String security = configuration.getString("application.smtp.security"); | ||
| timeoutMs = configuration.getInt("application.smtp.timeoutMs", 10 * 1000); |
There was a problem hiding this comment.
Move defaults to reference.conf
app/EmailSenderProvider.java
Outdated
| import java.util.concurrent.ForkJoinPool; | ||
|
|
||
|
|
||
| public class EmailSenderProvider implements Provider<EmailSender> { |
There was a problem hiding this comment.
Make it final to avoid overriding.
app/EmailSenderProvider.java
Outdated
| timeoutMs = configuration.getInt("application.smtp.timeoutMs", 10 * 1000); | ||
| smtpConfiguration = | ||
| new SmtpConfiguration(host, port, SmtpConfiguration.TransportSecurity.valueOf(security), username, password); | ||
| executor = new ForkJoinPool(5); |
There was a problem hiding this comment.
Why 5? Why not common pool?
There was a problem hiding this comment.
I changed it to ForkJoinPool.commonPool() 😉
| this.authenticationReverseRouter = authenticationReverseRouter; | ||
| } | ||
|
|
||
| @Nullable |
There was a problem hiding this comment.
We can remove this nullable
public/javascripts/sunrise.js
Outdated
| @@ -1,3 +1,8 @@ | |||
| // Focus email field in reset change password modal | |||
| $("#forgot-password-modal").on('shown.bs.modal', function(){ | |||
| $(this).find('#reset-email-input').focus(); | |||
There was a problem hiding this comment.
And it's not used anymore, I just forgot to remove it after we decided to not use modal dialogs ;-)
There was a problem hiding this comment.
Oh true, there are no modals anymore 😅
| final CustomerToken customerToken = customerTokenCompletionStage.get(); | ||
| assertThat(customerToken.getValue()).isEqualTo(tokenValue); | ||
| } | ||
| } |
There was a problem hiding this comment.
I find the test a bit hard to read. I think specially tests should be very easy to read, so that the assertion is clear as water and also when something changes the test is easily adaptable too.
I'd suggest to just brainstorm ideas together ( @acbeni too, since this is going to be the model for future action controller tests). But one objective would be to hide more the details of "how the factor parameters of the tests are implemented internally" and show only high level information about what do they do.
For example, instead of having:
final DefaultRecoverPasswordFormData recoveryEmailFormData = new DefaultRecoverPasswordFormData();
recoveryEmailFormData.setEmail("test@example.com");Extract it to their own method that is called formDataWithExistingEmail and use it directly in the call:
final CompletableFuture<CustomerToken> customerTokenCompletionStage =
(CompletableFuture<CustomerToken>)
defaultPasswordRecoveryControllerAction.apply(formDataWithExistingEmail());That saves 2 lines of code, it reads nicely, and I the details of the form are hidden from the test, we just trust it's a valid email that should therefore return a customerToken. Also it lets it clear for the reader the intention of using that email.
We can apply more ideas like this. WDYT?
There was a problem hiding this comment.
I completely agree with your points and let's discuss them further 👍
|
|
||
| final CompletableFuture<Customer> customerCompletableFuture = | ||
| (CompletableFuture<Customer>) defaultResetPasswordControllerAction.apply(resetTokenValue, resetPasswordFormData); | ||
| final Customer customer = customerCompletableFuture.get(); |
There was a problem hiding this comment.
I would consider making a one-liner:
final Customer customer = defaultResetPasswordControllerAction.apply(resetTokenValue, resetPasswordFormData).toCompletableFuture().get();I find the natural conversion to completable future more readable than the casting, which probably (didn't check) shows as a warning in most IDE too.
There was a problem hiding this comment.
Uh now I'm confused. How is this customer inside the method not conflicting in name with the customer declared in the class? 😮
I must be missing something but I can't see it.
There was a problem hiding this comment.
The local variable customershadows the member variable customer. I will change the name to make this clear 👍
| @Mock | ||
| private MessagesApi messagesApi; | ||
| @Mock | ||
| private Formatters formatters; |
There was a problem hiding this comment.
Seeing that you have to mock so many things, why don't we better have a running Play app behind the tests? We increase the test time in 2 seconds, but I think it's worth it if we make the tests simpler.
There was a problem hiding this comment.
I tried to run a play app, but couldn't figure out how to set it up with our modular framework architecture. And because we hide a lot of our implementation classes (which is good), using mocks was the easiest solution. Let's discuss this further ;-)
| forgotPassword: | ||
| title: Forgot Password | ||
| description: Please submit your email address to receive a password reset link. | ||
| email: |
There was a problem hiding this comment.
Not sure how it is done now, but we should deliver the email in the language the user is in currently. I see in the German version this part is missing. Is the email always sent in English?
There was a problem hiding this comment.
The email is rendered via the handlebar template password-reset-email and uses the locale of the request. But you're right that I forgot to add the i18n entries for german to the de/my-account.yaml file ;-)
|
@lauraluiz I now improved the code as we discussed and am looking forward to your review. |
| msg.setRecipients(Message.RecipientType.TO, recoveryEmailFormData.email()); | ||
| msg.setSubject(recoverPasswordEmailPageContent.getSubject(), "UTF-8"); | ||
| msg.setContent(emailText, "text/html"); | ||
| }).thenApply(s -> resetPasswordToken); |
There was a problem hiding this comment.
I just thought this block here is a perfect candidate to go into its own class: building the page data, rendering the html content, sending the email. Then the tests would definitely be cleaner :)
That's the part we were missing I think. What do you think?
In any case, it's a test, we can improve it later.
| * | ||
| * @return the repeated new password | ||
| */ | ||
| String confirmPassword(); |
There was a problem hiding this comment.
Sorry to bring this up and not before, but do you think we should expose this in the interface? The idea is to have here only the "util" data coming from the form, not the raw data. The confirmation password is more an "implementation" detail, not useful for the operation. IMO we should remove it from the interface.
There was a problem hiding this comment.
@lauraluiz No problem 😉 I agree with you. I will remove this method 👍
| } | ||
|
|
||
| @Override | ||
| public CompletionStage<Result> handleInvalidForm(final String resetToken, final Form<? extends ResetPasswordFormData> form) { |
There was a problem hiding this comment.
This override is not necessary anymore because the parent class already contains the same implementation. Maybe it was a leftover from the previous implementation?
|
@lauraluiz I updated my PR and am looking forward to your review 😄 |
Closes #532