feat(awards): add slack notifications #31

zhammer · 2024-02-23T23:46:59Z

adds slack notifications when new users receive an award

- icon_emoji and username fields seem to not actually work for the incoming webhook bot - i imagine we may have a `slack.bot` configuration, so isolating `webhook` to its own object

zhammer · 2024-02-24T00:05:46Z

plugins/awards-backend/src/service/router.ts


 export interface RouterOptions {
  identity: IdentityApi;
  database: PluginDatabaseManager;
  logger: Logger;
+  // todo: make required in next breaking change
+  config?: Config;


so i started here by trying to not make a breaking change (and then in the following two fields it was going to be more complicated so i just broke it for now.)

curious what’s the best approach here. right now i imagine we’re the only ones using these packages but we should have a plan for versioning. ideally we would have started major version zero but the semantic release tool explicitly doesn’t allow this: https://semantic-release.gitbook.io/semantic-release/support/faq#can-i-set-the-initial-release-version-of-my-package-to-0.0.1.

some options:

state that we’re treating version 1 as unstable, breaking changes may come in minor versions and we’ll note it in the release notes

just push out a few breaking changes now before anyone’s using the package and hope we’re stable enough after that

switch to using prereleases

curious about your thoughts here @colinodell

or, of course, just follow semver and be ok doing a major version release

eh if we wrap this in with #32 i’m okay with it being a major release

ideally we would have started major version zero but the semantic release tool explicitly doesn’t allow this

I was initially disappointed by this, but their reasoning does make sense. I'd strongly prefer that we avoid intentional BC breaks, since breaking BC tends to break trust. I think we should follow semver (and do a major version release), optionally with some pre-releases first if we need time to test and refine the changes before cutting that stable release.

+1 to following semver. It's more important for releases to be reliable (and changes expected) than for major releases to include a lot of changes.

ok this all sounds good. once these branches are merged i'm going to set up a prelease flow and won't cut a proper release until we've run locally a bit

hm, might just try to cut the release after shipping these changes and save prereleasing for later. seems like it'll be a good amount of work to get right

plugins/awards-backend/src/awards.ts

bbckr · 2024-02-26T15:13:11Z

plugins/awards-backend/README.md

+    slack:
+      webhook:
+        # https://api.slack.com/messaging/webhooks
+        url: <my_slack_webhook_url>


Doesn't the webhook url contain a secret? i think in the documentation here we should advise that it be included as an env var and referenced as such in the config (from what I understand about dynamic includes in backstage)

done here 8d2d2b2

colinodell · 2024-02-26T15:18:27Z

plugins/awards-backend/src/awards.ts

+    notifications: NotificationsGateway,
+    catalogClient: CatalogClient,
+    tokenManager: TokenManager,


These three new dependencies are only used by the notifyNewRecipients function, but now the entire class is untestable unless these are all injected/mocked. Should we decouple this with one additional abstraction layer (perhaps called AwardsNotifier or something) between the awards and the gateway so we can inject a single AwardNotifier and keep that logic isolated from awards management? WDYT?

yep i think that makes sense. AwardsNotifier could also have some smarter logic that i kept out from bloating the Awards class like fanning out to multiple notification gateways and skipping catalog fetching if there are no notification gateways (rather than the nullnotificationgateway)

done in fb4f4f0.. how's that look?

I like it!

like fanning out to multiple notification gateways and skipping catalog fetching if there are no notification gateways (rather than the nullnotificationgateway)

I often like to use this design pattern for those types of things (I don't know what it's called):

export class MultipleGateways implements NotificationsGateway { constructor(private readonly gateways: NotificationsGateway[]) {} async notifyNewRecipientsAdded(award: Award, newRecipients: UserEntity[]): Promise<void> { this.gateways.forEach((gateway) => { gateway.notifyNewRecipientsAdded(award, newRecipients); }); } }

But because you're also skipping the catalog fetching when no gateways are added I think it totally makes sense to implement AwardsNotifier exactly how you did - nicely done!

lol yeah i love that. i think it's the composite pattern?

Award logos are now stored via s3 or local filesystem storage. BREAKING CHANGE: Running the awards-backend plugin now requires a `awards.storage.<s3|fs>` configuration to run. BREAKING CHANGE: The awards-backend router now requires the following three fields from the plugin environment `{ config: Config, discovery: DiscoveryService, tokenManager: TokenManager }`. (Missed note from #31)

github-actions · 2024-02-28T14:19:58Z

🎉 This PR is included in version 1.0.2 🎉