-
Notifications
You must be signed in to change notification settings - Fork 878
feat(coderd): add company logo when available for email notifications #14935
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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
27 commits
Select commit
Hold shift + click to select a range
a42f108
feat(notifications): add company logo url when available for email no…
defelmnq 1fa1271
feat(notifications): adapt tests with new labels
defelmnq 2def52e
fix: revert changes on generated code version
defelmnq 1b1a4c4
feat(notification): move logo_url and app_name logic to helpers funct…
defelmnq 779260e
chore: remove unused GetLogoURL method from notifications store inter…
defelmnq 1e6899f
fix: add better context and timeout for db related queries
defelmnq b552267
chore: lint
defelmnq 73e07e9
feat(notifications): improve logo_url and app_name fetching moving it…
defelmnq 70e23ae
feat(notifications): improve logo_url and app_name fetching moving it…
defelmnq 2f620c9
feat(notifications): improve logo_url and app_name fetching moving it…
defelmnq fdcdf7b
feat(notifications): improve logo_url and app_name fetching moving it…
defelmnq 9c6c105
feat(coderd): regenerate golden files
0c131a5
feat(notifications): working on tests for logo and app_name
defelmnq 34d6611
feat(notifications): change helpers logic to be defined in the Dispat…
0a9a66a
feat(notifications): add golden file for custom appearance
bf558cb
feat(notifications): add golden file for custom appearance
a420f37
feat(notifications): work on tests
defelmnq 51d8d33
feat(notifications): add golden file for custom appearance
defelmnq d492d09
feat(notifications): add golden file for custom appearance
defelmnq 0c9c485
feat(notifications): add comment ent in tests for enterprise feature
defelmnq a6d4a0c
feat(coderd): remove unused call
defelmnq 4c5cb3d
fix(notifications): improve tests and some nit fixes
defelmnq e419431
chore(retry): improve retry policy on fetcher
defelmnq a7fec66
Merge remote-tracking branch 'origin/main' into feat/notif-custom-logo
defelmnq 8b766f6
improve errors handling
defelmnq 157e086
improve errors handling
defelmnq 790ff33
improve errors handling
defelmnq File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
feat(notification): move logo_url and app_name logic to helpers funct…
…ions
- Loading branch information
commit 1b1a4c413d4b33918e06e1f7163d49b86dd06cd2
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps passing a context to
templateHelpers
(server lifetime) would be a good idea, and perhaps we should define a maximum execution time here (context.WithTimeout) so that we don't end up in a situation where the DB query simply blocks for an indefinite amount of time.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
I have similar feelings. We might be risking by placing DB call inside a template helper. I'm wondering if we shouldn't implement something similar to AppearanceFetcher for the site handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed thanks, good catch ! 🙏
As these methods seem directly connected to templating engine from go , I've not been able to find a way to pass a context to it. Anyway, I created a context with a hardcoded 1 second timeout for it , to at least make it safer.
Not sure if there's a better way to do ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you say if we slightly modify this concept. My main concern is around swallowing the error in favor of returning an empty string
""
, so:Wherever we render the
htmlBody
:we create a dynamic
template.FuncMap
withapp_name
andlogo_url
. We could add a reference to the store toSMTPHandler
, and retrieve values before rendering.Thoughts?