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.

Sign up for GitHub

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

Jump to bottom

Show explanation if user cannot add new email #25502

Closed
wants to merge 1 commit into from
Closed

Show explanation if user cannot add new email #25502

wants to merge 1 commit into from

Conversation

n0toose
Copy link
Contributor

@n0toose n0toose commented Jun 25, 2023 •
edited
Loading

This change concerns the "Manage Email Addresses" submenu
in the Account settings.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 25, 2023
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jun 25, 2023
@n0toose
Copy link
Contributor Author

n0toose commented Jun 25, 2023

This PR was created after a request on https://codeberg.org/Codeberg/Community/issues/1085. I could not figure out how to trigger the not .CanAddEmail condition within the limited amount of time I had to tackle this issue, so I tested this without the condition and presumed that the condition is possible to trigger.

image

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jun 25, 2023
techknowlogick
techknowlogick requested changes Jun 25, 2023
View reviewed changes
options/locale/locale_en-US.ini
@@ -688,6 +688,7 @@ emails = Email Addresses
manage_emails = Manage Email Addresses
manage_themes = Select default theme
manage_openid = Manage OpenID Addresses
email_cant_add = You cannot add any new email addresses at this time. Please try again later.
Copy link
Member

Choose a reason for hiding this comment

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

Could you remove the "try again later"? As that may create confusion for the user, and they'll try to refresh the page without knowing why they can't add a new email. From the ticket you linked it sounds like the user hadn't verified their email yet.

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 25, 2023
@wxiaoguang
Copy link
Contributor

I do not think it is right to merge some "unclear" changes into code base.

I will figure out this problem by a new PR.

@wxiaoguang
Copy link
Contributor

I think I understand the problem now.

-> Clarify the reason why the user can't add a new email if there is a pending activation #25509

@techknowlogick
Copy link
Member

Thanks for this PR. I will close it in favour of the other PR.

@n0toose n0toose deleted the cant-add-email-warning branch June 26, 2023 10:03
@n0toose
Copy link
Contributor Author

n0toose commented Jun 26, 2023

I do not think it is right to merge some "unclear" changes into code base.

Absolutely understandable. Sorry for not addressing the previous review sooner, going to blame it on the timezones.

@go-gitea go-gitea locked as resolved and limited conversation to collaborators Sep 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@techknowlogick techknowlogick techknowlogick requested changes

@delvh delvh delvh approved these changes

Assignees
No one assigned
Labels
lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@n0toose @wxiaoguang @techknowlogick @delvh @GiteaBot