-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Take Exec Ed course data from course runs #785
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of comments on conditionals, overall looks pretty good.
enterprise_catalog/apps/api/tasks.py
Outdated
@@ -298,29 +298,23 @@ def _normalize_course_metadata(course_metadata_record): | |||
json_meta = course_metadata_record.json_metadata | |||
normalized_metadata = {} | |||
# For each content type, find the values that correspond to the desired output key. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this comment still relevant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, removed!
enterprise_catalog/apps/api/tasks.py
Outdated
all_seats = advertised_course_run.get('seats', []) | ||
seat = _find_best_mode_seat(all_seats) | ||
if seat: | ||
normalized_metadata['enroll_by_date'] = seat.get('upgrade_deadline') | ||
else: | ||
logger.info(f"No Seat Found for course run '{advertised_course_run.get('key')}'. Seats: {all_seats}") | ||
logger.info(f"No Enrollment End or Seat Found for course run '{advertised_course_run.get('key')}'. Seats: {all_seats}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
entirely small nit, why the funky capitalization? I see it was there before I'm just confused lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was just following along with the old funky capitalization, but went ahead and removed it.
|
||
end_date = None | ||
if additional_md.get('end_date'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol there was just a way better way to write this 🤦
csv_row.append(end_date) | ||
formatted_enroll_by = fetch_and_format_registration_date(additional_md) | ||
adv_course_run = hit.get('advertised_course_run', {}) | ||
if adv_course_run and (start_date := adv_course_run.get('start'), end_date := adv_course_run.get('end')): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you need the if adv_course_run
? It feels like you should be able to do a gettr on an empty dict and have it resolve to false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! Removed.
cbb951f
to
68d09f3
Compare
68d09f3
to
aa9dddd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one option small formatting/syntax nit but otherwise lgtm
enterprise_catalog/apps/api/tasks.py
Outdated
if advertised_course_run is not None: | ||
normalized_metadata['start_date'] = advertised_course_run.get('start') | ||
normalized_metadata['end_date'] = advertised_course_run.get('end') | ||
normalized_metadata['content_price'] = advertised_course_run.get('first_enrollable_paid_seat_price') \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional nit, you could make DEFAULT_NORMALIZED_PRICE
the default value in the advertised_course_run
getter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I don't have anything additional to Alex's comments :)
0cce2dc
to
42ae17a
Compare
…metadata style: fix long line fix: don't use advertised_course_run before it is populated fix: bug caused by typo fix: merge error chore: fix trailing whitespace chore: fix pylint issues chore: fix code style issues
42ae17a
to
65f4dc3
Compare
Description
Updates references for Exec Ed courses originally contained in the
additional_metadata
field, to now be taken from course runs.Ticket Link
ENT-8248: update custom logic for non-ocm courses in enterprise-catalog
Outstanding Questions
Post-review