Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nborovesnkiy/edxoldmng 218/copy certificate configurations including signatures to credentials on certificate update #2464

Conversation

NikolayBorovenskiy
Copy link

@NikolayBorovenskiy NikolayBorovenskiy commented Nov 26, 2022

Description: This PR is going to implement the feature Copy certificate configurations including signatures to credentials on certificate update

Youtrack: Link to Youtrack ticket

Configuration instructions: The raccoongang/openedx-events#1 has to be merged first.

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

credentials_api_base_url = get_credentials_api_base_url()
api_url = urljoin(f'{credentials_api_base_url}/', 'course_certificates/')
payload = attr.asdict(certificate_config)
payload['course_key'] = str(payload.pop('course_key'))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here too.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

credentials_api_base_url = get_credentials_api_base_url()
api_url = urljoin(f'{credentials_api_base_url}/', 'course_certificates/')
payload = attr.asdict(certificate_config)
payload['course_key'] = str(payload.pop('course_key'))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change it to course_id pls. Because the API takes the course_id argument by default.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree and I've fixed it already here https://github.com/raccoongang/edx-platform/pull/2465/files. Please look it also. This PR is going to be merged together with the current one.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's ok that you pass course_key to CertificateConfigData here, but in RP https://github.com/raccoongang/edx-platform/pull/2465/files you pass course_id?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not understand the question a little bit. There is no already course_key at all. Only we get now course_id. I've changed it everwhere. Please explain what do u mean.)

@NikolayBorovenskiy NikolayBorovenskiy force-pushed the nborovesnkiy/EDXOLDMNG-218/copy-certificate-configurations-including-signatures-to-credentials-on-certificate-update branch from 486e716 to 764b931 Compare November 30, 2022 19:32
@UvgenGen
Copy link

UvgenGen commented Dec 2, 2022

@NikolayBorovenskiy Main case for failed tests is ImportError: cannot import name 'CertificateConfigData' from 'openedx_events.content_authoring.data'. Also take a look at linter errors, please.

@NikolayBorovenskiy NikolayBorovenskiy force-pushed the nborovesnkiy/EDXOLDMNG-218/copy-certificate-configurations-including-signatures-to-credentials-on-certificate-update branch from 6fea164 to 4801b07 Compare December 6, 2022 11:03
…rtificate configs to credentials (#2465)

* refactor: [EDXOLDMNG-224] Moves course certificate configuration creation and deletion to separate functions.

* feat: [EDXOLDMNG-218] uses course id instead of course key for CertificateConfigData container

* test: [EDXOLDMNG-224] updates SignalCourseCertificateConfigurationListenerTestCase tests in order refactoring the approach to send data to Credentials via http.

* test: [EDXOLDMNG-224] creates tests for course certificate configuration credentials utils apis.

* feat: [EDXOLDMNG-224] Creates manage.py command to migrate course certificate coniguration.

* refactor: [EDXOLDMNG-224] code polishing for migrate cert config command.

* docs: [EDXOLDMNG-224] adds doc string for migrate_cert_config command's main class
@NikolayBorovenskiy NikolayBorovenskiy merged commit 454c787 into master Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants