feat: [EDXOLDMNG-236] certificate config for old courses is taken fro… #2466

NiedielnitsevIvan · 2022-12-09T12:53:30Z

Description: Use external signatures from credentials on WEB/PDF certificate rendering

Youtrack: Link to Youtrack ticket

Reviewers:

Merge checklist:

All reviewers approved
CI build is green
Documentation in source code updated
All related Confluence documentation is updated (including deployment documentation)
Commits are squashed

Post merge:

Delete working branch (if not needed anymore)

…m credentials

NikolayBorovenskiy · 2022-12-12T17:31:01Z

lms/djangoapps/certificates/utils.py

 import logging

 from django.conf import settings
+from django.contrib.auth.models import User  # lint-amnesty, pylint: disable=imported-auth-user


Please tell me what's wrong with the import that you need the # lint-amnesty, pylint: disable=imported-auth-user. Very looks like a regular django's import.

It's all about OedX linters. It crashes with such imports, but it is widely used.

weird but fine.

NikolayBorovenskiy · 2022-12-12T17:31:29Z

lms/djangoapps/certificates/utils.py

@@ -250,3 +253,24 @@ def get_preferred_certificate_name(user):
        name_to_use = ''

    return name_to_use
+
+
+def get_certificate_configuration_from_credentials(course_id, mode):


some typing here?

NikolayBorovenskiy · 2022-12-12T17:32:22Z

lms/djangoapps/certificates/utils.py

+
+def get_certificate_configuration_from_credentials(course_id, mode):
+    """
+    Makes a request to the credentials service to get the certificate configuration.


get a certificate configuration of the course

NikolayBorovenskiy · 2022-12-12T17:34:46Z

lms/djangoapps/certificates/utils.py

+    credentials_client = get_credentials_api_client(User.objects.get(username=settings.CREDENTIALS_SERVICE_USERNAME))
+    course_certificates_api_url = urljoin(f'{get_credentials_api_base_url()}/', 'course_certificates/')
+    api_url = urljoin(course_certificates_api_url, f'?course_id={course_id}&certificate_type={mode}')
+    response = None


No need to have it.

NikolayBorovenskiy · 2022-12-12T17:35:16Z

lms/djangoapps/certificates/utils.py

+            mode
+        )
+
+    return response


else: return response

NikolayBorovenskiy · 2022-12-12T17:36:44Z

lms/djangoapps/certificates/views/webview.py

-    get_preferred_certificate_name
+    get_preferred_certificate_name,
+    get_certificate_configuration_from_credentials,


alphabet order please.

NikolayBorovenskiy · 2022-12-12T17:39:21Z

lms/djangoapps/certificates/views/webview.py

@@ -669,6 +678,29 @@ def render_html_view(request, course_id, certificate=None):  # pylint: disable=t
        return response


+def _populate_certificate_configuration_from_credentials(configuration, course, mode):


Have you noticed that major part of the modules' functions have names def _update_? Please, track the style and follow it.

Need some typing?

The name fixed and typing added.

NikolayBorovenskiy · 2022-12-12T17:40:18Z

lms/djangoapps/certificates/views/webview.py

+    """
+    Populates certificate configuration from credentials service.
+
+    If config not found in credentials returns received.


Do not understand what you want to say.

NikolayBorovenskiy · 2022-12-12T17:45:33Z

lms/djangoapps/certificates/views/webview.py

+    response = get_certificate_configuration_from_credentials(course.id, mode)
+    if response and response.status_code == status.HTTP_200_OK:


bad approach. Please try to use try/except here. Make a log if you need and make a decision after.

NikolayBorovenskiy · 2022-12-12T18:04:26Z

lms/djangoapps/certificates/views/webview.py

+        configuration['course_title'] = data.get('title') or configuration['course_title']
+        configuration['is_active'] = data.get('is_active') or configuration['is_active']


if data.get('title'): configuration['course_title'] = data['title'] if data.get('is_active'): configuration['is_active'] = data['is_active']

?

I thought it looked better this way. Fixed

…icate configuration

NikolayBorovenskiy · 2022-12-14T08:38:58Z

lms/djangoapps/certificates/utils.py

+        response = credentials_client.get(api_url)
+    except Exception:   # pylint: disable=broad-except
+        log.warning(
+            'There is no certificate configuration in credentials for a course [%s] with a mod [%s]',


You can't say that at all. After all, you do not understand whether there is a certificate or not. Maybe the network is down or the service is not responding. Everything is much more complicated here.

And the second important question. What will you do when there are problems with the network? You then return NONE. Is this normal for your logic?

Refactored.
If the response is None, the current configuration will remain from mongo.

NikolayBorovenskiy · 2022-12-14T08:42:10Z

lms/djangoapps/certificates/utils.py

+def get_certificate_configuration_from_credentials(course_id: CourseKey, mode: str) -> Optional[Response]:
+    """
+    Makes a request to the credentials service to get a certificate configuration of the course.
+    """
+    course_certificates_api_url = urljoin(f'{get_credentials_api_base_url()}/', 'course_certificates/')
+    api_url = urljoin(course_certificates_api_url, f'?course_id={course_id}&certificate_type={mode}')
+
+    try:
+        credentials_client = get_credentials_api_client(
+            User.objects.get(username=settings.CREDENTIALS_SERVICE_USERNAME)
+        )
+        response = credentials_client.get(api_url)
+    except Exception:   # pylint: disable=broad-except
+        log.warning(
+            'There is no certificate configuration in credentials for a course [%s] with a mod [%s]',
+            course_id,
+            mode
+        )
+    else:
+        return response


Please answer me why u do not use the function get_api_data?

Because I didn't know it existed.

NikolayBorovenskiy · 2022-12-14T08:42:53Z

lms/djangoapps/certificates/views/webview.py

@@ -669,6 +679,37 @@ def render_html_view(request, course_id, certificate=None):  # pylint: disable=t
        return response


+def _update_certificate_configuration_from_credentials(
+    configuration: Dict[str, Union[str, List[Dict[str, str]]]],
+    course: 'CourseBlockWithMixins', mode: str


course: 'CourseBlockWithMixins', mode: str

NikolayBorovenskiy · 2022-12-14T08:45:30Z

lms/djangoapps/certificates/views/webview.py

+        response = get_certificate_configuration_from_credentials(course.id, mode)
+        response.raise_for_status()
+    except Exception as err:   # pylint: disable=broad-except
+        log.warning(f'Failed to get course certificate config for course {course.id} from Credentials. {err}')


But you already have the log

log.warning( 'There is no certificate configuration in credentials for a course [%s] with a mod [%s]',

Why do we need this log?
Maybe in that function, the log is not needed and there is no need to intercept anything. Let it throw an exception. But in this place, you will intercept, if that?

Removed after using get_api_data.

NikolayBorovenskiy · 2022-12-14T08:45:48Z

lms/djangoapps/certificates/views/webview.py

+        log.warning(f'Failed to get course certificate config for course {course.id} from Credentials. {err}')
+    else:
+        data = response.json()
+


extra line.

…icate configuration

NikolayBorovenskiy · 2022-12-15T12:30:08Z

lms/djangoapps/certificates/utils.py

+    except User.DoesNotExist:
+        log.warning(f'Credentials service user [{settings.CREDENTIALS_SERVICE_USERNAME}] does not exist')


Please describe why we need this and why we can return nothing?

Because in this way you can find out why it is not possible to make a request to Credentials. I had a case on another project, when it was precisely because of the absence of this user that certificates for programs were not generated and there were no logs about this.

Refactored, according to an already existing implementation https://github.com/raccoongang/edx-platform/blob/master/openedx/core/djangoapps/credentials/utils.py#L112

NiedielnitsevIvan added 2 commits December 9, 2022 14:47

feat: [EDXOLDMNG-236] certificate config for old courses is taken fro…

fc81603

…m credentials

style: [EDXOLDMNG-236] disable pylint issues

0ba8909

NikolayBorovenskiy requested changes Dec 12, 2022

View reviewed changes

refactor: [EDXOLDMNG-236] refactored the logic for getting the certif…

e844442

…icate configuration

NikolayBorovenskiy self-requested a review December 14, 2022 08:32

NikolayBorovenskiy requested changes Dec 14, 2022

View reviewed changes

refactor: [EDXOLDMNG-236] refactored the logic for getting the certif…

0d060fa

…icate configuration

NiedielnitsevIvan requested a review from NikolayBorovenskiy December 14, 2022 15:30

NikolayBorovenskiy reviewed Dec 15, 2022

View reviewed changes

fix: [EDXOLDMNG-236] remove extra try except

f0ad2d5

NikolayBorovenskiy self-requested a review December 19, 2022 11:21

NikolayBorovenskiy approved these changes Dec 19, 2022

View reviewed changes

NikolayBorovenskiy merged commit 7039eb1 into master Dec 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: [EDXOLDMNG-236] certificate config for old courses is taken fro… #2466

feat: [EDXOLDMNG-236] certificate config for old courses is taken fro… #2466

NiedielnitsevIvan commented Dec 9, 2022 •

edited by UvgenGen

Loading

NikolayBorovenskiy Dec 12, 2022

NiedielnitsevIvan Dec 13, 2022

NikolayBorovenskiy Dec 14, 2022

NikolayBorovenskiy Dec 12, 2022

NiedielnitsevIvan Dec 13, 2022

NikolayBorovenskiy Dec 12, 2022

NiedielnitsevIvan Dec 13, 2022

NikolayBorovenskiy Dec 12, 2022

NiedielnitsevIvan Dec 13, 2022

NikolayBorovenskiy Dec 12, 2022

NiedielnitsevIvan Dec 13, 2022

NikolayBorovenskiy Dec 12, 2022

NiedielnitsevIvan Dec 13, 2022

NikolayBorovenskiy Dec 12, 2022 •

edited

Loading

NiedielnitsevIvan Dec 13, 2022

NikolayBorovenskiy Dec 12, 2022

NiedielnitsevIvan Dec 13, 2022

NikolayBorovenskiy Dec 12, 2022

NiedielnitsevIvan Dec 13, 2022

NikolayBorovenskiy Dec 12, 2022

NiedielnitsevIvan Dec 13, 2022

NikolayBorovenskiy Dec 14, 2022

NikolayBorovenskiy Dec 14, 2022

NiedielnitsevIvan Dec 14, 2022

NikolayBorovenskiy Dec 14, 2022

NiedielnitsevIvan Dec 14, 2022

NikolayBorovenskiy Dec 14, 2022

NikolayBorovenskiy Dec 14, 2022

NiedielnitsevIvan Dec 14, 2022

NikolayBorovenskiy Dec 14, 2022

NiedielnitsevIvan Dec 14, 2022

NikolayBorovenskiy Dec 15, 2022

NiedielnitsevIvan Dec 15, 2022

NiedielnitsevIvan Dec 16, 2022

		@@ -669,6 +678,29 @@ def render_html_view(request, course_id, certificate=None): # pylint: disable=t
		return response


		def _populate_certificate_configuration_from_credentials(configuration, course, mode):

		response = get_certificate_configuration_from_credentials(course.id, mode)
		if response and response.status_code == status.HTTP_200_OK:

		configuration['course_title'] = data.get('title') or configuration['course_title']
		configuration['is_active'] = data.get('is_active') or configuration['is_active']

		except User.DoesNotExist:
		log.warning(f'Credentials service user [{settings.CREDENTIALS_SERVICE_USERNAME}] does not exist')

feat: [EDXOLDMNG-236] certificate config for old courses is taken fro… #2466

feat: [EDXOLDMNG-236] certificate config for old courses is taken fro… #2466

Conversation

NiedielnitsevIvan commented Dec 9, 2022 • edited by UvgenGen Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikolayBorovenskiy Dec 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiedielnitsevIvan commented Dec 9, 2022 •

edited by UvgenGen

Loading

NikolayBorovenskiy Dec 12, 2022 •

edited

Loading