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

Display admin email in error message for password reminder #965

Conversation

petermaksymo
Copy link
Contributor

To solve #626

Let me know if this is needed for other error messages of if there is a cleaner way to implement it. For example, not sure if it is needed for this message.

Copy link
Member

@almet almet left a comment

Choose a reason for hiding this comment

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

I added a few comments that needs to be addressed before we can merge. It makes me think that we should probably create a helper for these messages, and display the admin email in multiple places, for the different error messages. I feel this would help to tidy a bit the codebase : we currently have the same try-sending-email-or-display-error logic in multiple places.

Also, it makes me wonder if we should add a note in the documentation about this admin email setting, saying that it will be a public email used to receive help support.

ihatemoney/web.py Outdated Show resolved Hide resolved
ihatemoney/web.py Outdated Show resolved Hide resolved
ihatemoney/web.py Outdated Show resolved Hide resolved
ihatemoney/web.py Outdated Show resolved Hide resolved
@petermaksymo
Copy link
Contributor Author

Thanks for the feedback! I'll probably work on these changes and update the PR on Wednesday.

@petermaksymo
Copy link
Contributor Author

I've changed the default admin email as discussed above and refactored the error message logic into a helper function. I'm not too sure on what to name a new environment variable to replace the use of ALLOW_PUBLIC_PROJECT_CREATION in the helper.

@almet
Copy link
Member

almet commented Dec 16, 2021

Maybe something like SHOW_ADMIN_EMAIL? Don't forget to document it 👍

docs/configuration.md Outdated Show resolved Hide resolved
@almet
Copy link
Member

almet commented Dec 16, 2021

Thanks, that works for me. I'm not sure about the fact that this means that all deployed instance will get their admin email shown to the public by default without a notice to them.

I also feel that adding this in the version notes should probably be enough to warn them. What do you think @zorun and @Glandos?

Copy link
Member

@Glandos Glandos left a comment

Choose a reason for hiding this comment

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

Some suggested changes to make the code more pythonic

ihatemoney/utils.py Outdated Show resolved Hide resolved
ihatemoney/utils.py Outdated Show resolved Hide resolved
@Glandos
Copy link
Member

Glandos commented Dec 20, 2021

@almet commented on 17 déc. 2021, 00:19 UTC+1:

Thanks, that works for me. I'm not sure about the fact that this means that all deployed instance will get their admin email shown to the public by default without a notice to them.

I also feel that adding this in the version notes should probably be enough to warn them. What do you think @zorun and @Glandos?

Well, to be honest, it's quite easy to get this value already: just create a dummy project, and you should get an email. If you don't have it, try to reset the password. If you still don't have it, then the value doesn't really matter, since the email configuration is probably invalid.

By the way, do you think we could import MAIL_DEFAULT_SENDER from default_settings? It could be better for tests and main code I guess.

@Glandos Glandos merged commit a5452cc into spiral-project:master Jan 30, 2022
@Glandos
Copy link
Member

Glandos commented Jan 30, 2022

Thanks for your work!

TomRoussel pushed a commit to TomRoussel/ihatemoney that referenced this pull request Mar 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants