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

feat(coderd/notifications): improve notification format consistency#14967

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
dannykopping merged 20 commits intomainfromjjs/consistent-notification-format
Oct 9, 2024

Conversation

SasSwart
Copy link
Contributor

This Pull request addresses the more trivial items in#14893.
These were simple formatting changes that I was able to fix despite limited context.

Some more changes are required for which I will have to dig a bit deeper into how the template contexts are populated. I'm happy to add those to this PR or create a subsequent PR.

@github-actionsGitHub Actions
Copy link

github-actionsbot commentedOct 3, 2024
edited
Loading

All contributors have signed the CLA ✍️ ✅
Posted by theCLA Assistant Lite bot.

@SasSwart
Copy link
ContributorAuthor

I have read the CLA Document and I hereby sign the CLA

cdrcommunity added a commit to coder/cla that referenced this pull requestOct 3, 2024
@SasSwartSasSwart changed the titlefeat(notifications): Improve notification format consistencyfeat(notifications): improve notification format consistencyOct 3, 2024
@SasSwartSasSwart changed the titlefeat(notifications): improve notification format consistencyfeat(migrations): improve notification format consistencyOct 4, 2024
@SasSwartSasSwart marked this pull request as draftOctober 4, 2024 08:34
Copy link
Member

@mtojekmtojek left a comment

Choose a reason for hiding this comment

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

I think you need to regenerate the text samples withmake update-golden-files, or overwrite default forupdateGoldenFiles (false -> true) and rerun test.

Once we have new samples, it will be straightforward to review changes 👍

@mafredri
Copy link
Member

I think you need to regenerate the text samples withmake update-golden-files, or overwrite default forupdateGoldenFiles (false -> true) and rerun test.

We should make the test output this hint on error (i.e. change requires to assert and if test failed, show hint). Right now it's only shown when there are missing files.

mtojek reacted with thumbs up emoji

@SasSwart
Copy link
ContributorAuthor

I think you need to regenerate the text samples with make update-golden-files, or overwrite default for updateGoldenFiles (false -> true) and rerun test.

I was trying to do this for most of Friday. Turns out that the makefile wasn't actually calling the right test to update these. Firstly because it didn't have a job in the makefile for the notifications package, but also because the test wasn't named correctly.

I've pushed a commit that fixes it. We now have updated testdata, which has shown some issues. I'm working on resolving them. Expect updated queries and rendered testdata soon.

@SasSwartSasSwart changed the titlefeat(migrations): improve notification format consistencyfeat(coderd/notifications): improve notification format consistencyOct 5, 2024
@SasSwartSasSwart marked this pull request as ready for reviewOctober 5, 2024 13:02
@SasSwartSasSwart mentioned this pull requestOct 6, 2024
13 tasks
Copy link
Member

@mafredrimafredri left a comment

Choose a reason for hiding this comment

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

Except for missing fmt, these look good to me. Would like to see what@mtojek thinks before merging though.

Copy link
Member

@mtojekmtojek left a comment

Choose a reason for hiding this comment

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

Definitely better than it is now! Thanks 👍

@SasSwart
Copy link
ContributorAuthor

@mtojek@mafredri

Apologies, but I wasn't happy with the standard of the PR. I reworked it after considering some verbal feedback (thanks@mafredri and@defelmnq).

I also figured out how to deal with the rest of#14893.

Please have another look?

Enough has changed that I would have revoked the initial approvals if I could.
I'd particularly appreciate your thoughts on my changes in functions postUser, postFirstUser and oauthLogin

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.

Noticed a few problems in the rendered templates.
Please go through these closely to make sure they are as you're expecting.

The template **bobby-template** was deleted by **rob**.

The template's display name was **Bobby's Template**.
Copy link
Member

Choose a reason for hiding this comment

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

The display name is just a human-readable representation of the template name, so in this case, you don't need to indicate that it is not the real template name.

Just go with:

The templateBobby's Template was deleted byrob.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I wondered about this, but didn't want to assume I could overwrite what was specified.
Will update.

Comment on lines +3 to +5

The newly activated account belongs to **william tables** and was activated by **rob**.
Copy link
Member

Choose a reason for hiding this comment

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

Have we considered a bit different phrase?

The account for bobby has been activated by Rob. This account belongs to William Tables.

User account **bobby** has been deleted.

The deleted account belonged to **william tables** and was deleted by **rob**.
Copy link
Member

Choose a reason for hiding this comment

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

Curious, if we can make notificationsTemplateUserAccountDeleted,TemplateUserAccountCreated,TemplateUserAccountActivated andTemplateUserAccountSuspended sound more natural?

Any ideas,@dannykopping? Should we involve Christin?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I'd be keen for a professional Wordsmith to have a look if we want.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, we should involve@EdwardAngert.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can do this in a follow-up. Let's get this merged.

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.

LGTM modulo the existing nits.

User account **bobby** has been deleted.

The deleted account belonged to **william tables** and was deleted by **rob**.
Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, we should involve@EdwardAngert.

@@ -576,11 +589,24 @@ func (api *API) deleteUser(rw http.ResponseWriter, r *http.Request) {
return
}

apiKey := httpmw.APIKey(r)

accountDeleter, err := api.Database.GetUserByID(ctx, apiKey.UserID)
Copy link
Contributor

Choose a reason for hiding this comment

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

As a follow-up PR, this feels like something that we should inject into the context.

@dannykoppingdannykopping merged commit9d02269 intomainOct 9, 2024
29 checks passed
@dannykoppingdannykopping deleted the jjs/consistent-notification-format branchOctober 9, 2024 22:31
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsOct 9, 2024
Copy link
Member

@mafredrimafredri left a comment

Choose a reason for hiding this comment

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

Few minor things but otherwise LGTM.

@mtojek
Copy link
Member

Re#14967 (comment):

I wondered about this, but didn't want to assume I could overwrite what was specified.
Will update.

@SasSwart@dannykopping Right now the result looks strange. Please take a look below at the notification I received today, it needs more love. I still don't think we need to refer to thedisplay name specifically.

Screenshot 2024-10-12 at 21 18 40

Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@mafredrimafredrimafredri approved these changes

@dannykoppingdannykoppingdannykopping approved these changes

@mtojekmtojekAwaiting requested review from mtojek

Assignees

@SasSwartSasSwart

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@SasSwart@mafredri@mtojek@dannykopping

[8]ページ先頭

©2009-2025 Movatter.jp