[FC-0004][RG]/edxoldmng 218/Signals to save/delete certificate configuration for a course. #158

NikolayBorovenskiy · 2022-12-14T10:02:18Z

Description: This pr adds signals to save/delete certificate configuration for a course.

ISSUE: Link to GitHub issue

Reviewers:

@ormsbee

Merge checklist:

Post merge:

Create a tag
Check new version is pushed to PyPI after tag-triggered build is
finished.
Delete working branch (if not needed anymore)

openedx-webhooks · 2022-12-14T10:02:25Z

Thanks for the pull request, @NikolayBorovenskiy! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

mariajgrimaldi · 2022-12-20T11:50:23Z

@mphilbrick211: hello 😄, what should we do with the CLA issue here?

ormsbee · 2022-12-21T04:10:59Z

openedx_events/content_authoring/data.py

+        title (int): signatory title.
+    """
+
+    image = attr.ib(type=BinaryIO)


Can this be a URL instead (image_url)? Signals will get translated to CloudEvent messages, which should be 64K or less. There's no guarantee that a professor's signature will fit in that limit.

This is a very good question and I thought about it.
Why did I do this?
First, this is not the final version yet. What we are doing is an intermediate stage of moving towards where the configuration of certificates, the certificates themselves will be located in Credentials service.
Now the signature files are saved in MongoDB as an asset and passed to Credentials via HTTP. Also, it is passed as an ordinary multipart request where files are sent as bytes. This is the usual approach for sending files. Yes, now we use Event and look to the future, we are thinking about the future architecture, where there is a message Event Bus and events. We are doing our best to make it easy to switch to this, but for now, we are temporarily passing the certificate configuration through the HTTP API. So far, as we agreed before.

For this to be image_url, then, when saving the configuration from Studio UI, you first need to save the file somewhere on a file system or in a cloud and pass it to the Credentials service, and this service itself will also have access to this shared storage. When we do not have Mongo DB for cert configs and signatures, and we keep this in mind, then these certificate configuration files will not be saved there and they need to be passed immediately to Credentials. Then in order for it to be image_url, it must first be saved somewhere in intermediate public storage. But the problem is that I do not yet know what it will be and what architectural solution will be applied. This is not known. Since I wrote at the very beginning that we are at an intermediate stage of transition.  

What you're talking about, where size matters is easy to solve with server-side validation, and avoid where you need to transfer a large file. Or do post-processing of the file before passing this information through the bus. But all this will be decided and a decision will be made on how to do it a little later, at the next stage, so to speak, when more becomes known and when the transfer of the certificate configuration will be completely through EvenBus, and not imitation as we are doing now. That's why for now I propose to leave it as it is, but keep in mind those solutions and problems that I described.

What do you think?

My comments had two assumptions:

Backwards compatibility concerns will require us to continue to support whatever fields are added over the long term. In order to not break plugins in the future, openedx-events opts for a pretty strict conventions around schema evolution.

There is already a publicly accessible image URL representing the signature, since we need to display that signature to users–so we wouldn't have to save the file anywhere.

Dave, I suggest we take a step back if you don't mind. I want us to be on the same page and I'm afraid to take sharp, thoughtless steps.
Since we are talking about implementation, I suggest going to this PR openedx/edx-platform#31446. This is where the implementation is. You will be able to understand what was done and how. We will approve the final decision, and then we will continue our work here.

What do you say?

All right, let's just see how this goes. You can keep the BinaryIO image, but please log how large they get in the code that constructs these things so that we can examine that data after edx.org runs it.

ormsbee · 2022-12-21T04:12:30Z

openedx_events/content_authoring/data.py

+    """
+
+    certificate_type = attr.ib(type=str)
+    course_id = attr.ib(type=str)


In keeping with other conventions in this repo, please call this course_key and make it of type CourseKey.

ormsbee · 2022-12-21T04:17:11Z

openedx_events/content_authoring/data.py

+        is_active (bool): indicates whether the certifivate configuration is active.
+    """
+
+    certificate_type = attr.ib(type=str)


In the docstring, please either enumerate the types possible, or describe where the type field data comes from and give examples.

Good. I'll do it. Thank you.

ormsbee · 2022-12-21T04:18:41Z

openedx_events/content_authoring/signals.py

+COURSE_CERTIFICATE_CONFIG_DELETED = OpenEdxPublicSignal(
+    event_type="org.openedx.content_authoring.course.certificate_config.deleted.v1",
+    data={
+        "certificate_config": CertificateConfigData,


Why do we need to pass all this information if it's been deleted? Can we just pass the course_key instead?

I think we should. Why did I do this? It's all about the low coupling of the two services Studio and Credentials. We have to support it and not depend on each other. How can we efficiently delete resources in general? How do we do it with a regular http request in monolithic systems, i.e. explicitly pass an entity identifier, which is usually the primary key in a database table, or pass some unique field at the database level. Then the removal will be accurate and correct.

It’s not about our case now. Studio does not store the ID and does not know about the model from the Credentials database. It's good. course_key may or may not be a unique field. Studio doesn't know this for sure and shouldn't know. Only Credentials knows this. What can we do then when we keep services loosely coupled? Pass on as much meaningful information as possible and ask for Credentials to be removed. And Credentials itself will decide which record in the table needs to be deleted, since it is an information expert here. Which is what we do.

course_key may or may not be a unique field.

But there's only ever one credentials configuration for a given course_key, right? When would we ever have more than one?

Not certainly in that way. Look at this model please https://github.com/openedx/credentials/blob/master/credentials/apps/credentials/models.py#L194-L230
Here course_id is just a CharField and not unique. And here is our constraint unique_together = (("course_id", "certificate_type", "site")),) which explains the behavior.

And in any case, it's not so important. After all, as I wrote above, Studio is not an information expert for Credentials and does not know the exact URI of this resource. It is important for us to maintain low coupling and sending more data will not be superfluous here.

I see. I misunderstood in that I thought CertificateConfigData was the representation of all certificates for a given course (i.e. a cleaner version of the entire certificates attribute off of the CourseBlock), rather than being the configuration for one particular certificate type for a course.

mphilbrick211 · 2022-12-21T19:23:55Z

@mphilbrick211: hello 😄, what should we do with the CLA issue here?

Thanks for the ping, @mariajgrimaldi! We're looking into this now, and hope to be able to push it through as soon as we can.

mphilbrick211 · 2023-01-06T18:52:41Z

Hi @NikolayBorovenskiy! When you re-run the checks, your CLA should be all set and the check should turn green. Please let me know if you have any questions.

NikolayBorovenskiy · 2023-01-07T17:23:39Z

Hi, @mphilbrick211 looks like that CLA is good now. Thanks for pushing me.

NikolayBorovenskiy · 2023-01-18T12:30:39Z

@ormsbee Dave, I've pushed all we need. Please verify this. Tnx in advance a lot.

ormsbee · 2023-01-27T18:26:51Z

@NikolayBorovenskiy: Please squash and fix the quality error (sort order of imports) and I'll merge.

…n for a course

NikolayBorovenskiy · 2023-01-30T08:56:32Z

@ormsbee Dave, thanks a lot. I've done my part. Please do a final review and if the checkers are fine this time let's merge and make a new tag.

openedx-webhooks · 2023-01-30T15:57:26Z

@NikolayBorovenskiy 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

timmc-edx · 2023-02-02T19:07:43Z

Would you mind making a quick PR to add this to the changelog as well?

timmc-edx · 2023-02-02T19:16:09Z

Actually, if you could just review #180 that would be great -- let me know if that summary is correct or not.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Dec 14, 2022

NikolayBorovenskiy mentioned this pull request Dec 14, 2022

[FC-0004][RG]/edxoldmng 218/copy certificate configurations including signatures to credentials on certificate update openedx/edx-platform#31446

Closed

10 tasks

NikolayBorovenskiy changed the title ~~[RG]/edxoldmng 218/Signals to save/delete certificate configuration for a course.~~ [FC-0004][RG]/edxoldmng 218/Signals to save/delete certificate configuration for a course. Dec 14, 2022

ormsbee requested changes Dec 21, 2022

View reviewed changes

NikolayBorovenskiy force-pushed the nborovesnkiy/EDXOLDMNG-218/copy-certificate-configurations-including-signatures-to-credentials-on-certificate-update branch from 8a46d50 to 559938f Compare December 26, 2022 17:31

mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Dec 27, 2022

e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jan 5, 2023

mphilbrick211 added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Jan 6, 2023

NikolayBorovenskiy force-pushed the nborovesnkiy/EDXOLDMNG-218/copy-certificate-configurations-including-signatures-to-credentials-on-certificate-update branch 4 times, most recently from af232b5 to a2025de Compare January 7, 2023 17:20

mphilbrick211 added changes requested and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Jan 10, 2023

NikolayBorovenskiy force-pushed the nborovesnkiy/EDXOLDMNG-218/copy-certificate-configurations-including-signatures-to-credentials-on-certificate-update branch 2 times, most recently from 24264b7 to 22d4a7a Compare January 13, 2023 14:42

NikolayBorovenskiy requested a review from ormsbee January 13, 2023 19:18

ormsbee approved these changes Jan 17, 2023

View reviewed changes

NikolayBorovenskiy force-pushed the nborovesnkiy/EDXOLDMNG-218/copy-certificate-configurations-including-signatures-to-credentials-on-certificate-update branch from 99877a9 to 4015419 Compare January 18, 2023 13:21

mphilbrick211 removed the changes requested label Jan 23, 2023

ormsbee assigned ormsbee and NikolayBorovenskiy and unassigned ormsbee Jan 27, 2023

feat: [EDXOLDMNG-218] Signals to save/delete certificate configuratio…

c744de0

…n for a course

NikolayBorovenskiy force-pushed the nborovesnkiy/EDXOLDMNG-218/copy-certificate-configurations-including-signatures-to-credentials-on-certificate-update branch from 28ac275 to c744de0 Compare January 30, 2023 08:53

NikolayBorovenskiy removed their assignment Jan 30, 2023

ormsbee merged commit bab0b39 into openedx:main Jan 30, 2023

GlugovGrGlib deleted the nborovesnkiy/EDXOLDMNG-218/copy-certificate-configurations-including-signatures-to-credentials-on-certificate-update branch June 13, 2023 09:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FC-0004][RG]/edxoldmng 218/Signals to save/delete certificate configuration for a course. #158

[FC-0004][RG]/edxoldmng 218/Signals to save/delete certificate configuration for a course. #158

NikolayBorovenskiy commented Dec 14, 2022

openedx-webhooks commented Dec 14, 2022 •

edited

Loading

mariajgrimaldi commented Dec 20, 2022

ormsbee Dec 21, 2022

NikolayBorovenskiy Dec 26, 2022

ormsbee Jan 11, 2023

NikolayBorovenskiy Jan 13, 2023

ormsbee Jan 17, 2023

NikolayBorovenskiy Jan 18, 2023

ormsbee Dec 21, 2022

NikolayBorovenskiy Dec 26, 2022

ormsbee Dec 21, 2022

NikolayBorovenskiy Dec 26, 2022

ormsbee Dec 21, 2022

NikolayBorovenskiy Dec 26, 2022

ormsbee Jan 11, 2023

NikolayBorovenskiy Jan 13, 2023

ormsbee Jan 17, 2023

mphilbrick211 commented Dec 21, 2022

mphilbrick211 commented Jan 6, 2023

NikolayBorovenskiy commented Jan 7, 2023

NikolayBorovenskiy commented Jan 18, 2023

ormsbee commented Jan 27, 2023

NikolayBorovenskiy commented Jan 30, 2023

openedx-webhooks commented Jan 30, 2023

timmc-edx commented Feb 2, 2023

timmc-edx commented Feb 2, 2023

[FC-0004][RG]/edxoldmng 218/Signals to save/delete certificate configuration for a course. #158

[FC-0004][RG]/edxoldmng 218/Signals to save/delete certificate configuration for a course. #158

Conversation

NikolayBorovenskiy commented Dec 14, 2022

openedx-webhooks commented Dec 14, 2022 • edited Loading

mariajgrimaldi commented Dec 20, 2022

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

mphilbrick211 commented Dec 21, 2022

mphilbrick211 commented Jan 6, 2023

NikolayBorovenskiy commented Jan 7, 2023

NikolayBorovenskiy commented Jan 18, 2023

ormsbee commented Jan 27, 2023

NikolayBorovenskiy commented Jan 30, 2023

openedx-webhooks commented Jan 30, 2023

timmc-edx commented Feb 2, 2023

timmc-edx commented Feb 2, 2023

openedx-webhooks commented Dec 14, 2022 •

edited

Loading