Closed shafiq-muhammad closed 2 years ago
Hi @shafiq-muhammad!
Some considerations:
About adding tests, I found there's a flag debug
that can be sent to MailSender()
so no actual messages are sent (reference). This is used in Scrapy's tests with a callback to catch the message (reference). Maybe that's something we could replicate to add tests for the new SendSmtpEmail()
?
We could add a debug
argument to SendSmtpEmail.__init__()
or SendSmtpEmail.send_message()
, and then pass that flag to MailSender()
. We don't need to test MailSender()
much as that's already handled by Scrapy, we can focus here on making sure we are instantiating it correctly and calling its send method as expected, and that the message gets "delivered", by using that callback or capturing the debug log messages.
Let me know if that helps and ping me for anything else I can assist you with!
I've been checking and the CI started failing because Sphinx's version isn't pinned in setup.py (docs
section), and the latest version 5.0.0 requires that language
variable to be set apparently.
We could pin it (though ideally, the version requirements in setup.py should be as loose as possible), but it seems better to support it instead if the only thing preventing us from doing so is that variable.
@shafiq-muhammad Could you send a separate PR with the language
change? We can merge that one right away.
These changes should be enough for docs/source/conf.py
:
-language = None
+language = "en"
After it gets merged to master you can rebase changes here to get rid of the CI errors.
@shafiq-muhammad Sphinx build problem was fixed in https://github.com/scrapinghub/spidermon/pull/347 . Please rebase your branch so the CI checks will work properly.
Merging #345 (1751238) into master (b7b4e0e) will increase coverage by
0.24%
. The diff coverage is96.87%
.
@@ Coverage Diff @@
## master #345 +/- ##
==========================================
+ Coverage 74.03% 74.28% +0.24%
==========================================
Files 72 73 +1
Lines 3096 3126 +30
Branches 483 487 +4
==========================================
+ Hits 2292 2322 +30
Misses 737 737
Partials 67 67
Impacted Files | Coverage Δ | |
---|---|---|
spidermon/contrib/actions/email/ses.py | 0.00% <0.00%> (ø) |
|
spidermon/contrib/actions/email/__init__.py | 67.39% <100.00%> (ø) |
|
spidermon/contrib/actions/email/smtp.py | 100.00% <100.00%> (ø) |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
This PR solves #337
@rennerocha hi! :) I see you have continued this PR, thank you for your help! Is there anything missing here that we can help with?
Hi @curita ! Yes, I am trying to finish this PR so we can have a new release of Spidermon (I need some new features!). I added a few more settings options (that are available in Scrapy MailSender class) and added the documentation. It is only missing an update on the tests (current tests are testing MailSender, not the action). I should be finishing this by this week.
Thank you for the update and your work here again 🙇♀️ Sounds good then, if you need any help let us know!
Adding the support for SMTP email support