-
Notifications
You must be signed in to change notification settings - Fork 27
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
🐛 Fix email sending issue missing "From" field #2926
Conversation
services/web/server/src/simcore_service_webserver/login/plugin.py
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #2926 +/- ##
========================================
+ Coverage 78.4% 79.6% +1.2%
========================================
Files 677 677
Lines 28230 28231 +1
Branches 3282 3282
========================================
+ Hits 22136 22479 +343
+ Misses 5346 4992 -354
- Partials 748 760 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
|
services/web/server/src/simcore_service_webserver/login/settings.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
marvelous
@@ -26,7 +26,7 @@ class MockedCalls(BaseModel): | |||
attach: List[Any] | |||
|
|||
|
|||
class TestCase(BaseModel): | |||
class Example(BaseModel): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example?? really? is there no better name? 🤣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true ..
SMTP_USERNAME: Optional[str] = Field(...) | ||
SMTP_PASSWORD: Optional[SecretStr] = Field(...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the Field(...)
really needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sanderegg yes, this way the field is required, which is what will solve the bug we had.
Check
class Settings(BaseSettings): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it means that the fields need to be provided even if the provided value is None
What do these changes do?
The "From" field was not found during the sending of the email. Turns out the wrong parameter to parse the pedantic model was used.
This fixes the issue and allows for emails to get sent once again.
Extra:
Related issue/s
How to test
Checklist