Skip to content
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

Add user notifications settings tab #31086

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

rafaelsgirao
Copy link
Contributor

Adds a notification tab to the user's settings page, allowing a user to customize both the e-mail and in-UI notifications they receive.

Additionally, this adds logic to handle UI notification options, similarly to what is already done for e-mail notifications.


This was inspired by #30537.
"Inspired", since we don't implement the requested feature, but instead implement some of the groundwork needed for it, and other notification customization options.

Feedback appreciated! 🙂

Adds a notification tab to the user's settings page,
allowing a user to customize both the e-mail and in-UI notifications
they receive.

Additionally, this commit adds logic to handle UI notification options,
similarly to what is already done for e-mail notifications.
Refs: go-gitea#30537

Co-Authored-By: João Tiago <[email protected]>
Signed-off-by: Rafael Girão <[email protected]>
Signed-off-by: João Tiago <[email protected]>
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 26, 2024
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 26, 2024
@github-actions github-actions bot added modifies/translation modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files labels May 26, 2024
@lunny
Copy link
Member

lunny commented May 27, 2024

Thank you for your contribution. Can you put some screenshots?

@rafaelsgirao
Copy link
Contributor Author

Absolutely. Here's a quick showcase:

Screencast.from.2024-05-28.18-37-24.webm

Regarding your feedback on radio buttons: I can change to that if it's preferred, but I feel that it's more in line with the rest of the User's settings pages this way (I don't think there are any radio buttons there right now).

I think it's also more versatile this way, if a future PR wants to introduce more options/customizability. (We deliberately kept the scope small so to not end up with a very large PR.)

@lunny
Copy link
Member

lunny commented May 29, 2024

Yes, I mean convert the list menu to radio box list is better.

@rafaelsgirao
Copy link
Contributor Author

Thank you for your contribution. Can you put some screenshots?

image

Here's a preview of what it looks like right now. Is this what you had in mind?

@@ -680,6 +680,7 @@ block.list.none = You have not blocked any users.
profile = Profile
account = Account
appearance = Appearance
notifications = Notifications
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have one
image

Copy link
Contributor Author

@rafaelsgirao rafaelsgirao Jun 27, 2024

Choose a reason for hiding this comment

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

I noticed there's already two entries for notifications: notifications and notification.notifications, is there any explicit reason for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a history problem. Now we recommend to use existing translations if it is possible.

@@ -82,6 +83,7 @@ type User struct {
Email string `xorm:"NOT NULL"`
KeepEmailPrivate bool
EmailNotificationsPreference string `xorm:"VARCHAR(20) NOT NULL DEFAULT 'enabled'"`
UINotificationsPreference string `xorm:"VARCHAR(20) NOT NULL DEFAULT 'enabled'"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Migration is missing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you provide some pointers on how this should be done? I looked through some of the migration files/commits there and couldn't really understand what the general flow for creating a new migration is.
Besides the files themselves, I only found this page on the docs, but it only mentions on how to test new migrations.

Copy link
Contributor

@yp05327 yp05327 Jul 1, 2024

Choose a reason for hiding this comment

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

The docs is correct, but no details about how to do it.

What you need to do is that creating a new migration file in models/migrations/{latest version}, and the file name should follow the order of other files.
Then add your newly added migration function in migrations file:
https://github.com/go-gitea/gitea/blob/main/models/migrations/migrations.go#L593
Then it will be auto executed after Gitea server started.

ctx.Data["PageIsSettingsNotifications"] = true

// Set Email Notification Preference
if ctx.FormString("_method") == "EMAIL" {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be simplified, as almost all codes are the same.

@yp05327
Copy link
Contributor

yp05327 commented Sep 17, 2024

Docs was moved to https://gitea.com/gitea/docs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/docs modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/translation size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants