feat: introduce new marketable field #4493
Conversation
hamzawaleed01
force-pushed
the
ENT-9748/introduce-new-marketable-field
branch
3 times, most recently
from
976ab16
to
6aab3f9
Compare
hamzawaleed01
force-pushed
the
ENT-9748/introduce-new-marketable-field
branch
3 times, most recently
from
00cb2c8
to
8c4eef9
Compare
…-9748/introduce-new-marketable-field
hamzawaleed01
force-pushed
the
ENT-9748/introduce-new-marketable-field
branch
from
8c4eef9
to
5bf0947
Compare
| @@ -656,6 +657,25 @@ def test_data(self): | |||
| expected = self.get_expected_data(course_run, request) | |||
| assert serializer.data == expected | |||
|
|
|||
| def test_marketable_status(self): | |||
| # Test for executive education course with in-review status, not marketable, and future start date | |||
There was a problem hiding this comment.
Are all the core permutations of is_marketable_external covered by this test? For example, if an EE course already has is_marketable: True, the property is a pass-thru even for EE, but I'm not sure this is reflected in this test?
Would it make sense to extend it with @ddt.data to account for the different permutations (e.g., different status of "in-review" vs. "unpublished", etc.)?
There was a problem hiding this comment.
oh yes my bad, after the recent changes is_marketable_external property; also need to update the tests.
There was a problem hiding this comment.
Pushed latest changes!
hamzawaleed01
force-pushed
the
ENT-9748/introduce-new-marketable-field
branch
2 times, most recently
from
53395ab
to
062522e
Compare
| # Exec-ed, in-review, marketable, future start date, is_marketable_external | ||
| (CourseType.EXECUTIVE_EDUCATION_2U, CourseRunStatus.InternalReview, False, True, True), |
There was a problem hiding this comment.
This is exactly the same as the case above, I think you wanted marketable to be True here?
There was a problem hiding this comment.
Good catch!
Yes, as per the code comment it was marketable BUT it was failing because it's not possible to have status = in-review and marketable = True, so changing both now.
| # Non-exec ed, in-review, not marketable, future start date, is_marketable_external | ||
| (CourseType.PROFESSIONAL, CourseRunStatus.InternalReview, False, True, False), | ||
| # Non-exec ed, published, marketable, future start date, is_marketable_external | ||
| (CourseType.VERIFIED_AUDIT, CourseRunStatus.Published, True, True, True), |
There was a problem hiding this comment.
Maybe have the last two values set to False just to "prove" that if is_marketable is true, it doesn't matter what the other values are.
There was a problem hiding this comment.
These two CourseType.VERIFIED_AUDIT, CourseRunStatus.Published already should result in is_marketable_external=False but it's still True that proves it's working as expected.
But I think I can also set future date to False and it should still work.
Actually last two values (marketable and is_marketable_external) are evaluated by their definition in model so changing them from ddt.data won't actually update them - but will result in failure of assertions.
hamzawaleed01
force-pushed
the
ENT-9748/introduce-new-marketable-field
branch
from
062522e
to
1ea33f1
Compare
hamzawaleed01
force-pushed
the
ENT-9748/introduce-new-marketable-field
branch
from
1ea33f1
to
1f33105
Compare
hamzawaleed01
requested review from
macdiesel and
iloveagent57
and removed request for
bcitro
There was a problem hiding this comment.
Left a few comments, mostly minor cleanup nits but also a clarifying question around whether the intent is to support all reviewable states (internal review vs. legal review), or if there's a specific/individual reviewable state that should be used (might be worth verifying).
| return self.is_marketable | ||
| if is_exec_ed_course: | ||
| # Check whether external course run is marketable | ||
| return self.in_review and self.is_upcoming() |
There was a problem hiding this comment.
[sanity check/clarification] Are we confident the requirements for this property call for whether the course run is "in_review" regardless of the review state? For example, per the tests, both CourseRunStatus.InternalReview and CourseRunStatus.LegalReview are used as "in_review" statuses; will a future EE variant be considered externally marketable for either review state, or is only 1 of these review states valid for this use case?
I tried to find any notes about the review state documented explicitly in any of the tickets/docs around this work, but wasn't able to find a mention of the exact review state. Perhaps this is something that should be verified by the team who worked on ingesting EE variants, if it hasn't been verified already?
There was a problem hiding this comment.
As per my understanding any of those two review states will be considered "in_review" because in_review itself is a property not a field
@property
def in_review(self):
return self.status in CourseRunStatus.REVIEW_STATES()
@classmethod
def REVIEW_STATES(cls):
return [cls.LegalReview.value, cls.InternalReview.value]
There was a problem hiding this comment.
But I agree it's a good idea to confirm with the relevant team.
hamzawaleed01
force-pushed
the
ENT-9748/introduce-new-marketable-field
branch
from
e4945d4
to
a4bc506
Compare
No description provided.