Skip to content
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

Improve trip alert template (push + email) #196

Merged
merged 17 commits into from
Dec 20, 2023
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,19 @@
*/
public class TripMonitorNotification extends Model {
private static final Logger LOG = LoggerFactory.getLogger(TripMonitorNotification.class);

public final NotificationType type;
public final String body;

/** Getter functions are used by HTML template renderer */
public String getBody() {
return body;
}

public NotificationType getType() {
return type;
}

public TripMonitorNotification(NotificationType type, String body) {
this.type = type;
this.body = body;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -428,10 +428,13 @@ private void sendNotifications() {
Map<String, Object> templateData = Map.of(
"tripId", trip.id,
"tripName", trip.tripName,
"notifications", notifications.stream()
"notifications", new ArrayList<>(notifications).stream()
// Make certain notifications, such as the initial reminder one, appear on top.
.sorted(Comparator.comparingInt(TripMonitorNotification::sortOrder))
.map(notification -> notification.body)
.map(notification -> Map.of(
"type", notification.type.toString(),
"body", notification.body
))
.collect(Collectors.toList())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha I think this should be simply:

Suggested change
"notifications", new ArrayList<>(notifications).stream()
// Make certain notifications, such as the initial reminder one, appear on top.
.sorted(Comparator.comparingInt(TripMonitorNotification::sortOrder))
.map(notification -> notification.body)
.map(notification -> Map.of(
"type", notification.type.toString(),
"body", notification.body
))
.collect(Collectors.toList())
"notifications", new ArrayList<>(notifications)

);
// FIXME: Change log level
Expand Down
16 changes: 13 additions & 3 deletions src/main/resources/templates/MonitoredTripHtml.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,21 @@
<#macro EmailMain>
<div>
<h1>Your trip has the following notifications:</h1>
<ul>
<#list notifications as notification>
<li>${notification}</li>

<#list notifications as notification>
<#if notification.type == "INITIAL_REMINDER">
<p>${notification.body}</p>
</#if>
</#list>

<ul>
<#list notifications as notification>
<#if notification.type != "INITIAL_REMINDER">
<li>${notification.body}</li>
</#if>
</#list>
</ul>

<p>View all of your saved trips in <a href="${OTP_UI_URL}${TRIPS_PATH}/${tripId}">${OTP_UI_NAME}</a>.</p>
</div>
</#macro>
Expand Down
13 changes: 10 additions & 3 deletions src/main/resources/templates/MonitoredTripPush.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,14 @@
- iOS: 178 characters over up to 4 lines,
- Android: 240 characters (We are not using notification title at this time).
The max length is thus 178 characters.
-->${tripName}
- List alerts with bullets if there are more than one of them.
-->
<#if notifications?size gt 1>
<#list notifications as notification>
• ${notification}
</#list>
• ${notification.body}
</#list>
<#else>
<#list notifications as notification>
${notification.body}
</#list>
</#if>
9 changes: 8 additions & 1 deletion src/main/resources/templates/MonitoredTripSms.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,16 @@
-->

Your trip has the following notifications:

<#if notifications?size gt 1>
<#list notifications as notification>
• ${notification.body}
</#list>
<#else>
<#list notifications as notification>
-${notification}
${notification.body}
</#list>
</#if>

View trip: ${OTP_UI_URL}${TRIPS_PATH}/${tripId}

Expand Down
12 changes: 9 additions & 3 deletions src/main/resources/templates/MonitoredTripText.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,14 @@
-->

Your trip has the following notifications:
<#list notifications as notification>
-${notification}
</#list>
<#if notifications?size gt 1>
<#list notifications as notification>
• ${notification.body}
</#list>
<#else>
<#list notifications as notification>
${notification.body}
</#list>
</#if>

View trip: ${OTP_UI_URL}${TRIPS_PATH}/${tripId}
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
import org.hamcrest.MatcherAssert;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.MethodSource;
import org.opentripplanner.middleware.models.TripMonitorNotification;
import org.opentripplanner.middleware.testutils.OtpMiddlewareTestEnvironment;
import org.opentripplanner.middleware.tripmonitor.jobs.NotificationType;

import java.util.ArrayList;
import java.util.HashSet;
Expand All @@ -14,6 +16,14 @@
import static com.zenika.snapshotmatcher.SnapshotMatcher.matchesSnapshot;
import static org.hamcrest.MatcherAssert.assertThat;

/**
* Unit tests for {@code resources/templates/*.ftl} Freemarker Template Language files.
* <p>
* When the tests are first run, the {@code com.zenika.snapshotmatcher.SnapshotMatcher} class will create JSON snapshot
* files in {@code resources/snapshots/org/opentripplanner/middleware} which are used thereafter to compare with the
* results of the template files. If you change any {@code *.ftl} files you will need to delete their corresponding
* snapshot files, run the tests to create new snapshots, and then commit those files along with new template files.
*/
public class TemplateUtilsTest extends OtpMiddlewareTestEnvironment {
/**
* A parameterized test that checks whether various templates render in a way that matches a snapshot. The name of
Expand Down Expand Up @@ -50,8 +60,10 @@ public static List<TemplateRenderingTestCase> createTemplateRenderingTestCases()
// Trip Monitor Notifications tests (for CheckMonitoredTrip#sendNotifications).
Map<String, Object> notificationsData = Map.of(
"tripId", "18f642d5-f7a8-475a-9469-800129e6c0b3",
"notifications", List.of("Test notification.", "Another notification.")
);
"notifications", List.of(
new TripMonitorNotification(NotificationType.INITIAL_REMINDER, "This is the initial reminder text"),
new TripMonitorNotification(NotificationType.DEPARTURE_DELAY, "This is the departure delay text")
));
testCases.add(new TemplateRenderingTestCase(
notificationsData, "MonitoredTripSms.ftl", "Monitored Trip SMS"
));
Expand Down
Original file line number Diff line number Diff line change
@@ -1 +1 @@
"<style>\n body { \n padding: 1em;\n }\n\n h1 {\n font-size: 18px;\n font-weight: 600;\n }\n\n small {\n color: #515151\n }\n</style>\n\n\n\n\n<body>\n <div>\n<div>\n <div>\n <h1>Your trip has the following notifications:</h1>\n <ul>\n <li>Test notification.</li>\n <li>Another notification.</li>\n </ul>\n <p>View all of your saved trips in <a href=\"https://plan.example.com/#/account/trips/18f642d5-f7a8-475a-9469-800129e6c0b3\">Trip Planner</a>.</p>\n </div>\n</div>\n<hr/>\n<div>\n <p>\n <small>\n You're receiving this email because you're subscribed to notifications through Trip Planner. You can manage that subscription <a href=\"https://plan.example.com/#/account\">here</a>.\n </small>\n </p>\n</div>\n </div>\n</body>\n"
"<style>\n body { \n padding: 1em;\n }\n\n h1 {\n font-size: 18px;\n font-weight: 600;\n }\n\n small {\n color: #515151\n }\n</style>\n\n\n\n\n<body>\n <div>\n<div>\n <div>\n <h1>Your trip has the following notifications:</h1>\n\n <p>This is the initial reminder text</p>\n\n <ul>\n <li>This is the departure delay text</li>\n </ul>\n\n <p>View all of your saved trips in <a href=\"https://plan.example.com/#/account/trips/18f642d5-f7a8-475a-9469-800129e6c0b3\">Trip Planner</a>.</p>\n </div>\n</div>\n<hr/>\n<div>\n <p>\n <small>\n You're receiving this email because you're subscribed to notifications through Trip Planner. You can manage that subscription <a href=\"https://plan.example.com/#/account\">here</a>.\n </small>\n </p>\n</div>\n </div>\n</body>\n"
Original file line number Diff line number Diff line change
@@ -1 +1 @@
"\nYour trip has the following notifications:\n -Test notification.\n -Another notification.\n\nView trip: https://plan.example.com/#/account/trips/18f642d5-f7a8-475a-9469-800129e6c0b3\n\nTo stop receiving notifications, reply STOP."
"\nYour trip has the following notifications:\n\n• This is the initial reminder text\n• This is the departure delay text\n\nView trip: https://plan.example.com/#/account/trips/18f642d5-f7a8-475a-9469-800129e6c0b3\n\nTo stop receiving notifications, reply STOP."
Original file line number Diff line number Diff line change
@@ -1 +1 @@
"\nYour trip has the following notifications:\n -Test notification.\n -Another notification.\n\nView trip: https://plan.example.com/#/account/trips/18f642d5-f7a8-475a-9469-800129e6c0b3"
"\nYour trip has the following notifications:\n • This is the initial reminder text\n • This is the departure delay text\n\nView trip: https://plan.example.com/#/account/trips/18f642d5-f7a8-475a-9469-800129e6c0b3"
Loading