DO NOT MERGE bulk email work for discussion - #30
Conversation
- Frontend interface in instructor dashboard - New app bulk_email
- Allows user to paste html
- Test that email settings flag works as expected
There was a problem hiding this comment.
I'm pretty sure we're moving away from using this way of mocking courses and using CourseFactory instead. There are a few examples of this floating around, and the definition for it is in common/lib/xmodule/xmodule/modulestore/tests/factories.py . @wedaly would have better information on how to use it and how to improve it if you need it to do more than it currently does.
There was a problem hiding this comment.
I believe @kluo just followed examples he found elsewhere. If you need him to recode in that manner, then he could if need be. I can't tell however if the new method is just desire/preference or the must-do way for some stronger reason (safer? more robust?). I guess before asking someone to recode something I'd like to explain why.
|
I think this is okay for interface/rendering tests, but I think we're a little more concerned about the backend unit testing, especially on the celery task level. |
There was a problem hiding this comment.
Why is this an @task? It looks to me like it's supposed to be run on the webapp side, and it triggers the remote tasks (which seems to me to say that it isn't a task itself).
There was a problem hiding this comment.
This task that fans out all the other tasks itself can take a while to run, so we don't want it to be synchronous on the web app side. For a 50k member MOOC, say, it has to kick off 5k tasks, assuming 10 mails per batch, which is what we used on Class2Go.
There was a problem hiding this comment.
Likely won't need this file, right @kluo? In c2g this was where we did the UI. I presume it is somewhere else now.
I don't believe that the "worker" jobs will use celery to further process their jobs, so changing the function name to remove that comment is appropriate.
… into feature/kluo/bulk-email
4 participants
@cpennington @dianakhuang Could you guys take a look at the tests I have written for the bulk email interface so far? They just check that the email link will display in the instructor dashboard if the settings flag is enabled, otherwise not. Are there other tests you want to see for the interface? It's still a work in progress, but I'd appreciate your input on the interface tests for now. Models.py and tasks.py are still being worked on.