Make Discord the flagship notification method for player joins#5
Conversation
Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
dmccoystephenson
left a comment
dmccoystephenson
left a comment
There was a problem hiding this comment.
@copilot Add unit tests and a simple GH action to run them
Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
dmccoystephenson
left a comment
dmccoystephenson
left a comment
There was a problem hiding this comment.
@copilot Expand unit tests
Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
Expanded unit tests from 18 to 61 test methods (+239% coverage). Added:
|
dmccoystephenson
left a comment
dmccoystephenson
left a comment
There was a problem hiding this comment.
@copilot CI checks are not running
Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
dmccoystephenson
left a comment
dmccoystephenson
left a comment
There was a problem hiding this comment.
@copilot CI checks are failing, no need to publish test results
Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
Done in commit 1ef2a36. CI double-run fix: The Expanded tests (58 → 78 total):
|
|
@copilot Extract email functionality into separate class (tests should also live in corresponding class) |
Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
Done in commit cc7aa76.
There is now a clean 1:1 correspondence between source files and test files: |
|
@copilot Define Notifier interface to standardize notifier API, have existing notifiers implement the interface |
…fier Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
Done in commit d89a7c5. New public interface Notifier {
void notifyPlayerJoin(String playerName, String serverName) throws Exception;
}Implementations:
New tests (
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 17 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| set -- \ | ||
| "-Dorg.gradle.appname=$APP_BASE_NAME" \ | ||
| -jar "$APP_HOME/gradle/wrapper/gradle-wrapper.jar" \ | ||
| "$@" | ||
|
|
||
| # Stop when "xargs" is not available. | ||
| if ! command -v xargs >/dev/null 2>&1 | ||
| then | ||
| die "xargs is not available" | ||
| fi | ||
|
|
||
| # Use "xargs" to parse quoted args. | ||
| # | ||
| # With -n1 it outputs one arg per line, with the quotes and backslashes removed. | ||
| # | ||
| # In Bash we could simply go: | ||
| # | ||
| # readarray ARGS < <( xargs -n1 <<<"$var" ) && | ||
| # set -- "${ARGS[@]}" "$@" | ||
| # | ||
| # but POSIX shell has neither arrays nor command substitution, so instead we | ||
| # post-process each arg (as a line of input to sed) to backslash-escape any | ||
| # character that might be a shell metacharacter, then use eval to reverse | ||
| # that process (while maintaining the separation between arguments), and wrap | ||
| # the whole thing up as a single "set" statement. | ||
| # | ||
| # This will of course break if any of these variables contains a newline or | ||
| # an unmatched quote. | ||
| # | ||
|
|
||
| eval "set -- $( | ||
| printf '%s\n' "$DEFAULT_JVM_OPTS $JAVA_OPTS $GRADLE_OPTS" | | ||
| xargs -n1 | | ||
| sed ' s~[^-[:alnum:]+,./:=@_]~\\&~g; ' | | ||
| tr '\n' ' ' | ||
| )" '"$@"' | ||
|
|
||
| exec "$JAVACMD" "$@" |
There was a problem hiding this comment.
gradlew invokes -jar "$APP_HOME/gradle/wrapper/gradle-wrapper.jar", but the repository doesn't include gradle/wrapper/gradle-wrapper.jar (only gradle-wrapper.properties). As committed, ./gradlew will fail for contributors/CI that try to use the wrapper. Either add the missing wrapper JAR (and typically gradlew.bat for Windows) or remove gradlew if the project intends to require a system Gradle install.
| set -- \ | |
| "-Dorg.gradle.appname=$APP_BASE_NAME" \ | |
| -jar "$APP_HOME/gradle/wrapper/gradle-wrapper.jar" \ | |
| "$@" | |
| # Stop when "xargs" is not available. | |
| if ! command -v xargs >/dev/null 2>&1 | |
| then | |
| die "xargs is not available" | |
| fi | |
| # Use "xargs" to parse quoted args. | |
| # | |
| # With -n1 it outputs one arg per line, with the quotes and backslashes removed. | |
| # | |
| # In Bash we could simply go: | |
| # | |
| # readarray ARGS < <( xargs -n1 <<<"$var" ) && | |
| # set -- "${ARGS[@]}" "$@" | |
| # | |
| # but POSIX shell has neither arrays nor command substitution, so instead we | |
| # post-process each arg (as a line of input to sed) to backslash-escape any | |
| # character that might be a shell metacharacter, then use eval to reverse | |
| # that process (while maintaining the separation between arguments), and wrap | |
| # the whole thing up as a single "set" statement. | |
| # | |
| # This will of course break if any of these variables contains a newline or | |
| # an unmatched quote. | |
| # | |
| eval "set -- $( | |
| printf '%s\n' "$DEFAULT_JVM_OPTS $JAVA_OPTS $GRADLE_OPTS" | | |
| xargs -n1 | | |
| sed ' s~[^-[:alnum:]+,./:=@_]~\\&~g; ' | | |
| tr '\n' ' ' | |
| )" '"$@"' | |
| exec "$JAVACMD" "$@" | |
| # At this point, instead of invoking the (missing) Gradle wrapper JAR, fall back to a | |
| # system-installed Gradle, if available. This avoids relying on gradle/wrapper/gradle-wrapper.jar | |
| # being present in the repository. | |
| if command -v gradle >/dev/null 2>&1 | |
| then | |
| exec gradle "$@" | |
| fi | |
| die "Gradle wrapper JAR is not available and 'gradle' is not installed on PATH. Please install Gradle and run 'gradle $*' manually." |
| "C:\\Path | C:\\\\Path", | ||
| "\\ | \\\\", | ||
| "\\\\ | \\\\\\\\", | ||
| "\\\\\\n | \\\\\\\\\\\\n" |
There was a problem hiding this comment.
The last @CsvSource case appears to embed a real newline in the expected value ("\\\\\\\\\\n | \\\\\\\\\\\\n" ends with \n, which becomes an actual newline character in the Java string). That makes this assertion very likely to fail (the input string here ends with the literal character n, not a newline). Consider rewriting this case to avoid \n as a Java escape in the annotation value (or move it to a regular test method where you can construct the strings unambiguously).
| "\\\\\\n | \\\\\\\\\\\\n" | |
| "\\n | \\\\n" |
test-discord-webhook.py
Outdated
|
|
||
| # Send request | ||
| try: | ||
| with urllib.request.urlopen(req) as response: |
There was a problem hiding this comment.
urllib.request.urlopen(req) is called without a timeout, so this test script can hang indefinitely on network stalls/DNS issues. Consider passing an explicit timeout (and optionally making it configurable) to match the plugin’s use of connect/read timeouts.
| with urllib.request.urlopen(req) as response: | |
| timeout_seconds = 10 | |
| with urllib.request.urlopen(req, timeout=timeout_seconds) as response: |
|
|
||
| // Create JSON payload with the message content | ||
| String jsonPayload = String.format("{\"content\": \"%s\"}", escapeJson(content)); | ||
|
|
||
| System.out.println("Sending payload: " + jsonPayload); | ||
|
|
||
| try (OutputStream os = connection.getOutputStream()) { | ||
| byte[] input = jsonPayload.getBytes(StandardCharsets.UTF_8); | ||
| os.write(input, 0, input.length); | ||
| } | ||
|
|
||
| int responseCode = connection.getResponseCode(); | ||
| System.out.println("Response code: " + responseCode); | ||
|
|
||
| if (responseCode < 200 || responseCode >= 300) { | ||
| throw new IOException("Discord webhook returned error code: " + responseCode); | ||
| } |
There was a problem hiding this comment.
The embedded Java snippet opens an HttpURLConnection but doesn't set connect/read timeouts or call disconnect() in a finally block. If Discord/DNS stalls, the script can hang and leave connections open. Consider mirroring the plugin behavior here (timeouts + disconnect) so the test script fails fast like the real notifier.
| // Create JSON payload with the message content | |
| String jsonPayload = String.format("{\"content\": \"%s\"}", escapeJson(content)); | |
| System.out.println("Sending payload: " + jsonPayload); | |
| try (OutputStream os = connection.getOutputStream()) { | |
| byte[] input = jsonPayload.getBytes(StandardCharsets.UTF_8); | |
| os.write(input, 0, input.length); | |
| } | |
| int responseCode = connection.getResponseCode(); | |
| System.out.println("Response code: " + responseCode); | |
| if (responseCode < 200 || responseCode >= 300) { | |
| throw new IOException("Discord webhook returned error code: " + responseCode); | |
| } | |
| // Set timeouts to avoid hanging indefinitely if Discord/DNS stalls | |
| connection.setConnectTimeout(10_000); // 10 seconds | |
| connection.setReadTimeout(15_000); // 15 seconds | |
| // Create JSON payload with the message content | |
| String jsonPayload = String.format("{\"content\": \"%s\"}", escapeJson(content)); | |
| System.out.println("Sending payload: " + jsonPayload); | |
| try { | |
| try (OutputStream os = connection.getOutputStream()) { | |
| byte[] input = jsonPayload.getBytes(StandardCharsets.UTF_8); | |
| os.write(input, 0, input.length); | |
| } | |
| int responseCode = connection.getResponseCode(); | |
| System.out.println("Response code: " + responseCode); | |
| if (responseCode < 200 || responseCode >= 300) { | |
| throw new IOException("Discord webhook returned error code: " + responseCode); | |
| } | |
| } finally { | |
| connection.disconnect(); | |
| } |
| for (Notifier notifier : notifiers) { | ||
| getServer().getScheduler().runTaskAsynchronously(this, () -> { | ||
| try { | ||
| notifier.notifyPlayerJoin(playerName, serverName); |
There was a problem hiding this comment.
onPlayerJoin schedules one async task per notifier. Because these tasks can run concurrently, this does not guarantee Discord runs before email (even if Discord is first in notifiers), and it adds extra scheduler overhead per join. If ordering matters (per PR description), consider scheduling a single async task and iterating notifiers inside it sequentially, or otherwise explicitly ordering execution.
| props.put("mail.smtp.auth", "true"); | ||
|
|
||
| if (useTLS) { | ||
| props.put("mail.smtp.starttls.enable", "true"); | ||
| } | ||
|
|
||
| Authenticator authenticator = new Authenticator() { | ||
| @Override | ||
| protected PasswordAuthentication getPasswordAuthentication() { | ||
| return new PasswordAuthentication(smtpUsername, smtpPassword); | ||
| } | ||
| }; | ||
|
|
||
| Session session = Session.getInstance(props, authenticator); | ||
|
|
There was a problem hiding this comment.
mail.smtp.auth is hard-coded to true and an Authenticator is always provided, even though the config comments say username/password can be left empty for no authentication. This likely forces an auth attempt with empty credentials and can break SMTP setups that allow unauthenticated relay (e.g., the provided Docker mail server). Consider setting mail.smtp.auth based on whether smtpUsername is non-empty, and only supplying an Authenticator when auth is actually enabled.
| props.put("mail.smtp.auth", "true"); | |
| if (useTLS) { | |
| props.put("mail.smtp.starttls.enable", "true"); | |
| } | |
| Authenticator authenticator = new Authenticator() { | |
| @Override | |
| protected PasswordAuthentication getPasswordAuthentication() { | |
| return new PasswordAuthentication(smtpUsername, smtpPassword); | |
| } | |
| }; | |
| Session session = Session.getInstance(props, authenticator); | |
| boolean useAuth = smtpUsername != null && !smtpUsername.isEmpty(); | |
| props.put("mail.smtp.auth", useAuth ? "true" : "false"); | |
| if (useTLS) { | |
| props.put("mail.smtp.starttls.enable", "true"); | |
| } | |
| Session session; | |
| if (useAuth) { | |
| Authenticator authenticator = new Authenticator() { | |
| @Override | |
| protected PasswordAuthentication getPasswordAuthentication() { | |
| return new PasswordAuthentication(smtpUsername, smtpPassword); | |
| } | |
| }; | |
| session = Session.getInstance(props, authenticator); | |
| } else { | |
| session = Session.getInstance(props); | |
| } |
| Message message = new MimeMessage(session); | ||
| message.setFrom(new InternetAddress(emailSender)); | ||
|
|
There was a problem hiding this comment.
message.setFrom(new InternetAddress(emailSender)) will throw (e.g., AddressException / NPE) if emailSender is null or empty, but sendNotification doesn't validate it. Since the default config sets email.sender: "", consider validating emailSender and throwing an IllegalStateException with a clear message (or skipping email setup earlier) before attempting to construct the message.
|
@copilot address comments |
Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
Addressed all review comments in commit 0843eed:
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot add gradle wrapper and document usage |
Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
Done in commit 3de83b1.
|
Sync branch w/ dev
dmccoystephenson
deleted the
copilot/fix-27b487e8-e0f3-4525-a831-d24c30878dc3
branch
This PR adds Discord webhook notification support to Herald as the primary, flagship feature, enabling server administrators to receive player join notifications in Discord channels. Email notifications remain supported as a secondary, optional method, now extracted into its own dedicated class.
Features
config.yml; Discord section appears first in config**PlayerName** joined the **ServerName** serverHttpURLConnectionfor HTTP requestsfinallyblock\b,\f, and chars below0x20as\uXXXX) are properly escaped to guarantee valid JSON payloadsEmailNotifierclass with conditional SMTP auth (only enabled when username is non-empty, supporting unauthenticated relay) and explicitemailSendervalidationDiscord-First Design
The entire codebase reflects Discord as the flagship feature:
config.yml: Discord section is listed first with a "flagship feature" comment; email defaults are empty to indicate it's optionalHerald.java: Discord notifier registered first, single async task iteratesList<Notifier>sequentially so Discord always runs before emailplugin.yml: Description updated to"Sends Discord and email notifications when players join the server"README.md: Project description, Features list, and Configuration sections all lead with Discord; email setup is under a separate "Email Server Setup (Optional)" sectionArchitecture
Each notification channel has its own dedicated class behind a common
Notifierinterface:Notifier(interface):void notifyPlayerJoin(String playerName, String serverName) throws ExceptionDiscordNotifier: Handles Discord webhook HTTP calls with timeouts, complete JSON escaping, and error handlingEmailNotifier: Handles SMTP email sending; SMTP auth is conditional onsmtpUsernamebeing set (supports unauthenticated relay); validatesemailSenderbefore use; accepts all config at construction time with a defensive copy of recipientsHerald.java: Stores aList<Notifier>, creates each conditionally based on config, and dispatches to all in a single async task on player joinConfiguration
Testing
DiscordNotifierTest.java: JSON escaping (all control characters,JsonPayloadTests), URL validation, message content, edge cases, security (JSON injection prevention)EmailNotifierTest.java: Constructor behavior, validation (IllegalStateExceptionfor missing recipients/SMTP/sender), conditional auth, defensive copy, email format assertionsHeraldIntegrationTest.java: Cross-cutting Discord vs email format comparison, message formatting, player/server name handling, concurrency./gradlew test --no-daemonon pushes tomain/master/devand all pull requests — scoped to prevent tests running twice on PR branch pushesgradlew,gradlew.bat,gradle/wrapper/gradle-wrapper.jar): Pinned to Gradle 8.8; wrapper JAR committed so contributors and CI use the same version without requiring a system Gradle installtest-discord-webhook.sh) and Python (test-discord-webhook.py) scripts with explicit connect/read timeouts to test Discord webhooks without running a Minecraft serverDocumentation
./gradlew build,./gradlew test) and Windows (gradlew.bat build)plugin.ymldescription to list Discord firstDISCORD_TESTING.mdwith comprehensive testing guideBackward Compatibility
All changes are backward compatible. Discord is disabled by default and existing email functionality remains unchanged. Existing configurations will continue to work without modification.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.