Closed WilliamMayor closed 1 year ago
Hello @WilliamMayor 👋
Thank you for your contribution, much appreciated 🙏 I'll make a review!
Thanks for the review @frankie567. I'm on holiday at the moment, but I'll make your suggested changes when I get back :)
Sure! Enjoy your holiday 🏖
Hello @WilliamMayor 👋
Just wanted to check-in to know if you'll still have time to complete this 🙂 If not, that's not a problem, it's already a good base for me to work on!
Cheers!
@frankie567 Yes! It's still on my todo list, very much so. Sorry for the huge delay. I've been swamped with work these past couple of months. Give me a week to try to carve out some time to work on this. I understand if you'd prefer to get it finished up though :)
No problem, @WilliamMayor! No rush ;)
@frankie567 Hey! I've merged upstream into my fork to fix the merge conflicts. I've then gone through and added your suggestions. The only one I couldn't resolve was the magic mock for the smtp library.
Let me know whaat you think :)
That's very good work, @WilliamMayor, thank you! Just two small things before we can merge:
just lint
) to fix code formatting@frankie567 Looks like our versions of black are arguing :) (I'll probably just need to update mine)
That's all done :)
Great, let's merge! Looking forward to try with a real SMTP server :)
@all-contributors add @WilliamMayor for code
@frankie567
I've put up a pull request to add @WilliamMayor! :tada:
For the record, I've tested it with a real SMTP server and it work very well! I only had to fix a small thing in the following commit: https://github.com/fief-dev/fief/commit/2fced3b40d81132256370ae12b8061a93ff9d6b0
We were assuming the text/plain
version was always available, which is not the case right now (we only have HTML versions). So, it raised an error.
But apart from that, all good 👍
Hey. Something that @frankie567 mentioned in a discussion. A generic email provider would be useful. I've made a start.
It's probably not feature complete yet. It only works with TLS enabled servers that require login (so hopefully that's most of the scenarios). I don't think it would be too much work to extend this to support other setups (which setups would we want to support?)
I've added tests, but I'm new the code base, so I may have made some misteps, let me know :)
I'll admit that I didn't get a full blown version of the service up and running on my machine, so I haven't tested it end to end. I found just getting the tests running fiddly enough, I'll work on getting a dev version running soon.
Any and all feedback/criticism greatfully received :)