-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CCAP-642] - Sends confirmation email when provider agrees to care #1171
Conversation
src/main/java/org/ilgcc/app/submission/actions/SendProviderConfirmationEmail.java
Fixed
Show fixed
Hide fixed
src/main/java/org/ilgcc/app/submission/actions/SendProviderConfirmationEmail.java
Fixed
Show fixed
Hide fixed
cce87d9
to
68e897a
Compare
src/main/java/org/ilgcc/app/submission/actions/SendProviderConfirmationEmail.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nits but good to go
@@ -21,7 +21,7 @@ public class SendGridEmailService { | |||
private final SendGrid sendGrid = new SendGrid(System.getenv("SENDGRID_API_KEY")); | |||
|
|||
public Response sendEmail(String recipientAddress, String senderName, String subject, Content content) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just delete the top sendEmail method in favor of the one that accepts ILGCCEmail instead of overloading?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - we just need to move the other actions to the new sendEmail class. I didn't make the change here because the ticket was already getting pretty big and I wanted to get a thumbs up on the new approach first.
src/main/java/org/ilgcc/app/submission/actions/SendProviderConfirmationEmail.java
Show resolved
Hide resolved
src/main/java/org/ilgcc/app/submission/actions/SendProviderConfirmationEmail.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you're all done with the other comments/changes, can you also make sure you run the IntelliJ formatter and make sure everything has the correct spacing, etc? Thanks!
src/main/java/org/ilgcc/app/submission/actions/SendProviderConfirmationEmail.java
Outdated
Show resolved
Hide resolved
4065942
to
4bee67f
Compare
} else { | ||
// More than 2 childrenInitials, use comma for all but the last one | ||
String last = childrenInitials.remove(childrenInitials.size() - 1); // Remove and keep the last name | ||
return String.join(", ", childrenInitials) + " and " + last; // Join remaining with commas, append 'and last' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this method work for Spanish (or other future languages)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. This does not support Spanish currently and neither does the original implementation. I'll fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess for spanish, since the actual way this would be written doesn't vary, you could just pull a single string of "and" vs "y" ... right?
(this would all fall apart if we were doing like... Arabic?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes - you are correct for Spanish. It would probably break in other languages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool, let's just worry about Spanish and when we run into another language where this doesn't work -- and that's probably a non-western/non-latin based language -- we can worry about it. And by "we" I mean "someone else in the future who is probably not us" 😎
|
||
String emailCopy = action.setBodyCopy(emailData, Locale.ENGLISH); | ||
|
||
assertThat(emailCopy).contains( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems weird to me that we have tests that are now dependent on hardcoded strings; I thought we decided to stop hardcoding text and instead wanted to load things from the messages.properties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair. I can update the tests to use the message keys instead of the hard coded strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a big deal, just wanted to consistent.
…copies of hard strings in tests
🔗 Jira ticket
CCAP-642
✍️ Description
📷 Design reference
✅ Completion tasks