feat: add CRUD support for RestrictedCourseRun in CourseRun Api #4331
Conversation
zawan-ila
force-pushed
the
anawaz/prod-4004
branch
from
4c7cf36 to
84633ac
Compare
|
Overall, the PR looks good. I have not had a chance to test it out on the local yet. |
course_discovery/apps/api/v1/tests/test_views/test_course_runs.py
Outdated
Show resolved
Hide resolved
| if hasattr(official_obj, "restricted_run"): | ||
| official_obj.restricted_run.delete() | ||
| elif restriction_type: | ||
| RestrictedCourseRun.everything.update_or_create( |
There was a problem hiding this comment.
q: do we need data_modified_timestamp related pre_save signal against this model?
There was a problem hiding this comment.
I am not sure. Mostly(exclusively?) we would want this to be set during creation so later modifications are not expected. Hence, this is effectively a "read only field". Thus, I decided against adding this.
I am happy to add this if you think it is important/useful.
| def test_patch_restriction_type(self, is_restricted, patch_data, changed_restriction_value): | ||
| self.mock_patch_to_studio(self.draft_course_run.key) | ||
| self.mock_ecommerce_publication() | ||
|
|
||
| if is_restricted: | ||
| RestrictedCourseRunFactory(course_run=self.draft_course_run, restriction_type='custom-b2c', draft=True) | ||
| url = reverse('api:v1:course_run-detail', kwargs={'key': self.draft_course_run.key}) | ||
|
|
||
| response = self.client.patch(url, patch_data, format='json') | ||
| assert response.status_code == 200, f"Status {response.status_code}: {response.content}" | ||
|
|
||
| self.draft_course_run.refresh_from_db() | ||
| assert hasattr(self.draft_course_run, 'restricted_run') == bool(changed_restriction_value) | ||
| if changed_restriction_value: | ||
| assert self.draft_course_run.restricted_run.restriction_type == changed_restriction_value | ||
| assert RestrictedCourseRun.everything.count() == 1 | ||
| else: | ||
| assert RestrictedCourseRun.everything.count() == 0 | ||
|
|
||
| # Publish the course run and verify that official versions | ||
| # of RestrictedCourseRuns are created | ||
| self.draft_course_run.status = CourseRunStatus.InternalReview | ||
| self.draft_course_run.save() | ||
| assert CourseRun.objects.filter(key=self.draft_course_run.key, draft=False).count() == 0 | ||
| response = self.client.patch(url, {'status': 'reviewed'}, format='json') | ||
| assert response.status_code == 200, f"Status {response.status_code}: {response.content}" |
There was a problem hiding this comment.
Generally speaking, this unit test is way too bulky and has many conditionals in it. It makes it difficult to figure out the behavior. It would be good to split this test into two tests (Whilst maintaining ddt behavior) to capture the behavior in a clean way.
There was a problem hiding this comment.
The test is indeed bulky. I sorta like it though. I had a quick look above and below this, and found multiple similarly bulky tests. I have added a docstring to make the intent of the test clear. Let me know if it is still confusing.
There was a problem hiding this comment.
Even if there are existing bulky tests, that does not mean we should follow the convention. Ideally, the slimmer the tests, the better.
There was a problem hiding this comment.
Even if there are existing bulky tests, that does not mean we should follow the convention
What I wanted to hint at is that perhaps it's not "bad" to have somewhat large tests.
Splitting this out into multiple tests will need setting the state up (draft and non-draft versions of the restrictions etc.) in each of these and would lead to 3-4 tests instead of 1. I am unconvinced that trying to parse those 3-4 tests instead of this 1 (especially after the docstring update) would be any less mentally taxing than it is currently.
There was a problem hiding this comment.
Hmmm. While I still think the test is doing much more than it should be, the draft/non-draft weirdness can be a vouch for having this bulky test. Ideally, if we plan to test a behavior end-to-end, it is better to have segmented/separate unit tests for each part of interaction/behavior.
course_discovery/apps/api/v1/tests/test_views/test_course_runs.py
Outdated
Show resolved
Hide resolved
course_discovery/apps/api/v1/tests/test_views/test_course_runs.py
Outdated
Show resolved
Hide resolved
zawan-ila
force-pushed
the
anawaz/prod-4004
branch
from
80f34c2 to
f254ffa
Compare
zawan-ila
force-pushed
the
anawaz/prod-4004
branch
from
53bfe02 to
b1a6b37
Compare
PROD-4004
This PR adds support for marking CourseRuns as restricted through the CourseRun Api
Testing Instructions:
You can test it by hitting the API directly with postman or any other tool and provide the
restriction_typeparameter. Then verify that the RestrictedCourseRun objects are added/updated appropriately in the database.Here's how I have been doing it though.
window.restriction_typeto whatever restriction_type you want to set.