Email Implementation on local units #2357
Conversation
susilnem
requested review from
Rup-Narayan-Rajbanshi and
thenav56
and removed request for
Rup-Narayan-Rajbanshi
| @@ -0,0 +1,43 @@ | |||
| <p>Dear {{full_name}} {% include "includes/logo.html" %}</p> | |||
There was a problem hiding this comment.
This is weird. What does logo.html introduce? Can this be splitted on multiple lines?
There was a problem hiding this comment.
Yes, this file needs to be refactored with correct styling and structure. Someone from frontend will need to help here.
@susilnem will add a preview view in backend for local develop run which can be used for developer to make required changes.
We will also need to add a proper base template for emails as well
| </p> | ||
| {% endif %} | ||
|
|
||
| {% if regional_admin %} |
There was a problem hiding this comment.
Not sure if the variable name is correct here. Better name could be pending_regional_admin_validation
There was a problem hiding this comment.
They are passed through context variable from this file local_units/management/commands/notify_validators.py
|
|
||
| {% endif %} | ||
|
|
||
| {% if global_admin %} |
There was a problem hiding this comment.
Not sure if the variable name is correct here.
There was a problem hiding this comment.
They are passed through context variable from this file local_units/management/commands/notify_validators.py
notifications/notification.py
Outdated
| server.starttls() | ||
| # NOTE: TLS is disabled for now, for alpha instance | ||
| # server.starttls() |
There was a problem hiding this comment.
Shouldn't this also be configured through the environment variable?
There was a problem hiding this comment.
Let's add and use EMAIL_USE_TLS here (set True as default value to avoid changing production logic)
| queryset_for_regional_validtors = LocalUnit.objects.filter( | ||
| validated=False, is_deprecated=False, created_at__date=timezone.now() - timedelta(days=14) | ||
| ) | ||
|
|
||
| # Global Validators: 28 days | ||
| queryset_for_global_validators = LocalUnit.objects.filter( | ||
| validated=False, is_deprecated=False, created_at__date=timezone.now() - timedelta(days=28) | ||
| ) |
There was a problem hiding this comment.
We should use greater than or equal to comparison instead of is equal to for the dates
| self.stdout.write(self.style.NOTICE("Notifying the validators...")) | ||
|
|
||
| # Regional Validators: 14 days | ||
| queryset_for_regional_validtors = LocalUnit.objects.filter( |
There was a problem hiding this comment.
Let's use date threshold filter and track it using database (one field last_sent_validator_type)
Also
| queryset_for_regional_validtors = LocalUnit.objects.filter( | |
| queryset_for_regional_validators = LocalUnit.objects.filter( |
local_units/tasks.py
Outdated
|
|
||
|
|
||
| @shared_task | ||
| def send_local_unit_email(local_unit_id: int, users_details: list, new: bool = True): |
local_units/tasks.py
Outdated
| for email, first_name, last_name in users_details: | ||
| # NOTE: Adding the validator email to the context | ||
| email_context["validator_email"] = email | ||
| email_context["full_name"] = f"{first_name} {last_name}" |
local_units/tasks.py
Outdated
|
|
||
|
|
||
| @shared_task | ||
| def send_revert_email(local_unit_id: int, user_id: int, reason: str = ""): |
There was a problem hiding this comment.
Let's use local_unit.created_by instead of user_id
For other tasks as well
| def get_email_context(instance): | ||
| from local_units.serializers import PrivateLocalUnitSerializer | ||
|
|
||
| local_unit_data = PrivateLocalUnitSerializer(instance).data |
There was a problem hiding this comment.
Let's add a NOTE on why we are using PrivateLocalUnitSerializer here
| serializer.save() | ||
| self.perform_update(serializer) | ||
| deprecated_reason = serializer.validated_data["deprecated_reason_overview"] | ||
| transaction.on_commit(lambda: send_deprecate_email(instance.id, instance.created_by_id, deprecated_reason)) |
There was a problem hiding this comment.
Let's use instance.deprecated_reason from within the task and remove it from here
| email_subject = "Action Required: Local Unit Pending Validation" | ||
| email_type = "Local Unit" | ||
|
|
||
| for region_admin_validator in get_region_admins(local_unit): |
There was a problem hiding this comment.
Let's add try catch... Only update success sends
|
|
||
| {% endif %} | ||
|
|
||
| {% if global_admin %} |
There was a problem hiding this comment.
They are passed through context variable from this file local_units/management/commands/notify_validators.py
| </p> | ||
| {% endif %} | ||
|
|
||
| {% if regional_admin %} |
There was a problem hiding this comment.
They are passed through context variable from this file local_units/management/commands/notify_validators.py
| @@ -0,0 +1,43 @@ | |||
| <p>Dear {{full_name}} {% include "includes/logo.html" %}</p> | |||
There was a problem hiding this comment.
Yes, this file needs to be refactored with correct styling and structure. Someone from frontend will need to help here.
@susilnem will add a preview view in backend for local develop run which can be used for developer to make required changes.
We will also need to add a proper base template for emails as well
Addresses
Changes
Checklist
Things that should succeed before merging.
Release
If there is a version update, make sure to tag the repository with the latest version.