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

PR notifications #1146

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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 @@ -22,7 +22,10 @@
import org.togetherjava.tjbot.features.code.CodeMessageManualDetection;
import org.togetherjava.tjbot.features.filesharing.FileSharingMessageListener;
import org.togetherjava.tjbot.features.github.GitHubCommand;
import org.togetherjava.tjbot.features.github.GitHubLinkCommand;
import org.togetherjava.tjbot.features.github.GitHubReference;
import org.togetherjava.tjbot.features.github.GitHubUnlinkCommand;
import org.togetherjava.tjbot.features.github.PullRequestNotificationRoutine;
import org.togetherjava.tjbot.features.help.GuildLeaveCloseThreadListener;
import org.togetherjava.tjbot.features.help.HelpSystemHelper;
import org.togetherjava.tjbot.features.help.HelpThreadActivityUpdater;
Expand Down Expand Up @@ -136,6 +139,7 @@ public static Collection<Feature> createFeatures(JDA jda, Database database, Con
features.add(new MarkHelpThreadCloseInDBRoutine(database, helpThreadLifecycleListener));
features.add(new MemberCountDisplayRoutine(config));
features.add(new RSSHandlerRoutine(config, database));
features.add(new PullRequestNotificationRoutine(database, config));

// Message receivers
features.add(new TopHelpersMessageListener(database, config));
Expand Down Expand Up @@ -192,6 +196,8 @@ public static Collection<Feature> createFeatures(JDA jda, Database database, Con
features.add(new BookmarksCommand(bookmarksSystem));
features.add(new ChatGptCommand(chatGptService, helpSystemHelper));
features.add(new JShellCommand(jshellEval));
features.add(new GitHubLinkCommand(database, config));
features.add(new GitHubUnlinkCommand(database));

FeatureBlacklist<Class<?>> blacklist = blacklistConfig.normal();
return blacklist.filterStream(features.stream(), Object::getClass).toList();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
package org.togetherjava.tjbot.features.github;

import net.dv8tion.jda.api.events.interaction.command.SlashCommandInteractionEvent;
import net.dv8tion.jda.api.interactions.commands.OptionMapping;
import net.dv8tion.jda.api.interactions.commands.OptionType;
import org.kohsuke.github.GHRepository;
import org.kohsuke.github.GitHub;
import org.kohsuke.github.GitHubBuilder;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import org.togetherjava.tjbot.config.Config;
import org.togetherjava.tjbot.db.Database;
import org.togetherjava.tjbot.db.DatabaseException;
import org.togetherjava.tjbot.db.generated.tables.PrNotifications;
import org.togetherjava.tjbot.db.generated.tables.records.PrNotificationsRecord;
import org.togetherjava.tjbot.features.CommandVisibility;
import org.togetherjava.tjbot.features.SlashCommandAdapter;

import java.io.IOException;

/**
* Slash command used to link a GitHub project to a discord channel to post pull request
* notifications.
*/
public class GitHubLinkCommand extends SlashCommandAdapter {

private static final Logger logger = LoggerFactory.getLogger(GitHubLinkCommand.class);

private static final String REPOSITORY_OWNER_OPTION = "owner";
private static final String REPOSITORY_NAME_OPTION = "name";

private final Database database;
private final String githubApiKey;

/**
* Creates new GitHub link command.
*
* @param database the database to store the new linked pull request notifications
* @param config the config to get the GitHub API key
*/
public GitHubLinkCommand(Database database, Config config) {
super("link-gh-project",
"Links a GitHub repository to this project post to receive pull request notifications",
CommandVisibility.GUILD);
this.database = database;
this.githubApiKey = config.getGitHubApiKey();

getData()
.addOption(OptionType.STRING, REPOSITORY_OWNER_OPTION,
"The owner of the repository to be linked", true)
.addOption(OptionType.STRING, REPOSITORY_NAME_OPTION,
"The name of the repository to be linked", true);
}

@Override
public void onSlashCommand(SlashCommandInteractionEvent event) {
OptionMapping repositoryOwnerOption = event.getOption(REPOSITORY_OWNER_OPTION);
OptionMapping repositoryNameOption = event.getOption(REPOSITORY_NAME_OPTION);

if (repositoryOwnerOption == null || repositoryNameOption == null) {
event.reply("You must specify a repository owner and a repository name")
.setEphemeral(true)
.queue();
return;
}

long channelId = event.getChannelIdLong();
String repositoryOwner = repositoryOwnerOption.getAsString();
String repositoryName = repositoryNameOption.getAsString();

GitHub github;
try {
github = new GitHubBuilder().withOAuthToken(githubApiKey).build();
SquidXTV marked this conversation as resolved.
Show resolved Hide resolved
} catch (IOException e) {
logger.error("Failed to initialize GitHub API wrapper.", e);
SquidXTV marked this conversation as resolved.
Show resolved Hide resolved
event.reply("Failed to initialize GitHub API wrapper.").setEphemeral(true).queue();
SquidXTV marked this conversation as resolved.
Show resolved Hide resolved
return;
}

try {
if (!isRepositoryAccessible(github, repositoryOwner, repositoryName)) {
event.reply("Repository is not publicly available.").setEphemeral(true).queue();
Copy link
Contributor

Choose a reason for hiding this comment

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

Opt to provide the owner and name in this error message. It could have been they made a typo. Maybe include a link to the docs on how they can set the visibility too.

logger.info("Repository {}/{} is not accessible.", repositoryOwner, repositoryName);
return;
}
} catch (IOException e) {
logger.error("Failed to check if GitHub repository is available.", e);
event.reply("Failed to link repository.").setEphemeral(true).queue();
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, opt for the user friendly message. Make it a constant above since it can be reused.

return;
}

try {
saveNotificationToDatabase(channelId, repositoryOwner, repositoryName);
event.reply("Successfully linked repository.").setEphemeral(true).queue();
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make this response a pretty embed for everyone in the project thread to see.

} catch (DatabaseException e) {
logger.error("Failed to save pull request notification to database.", e);
event.reply("Failed to link repository.").setEphemeral(true).queue();
Copy link
Contributor

Choose a reason for hiding this comment

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

Better error message please

}
}

private boolean isRepositoryAccessible(GitHub github, String owner, String name)
throws IOException {
GHRepository repository = github.getRepository(owner + "/" + name);
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be static and also just return github.getRepository(owner + "/" + name) != null;

return repository != null;
}

private void saveNotificationToDatabase(long channelId, String repositoryOwner,
Copy link
Contributor

Choose a reason for hiding this comment

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

are we saving "notifications" to the database? It's more so, saving GH names for a specific project. Notification blurs that intent.

String repositoryName) {
database.write(context -> {
PrNotificationsRecord prNotificationsRecord =
context.newRecord(PrNotifications.PR_NOTIFICATIONS);
prNotificationsRecord.setChannelId(channelId);
prNotificationsRecord.setRepositoryOwner(repositoryOwner);
prNotificationsRecord.setRepositoryName(repositoryName);
prNotificationsRecord.insert();
});
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
package org.togetherjava.tjbot.features.github;

import net.dv8tion.jda.api.events.interaction.command.SlashCommandInteractionEvent;
import net.dv8tion.jda.api.interactions.commands.OptionMapping;
import net.dv8tion.jda.api.interactions.commands.OptionType;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import org.togetherjava.tjbot.db.Database;
import org.togetherjava.tjbot.db.DatabaseException;
import org.togetherjava.tjbot.db.generated.tables.PrNotifications;
import org.togetherjava.tjbot.features.CommandVisibility;
import org.togetherjava.tjbot.features.SlashCommandAdapter;

/**
* Slash command to unlink a project from a channel.
*/
public class GitHubUnlinkCommand extends SlashCommandAdapter {

private static final Logger logger = LoggerFactory.getLogger(GitHubUnlinkCommand.class);

private static final String REPOSITORY_OWNER_OPTION = "owner";
private static final String REPOSITORY_NAME_OPTION = "name";

private final Database database;

/**
* Creates new GitHub unlink command.
*
* @param database the database to remove linked pull request notifications
*/
public GitHubUnlinkCommand(Database database) {
super("unlink-gh-project", "Unlinks a GitHub repository", CommandVisibility.GUILD);
this.database = database;

getData()
SquidXTV marked this conversation as resolved.
Show resolved Hide resolved
.addOption(OptionType.STRING, REPOSITORY_OWNER_OPTION,
"The owner of the repository to get unlinked", true)
SquidXTV marked this conversation as resolved.
Show resolved Hide resolved
.addOption(OptionType.STRING, REPOSITORY_NAME_OPTION,
"The name of the repository to get unlinked", true);
}

@Override
public void onSlashCommand(SlashCommandInteractionEvent event) {
OptionMapping repositoryOwnerOption = event.getOption(REPOSITORY_OWNER_OPTION);
OptionMapping repositoryNameOption = event.getOption(REPOSITORY_NAME_OPTION);

if (repositoryOwnerOption == null || repositoryNameOption == null) {
event.reply("You must specify a repository owner and a repository name")
.setEphemeral(true)
.queue();
return;
}

long channelId = event.getChannelIdLong();
String repositoryOwner = repositoryOwnerOption.getAsString();
String repositoryName = repositoryNameOption.getAsString();

try {
int deleted = deleteNotification(channelId, repositoryOwner, repositoryName);

if (deleted == 0) {
event.reply("The provided repository wasn't linked to this channel previously.")
.setEphemeral(true)
.queue();
} else {
event.reply("Successfully unlinked repository.").setEphemeral(true).queue();
}
} catch (DatabaseException e) {
logger.error("Failed to delete pull request notification link from database.", e);
event.reply("Failed to unlink repository.").setEphemeral(true).queue();
}
}

private int deleteNotification(long channelId, String repositoryOwner, String repositoryName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

it's not a notification so please rename

return database
.writeAndProvide(context -> context.deleteFrom(PrNotifications.PR_NOTIFICATIONS)
.where(PrNotifications.PR_NOTIFICATIONS.CHANNEL_ID.eq(channelId))
.and(PrNotifications.PR_NOTIFICATIONS.REPOSITORY_OWNER.eq(repositoryOwner))
.and(PrNotifications.PR_NOTIFICATIONS.REPOSITORY_NAME.eq(repositoryName))
.execute());
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
package org.togetherjava.tjbot.features.github;

import net.dv8tion.jda.api.JDA;
import net.dv8tion.jda.api.entities.channel.concrete.ThreadChannel;
import org.kohsuke.github.GHIssueState;
import org.kohsuke.github.GHPullRequest;
import org.kohsuke.github.GHRepository;
import org.kohsuke.github.GitHub;
import org.kohsuke.github.GitHubBuilder;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import org.togetherjava.tjbot.config.Config;
import org.togetherjava.tjbot.db.Database;
import org.togetherjava.tjbot.db.generated.tables.PrNotifications;
import org.togetherjava.tjbot.db.generated.tables.records.PrNotificationsRecord;
import org.togetherjava.tjbot.features.Routine;

import java.io.IOException;
import java.util.Date;
import java.util.List;
import java.util.concurrent.TimeUnit;

/**
* Routine to send a notification about new pull request.
*/
public class PullRequestNotificationRoutine implements Routine {

private static final Logger logger =
LoggerFactory.getLogger(PullRequestNotificationRoutine.class);

private final Database database;
private final String githubApiKey;
private Date lastExecution;
SquidXTV marked this conversation as resolved.
Show resolved Hide resolved

/**
* Creates new notification routine.
*
* @param database the database to get the pull request notifications
* @param config the config to get the GitHub API key
*/
public PullRequestNotificationRoutine(Database database, Config config) {
this.database = database;
this.githubApiKey = config.getGitHubApiKey();
this.lastExecution = new Date();
}

@Override
public Schedule createSchedule() {
return new Schedule(ScheduleMode.FIXED_RATE, 0, 15, TimeUnit.MINUTES);
Copy link
Contributor

Choose a reason for hiding this comment

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

Make the time an option in the config or make it a static const.
Reasons for config is that I'm not sure 15 mins is what we want to set it as just yet. We need to experiment with the timer and have an easy way to update it.
If we don't make it a config, then a static constant for readability because 15 on it's own is a "magic number".

}

@Override
public void runRoutine(JDA jda) {
GitHub github;
try {
github = new GitHubBuilder().withOAuthToken(githubApiKey).build();
} catch (IOException e) {
logger.error("Failed to initialize GitHub API wrapper.", e);
return;
}

for (PrNotificationsRecord notification : getAllNotifications()) {
long channelId = notification.getChannelId();
String repositoryOwner = notification.getRepositoryOwner();
String repositoryName = notification.getRepositoryName();

try {
GHRepository repository =
github.getRepository(repositoryOwner + "/" + repositoryName);

if (repository == null) {
logger.info("Failed to find repository {}/{}.", repositoryOwner,
Copy link
Contributor

Choose a reason for hiding this comment

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

warn and consider removing the record from the DB and sending a message in the project chanel

repositoryName);
continue;
}

List<GHPullRequest> pullRequests = repository.getPullRequests(GHIssueState.OPEN);
for (GHPullRequest pr : pullRequests) {
if (pr.getCreatedAt().after(lastExecution)) {
SquidXTV marked this conversation as resolved.
Show resolved Hide resolved
sendNotification(jda, channelId, pr);
}
}
} catch (IOException e) {
logger.error("Failed to send notification for repository {}/{}.", repositoryOwner,
repositoryName, e);
}
}

lastExecution = new Date();
SquidXTV marked this conversation as resolved.
Show resolved Hide resolved
}

private List<PrNotificationsRecord> getAllNotifications() {
return database
.read(context -> context.selectFrom(PrNotifications.PR_NOTIFICATIONS).fetch());
}

private void sendNotification(JDA jda, long channelId, GHPullRequest pr) throws IOException {
ThreadChannel channel = jda.getThreadChannelById(channelId);
if (channel == null) {
logger.info("Failed to find channel {} to send pull request notification.", channelId);
return;
}
channel.sendMessage("New pull request from " + pr.getUser().getLogin() + ".").queue();
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make this a pretty embed. We're missing key details here such as the PR name, description, number, creation date and URL.

}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
CREATE TABLE pr_notifications
(
id INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT,
channel_id BIGINT NOT NULL,
repository_owner TEXT NOT NULL,
repository_name TEXT NOT NULL
)
Loading