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 custom activation email message #10959

Merged
merged 1 commit into from
Jan 4, 2021

Conversation

jdavcs
Copy link
Member

@jdavcs jdavcs commented Dec 17, 2020

Fixes #10607.

Is this sufficient?

@jdavcs jdavcs added kind/feature area/configuration Galaxy's configuration system labels Dec 17, 2020
@jdavcs jdavcs added this to the 21.01 milestone Dec 17, 2020
@hexylena
Copy link
Member

@dickgroenenberg is this fine for your needs?

@dickgroenenberg
Copy link

Yes, very convenient. Thanks for adding this feature Sergey and Helena.

@hexylena
Copy link
Member

hexylena commented Jan 4, 2021

@ic4f should there be a test for this function? If there was I'd be happy to hit merge.

@jmchilton jmchilton merged commit 9b5526a into galaxyproject:dev Jan 4, 2021
@jdavcs
Copy link
Member Author

jdavcs commented Jan 4, 2021

@ic4f should there be a test for this function? If there was I'd be happy to hit merge.

@hexylena no, it doesn't have a test, and you're right - it should have a simple unit test (same for the enclosing method, send_activation_email, which needs several). I've added it to my testing todo list; if you don't mind I'll take care of this later. And you are completely right - little trivial changes like this one are good triggers for refactoring, adding tests, and cleaning up the code base.

@hexylena
Copy link
Member

hexylena commented Jan 4, 2021

@ic4f sure, no worries, no time pressure for sure. I was just remembering marius suggesting a while back that reviewers should ask for more tests and thought it might not be a huge burden here. :)

@jmchilton
Copy link
Member

Opps - sorry I really didn't even see that comment. I was just going through and clicking merge on stuff that had stalled out and looked fine to me. Happy to also merge a test 😆

@jdavcs
Copy link
Member Author

jdavcs commented Jan 4, 2021

I think I also brought up the same point at that discussion - asking for more tests on such PRs. Now, this is embarrassing :) I'll add the test of course! Thanks, @hexylena , @jmchilton

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/configuration Galaxy's configuration system kind/feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Request] modify / personalize the verification email (send to new Galaxy users)
4 participants