Add membership termination reminder mail on move out#791
Add membership termination reminder mail on move out#791lukasjuhrich wants to merge 33 commits intodevelopfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the mail module by splitting a monolithic pycroft/lib/mail.py file into a structured package with separate modules for concepts, configuration, submission, and templates, while also adding a new email template for membership termination reminders on move out.
Key changes:
- Refactored mail module from single file to package structure with separated concerns
- Added new
move_out_reminder.htmlemail template for membership termination reminders - Improved code organization by separating mail concepts, configuration, submission logic, and templates into distinct modules
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pycroft/templates/mail/move_out_reminder.html | New bilingual email template for reminding users about membership termination when their rental contract ends |
| pycroft/lib/mail/templates.py | Extracted template class definitions from monolithic mail.py into dedicated templates module |
| pycroft/lib/mail/submission.py | Extracted mail sending logic including SMTP connection handling and SSL context creation |
| pycroft/lib/mail/config.py | Extracted mail configuration dataclass and environment variable parsing with improved type safety |
| pycroft/lib/mail/concepts.py | Extracted core Mail and MailTemplate classes with compose method moved to Mail class |
| pycroft/lib/mail/init.py | New package init exposing public API and providing send_template_mails helper function |
| pycroft/lib/mail.py | Removed entire monolithic file as part of refactoring to package structure |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if reply_to := self.reply_to or default_reply_to: | ||
| msg["Reply-To"] = reply_to | ||
|
|
||
| print(msg) |
There was a problem hiding this comment.
Debug print statement should be removed or replaced with proper logging. This appears to be leftover debug code that will pollute output in production.
lukasjuhrich
force-pushed
the
user/lukas/move-out-mail
branch
from
4e1d706 to
902fa85
Compare
Ideally, all of this would be typed sufficiently so that an error like this could not happen.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @t.final | ||
| class MoveOutReminder(MailTemplate): | ||
| template = "move_out_reminder.html" | ||
| subject = "Deine ausstehenden Zahlungen // Your due payments" |
There was a problem hiding this comment.
The subject line is incorrect for a move-out reminder. It currently says 'Your due payments' but should reference membership termination. The subject should be something like 'Mitgliedschaft beenden // Terminate membership'.
| subject = "Deine ausstehenden Zahlungen // Your due payments" | |
| subject = "Mitgliedschaft beenden // Terminate membership" |
pycroft/task.py
Outdated
| # TODO move to pycroft.lib.user or somwhere else suitable | ||
| def get_members_with_contract_end_at(date: date): | ||
| # TODO implement | ||
| pass | ||
|
|
||
|
|
||
| # TODO move to pycroft.lib.user or somwhere else suitable |
There was a problem hiding this comment.
Corrected spelling of 'somwhere' to 'somewhere'.
| # TODO move to pycroft.lib.user or somwhere else suitable | |
| def get_members_with_contract_end_at(date: date): | |
| # TODO implement | |
| pass | |
| # TODO move to pycroft.lib.user or somwhere else suitable | |
| # TODO move to pycroft.lib.user or somewhere else suitable | |
| def get_members_with_contract_end_at(date: date): | |
| # TODO implement | |
| pass | |
| # TODO move to pycroft.lib.user or somewhere else suitable |
pycroft/task.py
Outdated
| # TODO implement | ||
| pass |
There was a problem hiding this comment.
This function is called in mail_soon_to_move_out_members but is not implemented. It will return None, causing the mail sending function to receive None instead of an iterable of users, which will result in a runtime error.
| # TODO implement | |
| pass | |
| from pycroft.model.user import User | |
| return session.session.query(User).filter(User.contract_end == date).all() |
This was used somewhere; pytest startup failed otherwise.
1 participant
No description provided.