bbaga / github-scheduled-reminder-app Goto Github PK

Is your feature request related to a problem? Please describe.
Recently had to migrate from HubSpot's client to the official slack client, it could have been a lot less work if there was an abstraction layer.

Describe the solution you'd like
Create an abstraction to cover the slack functions currently used, so we can hide the actual client's implementation details.

Describe alternatives you've considered
Chances are, there won't be many changes in the future, but still, better to this sooner than later.

Is your feature request related to a problem? Please describe.
When putting in a timezone in the configuration of "EST" once we swap times to EDT it would require a configuration change to keep producing notifications at the same time.

For instance a week ago this would produce notifications at 8am ET

config:
      timezone: "EST"
      schedule: "0 0 8 ? * MON-FRI"

But now notifications are produced at 9am ET

Describe the solution you'd like
Instead of using EST or EDT timezone instead use the Java Zone.id so a user can add
Americas/NewYork and the timezone will always match ET.
https://docs.oracle.com/javase/8/docs/api/java/time/ZoneId.html
A list of options is available on that page.

Describe alternatives you've considered
Some sort of boolean to support Daylight shift? Might be hard to know when DT is active.

Additional context
N/a

Tracking issue for:

• https://github.com/bbaga/github-scheduled-reminder-app/security/code-scanning/1

Currently the schedules are based on the host machine's timezone.
This can be a problem in distributed teams, as this not only requires calculations, but also because different timezones have different dates for daylight savings transitions.

Notification messages can get really long and to reduce noise on the channel we could write the links in a thread.

Currently all the details are sent to the channel and it contains lots of details. We could split this message into one "main" message on the channel and then send the details to a thread under the "main" message.

The "main" message should contain:

• number of open issues
• number of open PRs

The details on the thread should contain:

• Issue/PR title
• Issue/PR link
• etc

Is your feature request related to a problem? Please describe.
Block Kit messages should be removable without channels:history and groups:history scopes. Block Kit messages won't be broken up into multiple messages, therefore remembering the previous message's timestamp should be enough to remove the previous message from the channel.

Describe the solution you'd like
If the message format is block kit, use the timestamp to delete the previous message.

Additional context
The requirement of channles:history and groups:history scopes is not welcomed by security conscious teams.

Describe the bug
Notification job isn't removed from the scheduler when the notification is removed from the configuration file

To Reproduce

• Add notification config
• Wait for the scheduled job to be created
• Remove the notification from the config

Expected behavior
The scheduled job is removed from the scheduler

Additional context
The observed behaviour is:

INFO 1 --- [eduler_Worker-7] c.b.g.a.jobs.ScheduledNotification       : Starting notification job for xxx/yyy
java.lang.NullPointerException
	at com.bbaga.githubscheduledreminderapp.application.jobs.ScheduledNotification.execute(ScheduledNotification.java:52)
	at org.quartz.core.JobRunShell.run(JobRunShell.java:202)
	at org.quartz.simpl.SimpleThreadPool$WorkerThread.run(SimpleThreadPool.java:573)

Version/Tag: 0.11.0-beta.8

search-prs-by-reviewers source should ignore merged PRs and only include ones that are still open.

Code ref: /src/main/java/com/bbaga/githubscheduledreminderapp/domain/sources/github/SearchByReviewersPRsSource.java#L26

Add test to cover this issue: #158

Channels can become overwhelmed by long notification messages when there is little activity on the channel. When this happens, teams start to wonder where to "hide" the notifications which is detrimental to the effectiveness of the reminders.

Proposed solution

An option to delete previous messages from the channel to keep the noise levels at minimum.

Every time a notification is triggered, we'll search the channel for messages from the Slack app's user handle and add them to a delete queue to avoid saturating the API limits imposed by Slack.

Describe the bug
When the search-prs-by-reviewers used to filter for teams and the application doesn't have permission to read org membership information, the search will fail which will cause the notification task to be skipped.

To Reproduce
Remove the the Organization/Members read permission and use the search-prs-by-reviewers source to search for teams.

Expected behavior
Instead of skipping the whole notification, the issue should be logged and the rest of the results should be shown.

Additional context
Documentation should be updated with the required permissions.

Version/Tag: 0.11.0-beta.5

The current documentation revolves around how to configure and get the application running, but doesn't explain how it works.
Documentation needed to clarify how and when repositories discovered, how the internal state is composed.

When running in GKE we can't rely in local disk, storing the state between restarts would be much more convenient.

Describe the bug
Previous messages will not be deleted from private channels, unless the Slack user that generated the app's user token is on said channel.

To Reproduce

• configure a schedule that sends messages to a private channel
• enable previous message deletion
• don't join to the channel with the user that installed the application on the organization

Expected behavior
Previous messages should be deleted regardless whether the user that installed the app is on the channel

Version/Tag: 0.10.0

Is your feature request related to a problem? Please describe.
Yes, in particular our team is closely tracking the GCP cost cutting project and we'd like to see a summary of costs in our daily slack notification.

Describe the solution you'd like
Quite simple at this point, just allow a configurable endpoint. Call this endpoint and receive results in a well known format, say, slack block format: https://api.slack.com/reference/block-kit/blocks.

Extra credit: allow parameters from the config to be passed to the web request so that the web request endpoint could also be configured from the same yaml.

Describe alternatives you've considered
Considered building it ourselves, probably a python service.

The current statistics only show how many clicks there were on a given type (i.e. pull request or issue), it'd be nice to see how many times a PR or issue was clicked.
Note: PR and issue ID are in the same namespace (i.e. there will never be a PR with the same ID as an issue)

Only local disk storage is supported at the moment, and a way to store the configuration in a GCP/GKE deployment is needed.

When. there is no handler configured for an even type in the event dispatcher, the application encounters the following exception:

2021-11-24 11:57:21.540 ERROR 1 --- [nio-8080-exec-4] o.a.c.c.C.[.[.[/].[dispatcherServlet]    : Servlet.service() for servlet [dispatcherServlet] in context with path [] threw exception [Request processing failed; nested exception is java.lang.NullPointerException] with root cause

java.lang.NullPointerException: null
	at com.bbaga.githubscheduledreminderapp.infrastructure.github.webhook.EventPublisher.publishEvent(EventPublisher.java:27) ~[classes!/:0.3.1-SNAPSHOT]

Describe the solution you'd like
Add configuration options for setting the template format for the headers and the individual rows.

Additional context
The current format can take up too much space on slack channels, shrinking it size would be useful for monitoring very active repositories.

Describe the bug
When multiple reviewers are specified, the search query will contain duplicated search terms and will include all the specified usernames. This will result in every username being ignored except one.

To Reproduce
Add multiple usernames to the filter that were asked to review different PRs. The resulting list of PRs will not contain all the expected pull requests.

Expected behavior
Every PR where either one of the users were asked to be a reviewer should be present in the notification.

Version/Tag: <=0.11.0

The readme mentions that tracking can be enabled, but there isn't any documentation on how to access the tracking data.

For the sake of simplicity the current implementation discards notification configurations when there isn't a known parent (schedule) configuration already. This can mean in some cases that a configuration will be added to a parent only on the second re-indexing of an installation.

Since re-indexing of installations run in parallel, a buffer can't be local to the re-indexing jobs.

The expected outcome is that every configuration is mapped to a schedule when an appropriate schedule exists in any of the repositories at indexing time, one re-indexing cycle should be enough to properly set up all the schedules.

The /state/update endpoint updated the internal config only, but the changes aren't reflected in the scheduled jobs.

At the moment, the only way the application discovers new repositories is by the scheduled job the re-indexes the installations. Since that happens every 12 hours or so, that isn't very user friendly, and the application could subscribe to installations events and register every new repository automatically.

Add proper documentation for the notification message template options.

Adding options to filter Issues and PRs would be useful in case a team want to filter items based on labels, assignees etc.
An easy filter option would be to enable/disable the listing of Draft PRs.

The reports are shoving the ID of the reminder that is used internally, but it isn't the repository name, nor the reminder name.

It would be nice to have a direct link to the reminder configuration file.