feat: backpopulate transaction email and content title #181
Conversation
| csod_only_filter = Q(integrated_channel_code='CSOD') | ||
| for items_batch in batch_by_pk(ContentMetadataItemTransmission, extra_filter=csod_only_filter): | ||
| for item in items_batch: |
There was a problem hiding this comment.
Nit: can we use more generic example model names, or make them not specific to integrated channels in here?
|
|
||
| def generate_transaction_reversal_idempotency_key(fulfillment_uuid, enrollment_unenrolled_at): | ||
| """ | ||
| Generates a unique idempotency key for a transaction reversal using the fulfillment uuid and time at which the | ||
| unenrollment occurred. | ||
| """ | ||
| return f'unenrollment-reversal-{fulfillment_uuid}-{enrollment_unenrolled_at}' | ||
|
|
||
|
|
||
| def batch_by_pk(ModelClass, extra_filter=Q(), batch_size=10000): |
| lms_user_email = subsidy.email_for_learner(item.lms_user_id) | ||
| content_title = subsidy.title_for_content(item.content_key) |
There was a problem hiding this comment.
nit: maybe only fetch these if the field is null - you can theoretically have records here where one of the two fields is non-null.
| parser.add_argument( | ||
| '--dry-run', | ||
| action='store_true', | ||
| dest='dry_run', | ||
| help=( | ||
| 'If set, no updates will occur; will instead log ' | ||
| 'the actions that would have been taken.' | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Consider whether we want to optionally skip internal-only subsidies (or maybe skip those by default).
There was a problem hiding this comment.
We might have test subsidy records for say, performance testing, where there's lots and lots of test transaction records. We might want the ability to have this command ignore test subsidy records, so we don't have to wait on those being updated.
| logger.info("Running in dry-run mode. No updates will occur.") | ||
|
|
||
| incomplete_only_filter = Q(lms_user_email__isnull=True) | Q(content_title__isnull=True) | ||
| for items_batch in batch_by_pk(Transaction, extra_filter=incomplete_only_filter): |
There was a problem hiding this comment.
Nit: batch_by_pk is really cool. OTOH, you could loop over Subsidies first to do some level of batching for you, and then the sequence of work would have some level of predictability, and you'd only need to fetch each subsidy record once. It also opens the door for us to do bulk fetches (filtered by the related subsidy record's enterprise uuid) in the future.
johnnagro
force-pushed
the
johnnagro/ENT-7974/0
branch
from
4a48f5f to
7d76db4
Compare
| logger.info(f"Processing {subsidy.uuid} transaction {txn.uuid}") | ||
|
|
||
| if txn.lms_user_email is None: | ||
| lms_user_email = subsidy.email_for_learner(txn.lms_user_id) |
There was a problem hiding this comment.
Do we want to make these calls if self.dry_run is true? I could go either way - on the one hand, it's nice to set --dry-run just to see what would be updated, and then have that happen relatively quickly. On the other hand, it's nice to know with a dry run if we're actually fetching the data we expect.
There was a problem hiding this comment.
i think we wanna be able to see that these API calls work without actually saving the results
| for txns in batch_by_pk(Transaction, extra_filter=txn_filter): | ||
| for txn in txns: | ||
| self.process_transaction(subsidy, txn) |
There was a problem hiding this comment.
Love this refactoring
| if txn.lms_user_email is None: | ||
| lms_user_email = subsidy.email_for_learner(txn.lms_user_id) | ||
| txn.lms_user_email = lms_user_email | ||
| logger.info(f"Found {lms_user_email} for transaction {txn.uuid}") |
There was a problem hiding this comment.
nit: log the subsidy uuid in these logs, too
johnnagro
force-pushed
the
johnnagro/ENT-7974/0
branch
from
7d76db4 to
c0e6240
Compare
johnnagro
force-pushed
the
johnnagro/ENT-7974/0
branch
from
c0e6240 to
0ce91e1
Compare
Description
lms_user_emailandcontent_titleforTransactions