Initial email module #2434
Initial email module #2434
Conversation
pbanaszkiewicz
force-pushed
the
feature/2433-email-automation-module
branch
4 times, most recently
from
ae090da
to
67a0cfe
Compare
|
@elichad I prepared this initial PR for the new email module. Please take a look. If the design doesn't make sense, let me know - I can explain. This is, in my opinion, a good start for the new system. It contains 1 email action (for persons merged) and some basic views. I will be working on next steps (stories and functions we need) in upcoming days. |
|
Small update: I was able to pretty easily fix tests related to persons merging, but there are 2 tests still failing. They relate to running a job that calculates |
|
@elichad Ready for your review. |
| active=True, | ||
| id=UUID("5703079f-2f37-4aee-8267-fe6db3eee870"), | ||
| name="Person records are merged", | ||
| signal="persons_merged", |
There was a problem hiding this comment.
Should this be persons_merged_signal?
There was a problem hiding this comment.
No, it's persons_merged (see https://github.com/carpentries/amy/pull/2434/files#diff-fa5a5ef6a674471029d02096c7f7694f7f106b89fa3f175bc5081bb51cf3b3feR33). I can change it to the same name as instance of Signal class, which is defined as persons_merged_signal in https://github.com/carpentries/amy/pull/2434/files#diff-770ab91787786daf35b394421418fd5b7cdcbc526357f6072a098ae9208b7cf0R8, if you think it would be clearer.
There was a problem hiding this comment.
I think it's fine for them to be different. I didn't realise that these are sort of separate things - a string that is used as a key to get the relevant EmailTemplate, and an actual Signal, which are only connected by the receiver function.
It would be a bit easier to follow if there was some sort of Signal.name attribute, that could be passed through to the email template instead.
There was a problem hiding this comment.
Unfortunately there's no such thing as Signal.name, I checked. I may add a custom field, or perhaps subclass Signal to achieve this result.
|
@elichad thank you for the review! I changed almost everything as you suggested. Please re-review. |
|
@elichad Please take a look when you have a chance. Thanks in advance! |
pbanaszkiewicz
force-pushed
the
feature/2433-email-automation-module
branch
from
a34bfdc
to
8523775
Compare
This includes: * email template * scheduled email * log
Unfortunately according to my tests, the receiver is not being invoked.
pbanaszkiewicz
force-pushed
the
feature/2433-email-automation-module
branch
from
de768db
to
a07f049
Compare
This fixes #2433.