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

🐛 Fix email sending issue missing "From" field #2926

Merged
merged 9 commits into from
Mar 30, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 13 additions & 15 deletions services/director-v2/tests/unit/test_modules_project_networks.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class MockedCalls(BaseModel):
attach: List[Any]


class TestCase(BaseModel):
class Example(BaseModel):
Copy link
Member

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? 🤣

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true ..

existing_networks_with_aliases: NetworksWithAliases
new_networks_with_aliases: NetworksWithAliases
expected_calls: MockedCalls
Expand All @@ -38,7 +38,7 @@ def using(
new: Dict[str, Any],
detach: List[Any],
attach: List[Any],
) -> "TestCase":
) -> "Example":
return cls(
existing_networks_with_aliases=NetworksWithAliases.parse_obj(existing),
new_networks_with_aliases=NetworksWithAliases.parse_obj(new),
Expand All @@ -62,12 +62,10 @@ def _network_name(number: int) -> str:


@pytest.fixture
def test_case_factory(
mock_scheduler: AsyncMock, project_id: ProjectID
) -> List[TestCase]:
def examples_factory(mock_scheduler: AsyncMock, project_id: ProjectID) -> List[Example]:
return [
# nothing exists
TestCase.using(
Example.using(
existing={},
new={
_network_name(1): {
Expand All @@ -90,7 +88,7 @@ def test_case_factory(
],
),
# with existing network, remove node 2
TestCase.using(
Example.using(
existing={
_network_name(1): {
_node_id(1): _node_alias(1),
Expand All @@ -111,7 +109,7 @@ def test_case_factory(
attach=[],
),
# remove node 2 and add node 2 with different alias
TestCase.using(
Example.using(
existing={
_network_name(1): {
_node_id(1): _node_alias(1),
Expand Down Expand Up @@ -139,7 +137,7 @@ def test_case_factory(
],
),
# nothing happens when updates with the same content
TestCase.using(
Example.using(
existing={
_network_name(1): {
_node_id(1): _node_alias(1),
Expand Down Expand Up @@ -227,20 +225,20 @@ def mock_docker_calls(mocker: MockerFixture) -> Iterable[Dict[str, AsyncMock]]:
async def test_send_network_configuration_to_dynamic_sidecar(
mock_scheduler: AsyncMock,
project_id: ProjectID,
test_case_factory: List[TestCase],
examples_factory: List[Example],
mock_docker_calls: Dict[str, AsyncMock],
) -> None:
for test_case in test_case_factory:
for example in examples_factory:

await _send_network_configuration_to_dynamic_sidecar(
scheduler=mock_scheduler,
project_id=project_id,
new_networks_with_aliases=test_case.new_networks_with_aliases,
existing_networks_with_aliases=test_case.existing_networks_with_aliases,
new_networks_with_aliases=example.new_networks_with_aliases,
existing_networks_with_aliases=example.existing_networks_with_aliases,
)

mock_scheduler.assert_has_calls(test_case.expected_calls.attach, any_order=True)
mock_scheduler.assert_has_calls(test_case.expected_calls.detach, any_order=True)
mock_scheduler.assert_has_calls(example.expected_calls.attach, any_order=True)
mock_scheduler.assert_has_calls(example.expected_calls.detach, any_order=True)


async def test_get_networks_with_aliases_for_default_network_is_json_serializable(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ def setup_login_storage(app: web.Application):
def _setup_login_options(app: web.Application):
settings: SMTPSettings = get_email_plugin_settings(app)

cfg = settings.dict(exclude_unset=True)
cfg = settings.dict()
if INDEX_RESOURCE_NAME in app.router:
cfg["LOGIN_REDIRECT"] = f"{app.router[INDEX_RESOURCE_NAME].url_for()}"

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,12 @@ class LoginOptions(BaseModel):
LOGIN_REDIRECT: str = "/"
LOGOUT_REDIRECT: str = "/"

SMTP_SENDER: Optional[str] = None
SMTP_SENDER: str
SMTP_HOST: str
SMTP_PORT: int
SMTP_TLS_ENABLED: bool = False
SMTP_USERNAME: Optional[str] = None
SMTP_PASSWORD: Optional[SecretStr] = None
SMTP_TLS_ENABLED: bool
SMTP_USERNAME: Optional[str] = Field(...)
SMTP_PASSWORD: Optional[SecretStr] = Field(...)
Comment on lines +49 to +50
Copy link
Member

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?

Copy link
Member

@pcrespov pcrespov Mar 29, 2022

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

as a reminder

Copy link
Contributor Author

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


# lifetime limits are in days
REGISTRATION_CONFIRMATION_LIFETIME: PositiveFloat = 5 * _DAYS
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ def flash_response(msg: str, level: str = "INFO") -> web.Response:

async def send_mail(app: web.Application, msg: MIMEText):
cfg: LoginOptions = get_plugin_options(app)
log.debug("Email configuration %s", cfg)

msg["From"] = cfg.SMTP_SENDER
smtp_args = dict(
Expand Down
23 changes: 23 additions & 0 deletions services/web/server/tests/unit/isolated/test_login_plugin.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# pylint: disable=unused-argument
import os
from typing import Any, Dict

from settings_library.email import SMTPSettings
from simcore_service_webserver.login.plugin import LoginOptions


def test_smtp_settings(mock_env_devel_environment: Dict[str, Any]):

settings = SMTPSettings()

cfg = settings.dict(exclude_unset=True)

for env_name in cfg:
assert env_name in os.environ

cfg = settings.dict()

config = LoginOptions(**cfg)
print(config.json(indent=1))

assert config.SMTP_SENDER is not None