PADV-3091: Add ENABLE_CCX_CERTIFICATES feature#190
PADV-3091: Add ENABLE_CCX_CERTIFICATES feature#190sergivalero20 wants to merge 4 commits intopearson-release/olive.stagefrom
Conversation
| self.grade) | ||
| assert _set_regular_cert_status(self.user, self.course_run_key, self.enrollment_mode, self.grade) is None | ||
|
|
||
| @override_settings(FEATURES={**settings.FEATURES, 'ENABLE_CCX_CERTIFICATES': False}) |
There was a problem hiding this comment.
For readability and organization I think it's better to organize these test cases in their own class and avoid expanding the current one. @sergivalero20
| ) is None | ||
|
|
||
| @override_settings(FEATURES={**settings.FEATURES, 'ENABLE_CCX_CERTIFICATES': False}) | ||
| def test_ccx_blocked_when_flag_disabled(self): |
There was a problem hiding this comment.
I think there is no substantial difference between this test and test_ccx_blocked_by_default. @sergivalero20
| Test that CCX certificates are blocked when ENABLE_CCX_CERTIFICATES is not set (default behavior). | ||
| """ | ||
| with mock.patch(CCX_COURSE_METHOD, return_value=True): | ||
| assert not _can_generate_regular_certificate( |
There was a problem hiding this comment.
These test classes are using UnitTest from the std lib: https://docs.python.org/3/library/unittest.html so I think it's better to create the assertions using assertions method (e.g. https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertEqual) instead of using Pytest-styled assertions. @sergivalero20
| """ | ||
| if _is_ccx_course(course_key): | ||
| if _is_ccx_course(course_key) and not settings.FEATURES.get('ENABLE_CCX_CERTIFICATES', False): | ||
| log.info(f'{course_key} is a CCX course. Certificate cannot be generated for {user.id}.') |
There was a problem hiding this comment.
I think the message should change to highlight this change. @sergivalero20
| mode=self.enrollment_mode, | ||
| ) | ||
|
|
||
| def test_blocked_by_default(self): |
There was a problem hiding this comment.
I guess there is something missing here. You are not testing a CCX course here, you are testing a regular course and a disable ENABLE_CCX_CERTIFICATES, which I think could be misleading in certain ways. So I think you should run these tests with an actual CCX course, not a regular course. @sergivalero20
There was a problem hiding this comment.
Since I was mocking the behavior I decided to use master course, however, you are right!
I will use CCX instead.
@Squirrel18
There was a problem hiding this comment.
Thanks!.
I think you can combine test_status_blocked_by_default and test_blocked_by_default. @sergivalero20
| assert not result['is_active'] | ||
|
|
||
|
|
||
| @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') |
There was a problem hiding this comment.
Why these tests are only valid in the LMS? @sergivalero20
There was a problem hiding this comment.
@Squirrel18 ok, you're right!
The decorator is unnecessary
| mode=self.enrollment_mode, | ||
| ) | ||
|
|
||
| # --- Blocked by default --- |
| """ | ||
| Test that CCX certificate status cannot be set when flag is not set. | ||
| """ | ||
| self.assertIsNone(_set_regular_cert_status( |
There was a problem hiding this comment.
Isn't _can_set_regular_cert_status the one we should be testing it? @sergivalero20 and I guess the assertion is assertIsFalse, which the actual value returned.
| Test that CCX certificates are blocked when ENABLE_CCX_CERTIFICATES is not set (default behavior). | ||
| """ | ||
| self.assertFalse(_can_generate_regular_certificate( | ||
| self.user, self.ccx_key, self.enrollment_mode, self.grade |
| Test that CCX certificate status cannot be set when flag is not set. | ||
| """ | ||
| self.assertIsNone(_set_regular_cert_status( | ||
| self.user, self.ccx_key, self.enrollment_mode, self.grade |
| Test that CCX certificates are blocked when ENABLE_CCX_CERTIFICATES is explicitly False. | ||
| """ | ||
| self.assertFalse(_can_generate_regular_certificate( | ||
| self.user, self.ccx_key, self.enrollment_mode, self.grade |
| # --- Allowed when flag enabled --- | ||
|
|
||
| @override_settings(FEATURES={**settings.FEATURES, 'ENABLE_CCX_CERTIFICATES': True}) | ||
| def test_can_generate_when_flag_enabled(self): |
There was a problem hiding this comment.
I think you are missing _can_set_regular_cert_status. @sergivalero20
| Test that CCX certificates can be generated when ENABLE_CCX_CERTIFICATES is True. | ||
| """ | ||
| self.assertTrue(_can_generate_regular_certificate( | ||
| self.user, self.ccx_key, self.enrollment_mode, self.grade |
| """ | ||
| Test that certificate status can be set for CCX courses when ENABLE_CCX_CERTIFICATES is True. | ||
| """ | ||
| GeneratedCertificateFactory( |
There was a problem hiding this comment.
Is this part of an integration test? @sergivalero20
| self.user, regular_key, self.enrollment_mode, self.grade | ||
| )) | ||
|
|
||
| @override_settings(FEATURES={**settings.FEATURES, 'ENABLE_CCX_CERTIFICATES': True}) |
There was a problem hiding this comment.
Are these tests necessary? you only change: _can_generate_regular_certificate and _can_set_regular_cert_status, so I don't think these are necessary for a unit test. @sergivalero20
| # Default enrollment mode for CCX courses when ENABLE_CCX_CERTIFICATES is True. | ||
| # Must be a certificate-eligible mode (anything except 'audit'). | ||
| # Common choices: 'honor', 'no-id-professional', 'professional', 'verified'. | ||
| CCX_DEFAULT_ENROLLMENT_MODE = 'honor' |
There was a problem hiding this comment.
Please, use the standard way to document new settings or feature toggles: https://docs.openedx.org/projects/edx-toggles/en/latest/how_to/documenting_new_feature_toggles.html
Ticket
Description
Added a feature flag ENABLE_CCX_CERTIFICATES to allow certificate generation for CCX (Custom Courses for edX) courses. By default, edx-platform blocks certificate generation for all CCX courses. This feature flag enables operators to opt-in to CCX certificate support without modifying the blocking logic permanently.
Changes Made
How to test
Certificate template configured at org level
cert_html_view_enabled = True
HTML certificates enabled globally (CERTIFICATES_HTML_VIEW = True)
Copy and parte following lines in the Django Shell:
After firing, verify the certificate was created: