feat: add allocate_assignments method to content_assignments app. #271
Conversation
iloveagent57
force-pushed
the
aed/allocate-from-policy
branch
2 times, most recently
from
03aba6b
to
9c813a1
Compare
| For existing assignment records with a (config, learner, content) combination, this function | ||
| does the following: | ||
| * If the existing record is cancelled or errored, update the existing record state to allocated | ||
| * If the existing record is allocated or accepted, don't do anything with the record |
There was a problem hiding this comment.
Non-blocking product question to ponder: if an existing record is allocated, should a notification be re-sent?
There was a problem hiding this comment.
If the state changes from cancelled or errored, then yes.
| def _create_new_assignments(assignment_configuration, learner_emails, content_key, content_price_cents): | ||
| """ | ||
| Helper to bulk save new LearnerContentAssignment instances. | ||
| """ |
There was a problem hiding this comment.
In the future, is this the function that also kicks off linking/notification tasks?
There was a problem hiding this comment.
Yeah, either here, or do it directly from allocated_assignments() above, calling something like notify_learner() on the updated_assignments and created_assignments records.
| ) | ||
|
|
||
| @classmethod | ||
| def bulk_create(cls, assignment_records): |
There was a problem hiding this comment.
Nit: The naming of this method feels potentially confusing. Callers call model.bulk_create() which calls bulk_create_with_history() which calls model.manager.bulk_create(). The first and last sharing the same name is what's weird for me, since the latter is already well documented. Can you just call this method bulk_create_with_history()?
There was a problem hiding this comment.
Ah good point, I'll change that name in an upcoming PR.
iloveagent57
force-pushed
the
aed/allocate-from-policy
branch
from
9c813a1
to
115d2b7
Compare
ENT-7531