Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

fix: fix dormancy notifications#14029

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
BrunoQuaresma merged 16 commits intomainfrombq/fix-dormancy-template
Jul 29, 2024
Merged

Conversation

BrunoQuaresma
Copy link
Collaborator

@BrunoQuaresmaBrunoQuaresma commentedJul 26, 2024•
edited
Loading

Dormancy templates have been updated for better readability

var bodyTmpl string
err := sql.
QueryRow("SELECT body_template FROM notification_templates WHERE id = $1 LIMIT 1", tc.id).
Copy link
CollaboratorAuthor

Choose a reason for hiding this comment

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

I've inlined this query because it's only used for tests now. I don't think the overhead of adding a new SQLC query would be justifiable. I actually tried that, and I encountered a few accounting errors because the query wasn't being used in the non-test codebase.

dannykopping reacted with thumbs up emoji

if _, err := s.NotificationsEnqueuer.Enqueue(ctx, workspace.OwnerID, notifications.TemplateWorkspaceAutobuildFailed,
map[string]string{
"name": workspace.Name,
"initiator": initiator,
Copy link
CollaboratorAuthor

Choose a reason for hiding this comment

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

I found out thatTemplateWorkspaceAutobuildFailed was not using the initiator label in the template body so I just removed it.

@BrunoQuaresma
Copy link
CollaboratorAuthor

Unfortunately, those tests don't prevent wrong labels from being used or required labels from being missed. To make that happen, the notifications tests would have to use the database. I think it would be valid but not in the scope for this PR.

@BrunoQuaresmaBrunoQuaresma marked this pull request as ready for reviewJuly 26, 2024 18:44
Copy link
Contributor

@dannykoppingdannykopping left a comment

Choose a reason for hiding this comment

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

We need a bit more attention to detail here please.
I think we should look into your idea of golden files for these email templates; we're missing too many mistakes without them. Could you add an issue for that?

BrunoQuaresmaand others added7 commitsJuly 29, 2024 10:12
Co-authored-by: Danny Kopping <danny@coder.com>
Co-authored-by: Danny Kopping <danny@coder.com>
Co-authored-by: Danny Kopping <danny@coder.com>
Co-authored-by: Danny Kopping <danny@coder.com>
@BrunoQuaresma
Copy link
CollaboratorAuthor

Issue created for tests:coder/internal#32

Copy link
Contributor

@dannykoppingdannykopping left a comment

Choose a reason for hiding this comment

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

Almost there!
I don't need to re-review once you've address the one remaining comment
Thanks@BrunoQuaresma

BrunoQuaresma reacted with heart emoji
@BrunoQuaresmaBrunoQuaresma merged commit58b810f intomainJul 29, 2024
29 checks passed
@BrunoQuaresmaBrunoQuaresma deleted the bq/fix-dormancy-template branchJuly 29, 2024 14:20
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsJul 29, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@dannykoppingdannykoppingdannykopping approved these changes

@mtojekmtojekAwaiting requested review from mtojek

Assignees

@BrunoQuaresmaBrunoQuaresma

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

reword the dormant template notification
2 participants
@BrunoQuaresma@dannykopping
[8]ページ先頭
©2009-2025 Movatter.jp