feat: normalize start/end/enroll_by dates across content types #666
Conversation
pwnage101
force-pushed
the
pwnage101/ENT-7545
branch
5 times, most recently
from
55d2016
to
a0b163e
Compare
| # Exec ed provides the start/end dates in additional_metadata, so we should copy those over to the keys that | ||
| # we use (inside the advertised course run). |
There was a problem hiding this comment.
Note about this if-block: Prior to this PR, it ran BEFORE merging the full metadata from /api/v1/courses/ into the local metadata object, but now it runs AFTER merging full metadata. The additional_metadata key is not present in the /api/v1/search/all/ response, only in /api/v1/courses/, so the only way this code worked was by running the task twice on a given course.
Old behavior:
- Load basic metadata from
/api/v1/search/all/. - Attempt to copy start/end from additional_metdata (fails because
additional_metdatadoesn't exist yet). - Merge full metadata from
/api/v1/courses/(addsadditional_metdata). - Load basic metadata from
/api/v1/search/all/. - Attempt to copy start/end from additional_metdata (success).
New behavior:
- Load basic metadata from
/api/v1/search/all/. - Merge full metadata from
/api/v1/courses/(addsadditional_metdata). - Attempt to copy start/end from additional_metdata (success).
| course_run = _get_course_run_by_uuid(json_meta, course_run_uuid) | ||
| if course_run is not None: | ||
| course_run_meta = metadata_by_key.get(course_run.get('key')) | ||
| if hasattr(course_run_meta, 'json_metadata'): | ||
| course_run_meta.json_metadata.update({'start': start_date, 'end': end_date}) | ||
| modified_content_metadata_records.append(course_run_meta) |
There was a problem hiding this comment.
This section is being removed because it is a no-op. Course runs are never processed in this task.
| # add aggregation_key and uuid to course objects since they should now exist | ||
| # after merging the original json_metadata with the course metadata | ||
| course_data_1.update(metadata_1.json_metadata) | ||
| course_data_2.update(metadata_2.json_metadata) | ||
| course_data_1.update({'aggregation_key': 'course:fakeX'}) | ||
| course_data_2.update({'aggregation_key': 'course:testX'}) |
There was a problem hiding this comment.
This is more of the testing pattern we're trying to move away from. It's like doing "x = y; assert y = x". Instead we should test each expected key manually.
pwnage101
force-pushed
the
pwnage101/ENT-7545
branch
6 times, most recently
from
6f081f6
to
f8a0f2d
Compare
| }, | ||
| ) | ||
| @ddt.unpack | ||
| def test_find_best_mode_seat(self, seats, expected_seat): |
There was a problem hiding this comment.
FYI: added this test since last review.
pwnage101
force-pushed
the
pwnage101/ENT-7545
branch
2 times, most recently
from
09026ec
to
3a5bf68
Compare
pwnage101
force-pushed
the
pwnage101/ENT-7545
branch
from
3a5bf68
to
1b28f87
Compare
| 'additional_metadata': { | ||
| 'start_date': '2023-03-01T00:00:00Z', | ||
| 'end_date': '2023-04-09T23:59:59Z', | ||
| } |
There was a problem hiding this comment.
Remove additional_metadata from the starter course metadata to properly simulate an object freshly seeded by /api/v1/search/all/ via a different task.
| """ | ||
|
|
||
|
|
||
| class CourseMode(): |
There was a problem hiding this comment.
[nit/inform]: Similar course mode constants are also defined under enterprise_catalog/apps/catalog/constants.py (e.g., CONTENT_COURSE_TYPE_ALLOW_LIST). We could likely consolidate to ensure that those hardcoded course modes also rely on this new CourseMode class.
That said, we might want to define this constant under the constants.py file under the catalog app, so they can be more readily shared across both the api and catalog apps.
I wouldn't say this is blocking feedback on this PR; likely something I can tackle in ENT-7548, which relates to ensuring these normalized dates get into Algolia so they're available across all course types.
ENT-7545
Ticket Link
[enterprise-catalog] Expose net-new fields on the existing
content-metadataAPIPost-review