add reporting location #25
Conversation
| SMTP: | ||
| PORT: 465 | ||
| SERVER: "smtp.gmail.com" | ||
| ADDRESS: "my@gmail.com" | ||
| PASSWORD: "PA$$WORD" | ||
| DEFAULT_SENDER: "sender@gmail.com" | ||
| ADMIN_EMAIL: "help.desk@gmail.com" |
There was a problem hiding this comment.
After adding PLUGINS, this should no longer be necessary.
There was a problem hiding this comment.
Yup, I thought about this, but later I got reflection, that we are testing if app starts at all. So should we test if only base part of an app works, or should we test whole app. If we test only a part of it, then how we are sure if someone who will want to use reporting location won't end up with broken app
There was a problem hiding this comment.
Oh. I totally agree that we should test whole app. :)
Yet - there is no e2e test yet (which should be here).
There was a problem hiding this comment.
And summing it up. I'd remove this config and plugin (which are not tested now) and move it to this second issue.
Which should be taken right away.
There was a problem hiding this comment.
It seems that it's still here. Did you forget do commit push?
kacper-cyra
force-pushed
the
feature/add_reporting_location
branch
from
af57224
to
e5c1e3a
Compare
| request_body = request.get_json() | ||
| email_service.send_report_location_email(request_body) | ||
| return "", 200 | ||
| if email_service: |
There was a problem hiding this comment.
I see a big misunderstanding here. What I meant was only to remove it from test config.
As this is an integral part of code.
| @@ -14,7 +14,7 @@ def create_app(config_path): | |||
|
|
|||
| app = platzky.create_app_from_config(config) | |||
| extend_app_db(app) | |||
| app.email_service = EmailService(app.config["SMTP"], app.sendmail) | |||
| app.email_service = EmailService(app.config["SMTP"], app.sendmail) if 'sendmail' in app.config["PLUGINS"] else None | |||
There was a problem hiding this comment.
Same here. Need to discuss it further.
|
Issue was closed with another PR |
Closes #16