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

[FC-0004] API to manage certificate signatures #1837

Conversation

NiedielnitsevIvan
Copy link

@NiedielnitsevIvan NiedielnitsevIvan commented Nov 29, 2022

Description: This PR implements the CourseCertificateViewSet extension feature to save certificate signatories along with the configuration.
The "get" method has been added to get data about the configuration of certificates and their signatories.
Added a method to "delete" deleting the configuration of certificates and their signatories.

Ticket: #1765

@openedx-webhooks
Copy link

openedx-webhooks commented Nov 29, 2022

Thanks for the pull request, @NiedielnitsevIvan! 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.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Nov 29, 2022
@NiedielnitsevIvan NiedielnitsevIvan changed the title Ivan niedielnitsev/edxoldmng 200/extend course certificate view set in credentials to include certificate signatures [FC-0004] API to manage certificate signatures Nov 30, 2022
@NiedielnitsevIvan NiedielnitsevIvan force-pushed the Ivan_Niedielnitsev/EDXOLDMNG-200/Extend-CourseCertificateViewSet-in-Credentials-to-include-certificate-signatures branch from 6c8913c to 627d209 Compare November 30, 2022 09:53
@NiedielnitsevIvan NiedielnitsevIvan marked this pull request as draft December 2, 2022 11:17
@NiedielnitsevIvan NiedielnitsevIvan force-pushed the Ivan_Niedielnitsev/EDXOLDMNG-200/Extend-CourseCertificateViewSet-in-Credentials-to-include-certificate-signatures branch 3 times, most recently from 37f66b6 to 653e734 Compare December 7, 2022 15:08
@NiedielnitsevIvan
Copy link
Author

During development, we had a misunderstanding related to the current database architecture.
CourseCertificate has a many-to-many relationship with Signatory, so it's not obvious how signatories should be saved. Based on the fact that the Signatory model does not have unique fields, there was a problem with updating existing signatories. Because if within one certificate configuration there is more than one signature with the same pair of name and title on the credentials service side, it is not known which particular signatory needs to be updated.
Our solution is to work with a given relationship as one-to-many and each time create new signatories and clean up existing relationships. Such an implementation solves the problem described above, but leads to the fact that signatories that do not belong to any configuration appear in the database.

@NiedielnitsevIvan NiedielnitsevIvan marked this pull request as ready for review December 7, 2022 15:18
@e0d
Copy link

e0d commented Dec 8, 2022

@hurtstotouchfire I want to make sure this is on the radar for scheduling and prioritization.

@cdeery
Copy link
Contributor

cdeery commented Jan 17, 2023

We can't take this PR at this time. We (Aperture) have discovered that we need to make some product decisions about how we need to modify the schema and system to work as the system of record, but we don't have that information right now. @hurtstotouchfire and @e0d should discuss. In the meantime, the short term solution is probably moving the Old-Mongo data into the new Mongo, so when we do migrate to Credentials, it can all come from one place.

@ormsbee
Copy link

ormsbee commented Jan 18, 2023

@cdeery: it can't really be moved into Split because the branching of "is this Old Mongo or is it Split Mongo" is the course key type, and those courses don't exist in Split (and can't exist in Split without a lot of work).

As long as we allow this certificate information to be edited by Studio (and particularly the import/export process), it's going to have potentially messy data in it. The courses might have two John Smith professors or two signature images for the same instructor, middle names, and all sorts of other gremlins. No matter what the long term decision around canonical representation is, there's going to have to be some reconciliation process between Studio OLX/Modulestore and Credentials.

So if there are difficult decisions ahead about the structure of the Credentials schema as a system of record, can we for now copy data into Credentials models in a way that represents, "this is the Studio record of what certs configuration is"? Whether Credentials uses that as one of multiple inputs or decides to override it entirely based on some course configuration is something that can still be hashed out later. Plus we'd start to get this data into Credentials in a saner, more queryable form that is consistent between Old and Split Mongo–making future data migrations/reconciliation something that happens between one set of Django models and another within the same service, instead of reaching back into Modulestore/MongoDB queries.

Would that work as a path forward?

@e0d e0d added the FC-0004 label Jan 18, 2023
@e0d
Copy link

e0d commented Jan 19, 2023

Bringing back some side-bar conversatons.

Moving Old Mongo signatures to another location is only a blocker for completing the Old Mongo deprecaton if we are required to support serving credentials earned in Old Mongo Courses after that module store has been deprecated. If another route like serializing to S3, asking learners to download the certificates, etc. is possible, this work is not required to finalize the Old Mongo deprecation.

We believe that this is only a substantive issue for 2U.

MIgrating Old Mongo courses to Split Mongo is, unfortunately, not supported, probably a lot of work, and was de-scoped during the initial implementation.

@ormsbee
Copy link

ormsbee commented Jan 28, 2023

@cdeery: Just wanted to ping on this, and check to see if my proposal sounded workable to you.

@mphilbrick211
Copy link

Hi @cdeery - friendly follow-up on Dave's question above.

@cdeery
Copy link
Contributor

cdeery commented Feb 15, 2023

I think I have a handle on the correct approach here.
The problem was that the Credentials schema is normalized, but probably too much to work effectively with the fully denormalized data in Mongo.
So the best approach that will also work when we migrate the split mongo course credentials to the Credentials service will be to create a separate set of tables to hold course certificates, and to preserve the 1-1 relationship between courses and their certificates, and the 1-1 relationship between Certificates and signatories.

The tables would be something like this (there are a couple of pending questions)

CourseCertificates
Course_run: CharField (Are they organized by course run or course? What about in Split Mongo?)
Course_title: CharField
Name: CharField
Is_active: BooleanField
Version: IntegerField (is this needed? I assume it is for Mongo document version)
Editing: BooleanField (is this needed?)
Id:int
Description:CharField


CourseCertificateSignatory
Course_run: CharField (Same question as above - course_run or course?)
Name: CharField
Certificate: ?? is this used? 
Title: CharField
Organization CharField
Signature_image_path: CharField (Does this point back into Mongo? Do we need to migrate these as well?
Id: IntegerField 
Description: CharField

The API would then take the course ID, and return the Certificate info as a JSON blob.

At this time, we only need the read API. The data from Old Mongo can be injected through a management command.

How does this sound as an approach?

@hurtstotouchfire
Copy link
Member

@e0d / @ormsbee let us know what you think of this approach. If this is out of scope for the work left on this funded contribution we can rethink and potentially pursue implementation on our side.

@ormsbee
Copy link

ormsbee commented Feb 22, 2023

@cdeery: I'm fine with loading data into new models, though I'm not sure exactly what they'd look like if they don't echo what's already in Mongo.

Some notes:

Course_run: CharField (Are they organized by course run or course? What about in Split Mongo?)

In the modulestore, pretty much everything is stored by the full course run (which in a lot of LMS is called just a "course"), and would be stored with a CourseKeyField.

Signature_image_path: CharField (Does this point back into Mongo? Do we need to migrate these as well?

We must be able to move the assets out of Mongo. One of the root motivations for this work was that we're removing the Old Mongo Modulestore's ability to serve static assets, which includes images used for certificates.

@NiedielnitsevIvan: Could you please take a look at @cdeery's proposed model and give feedback? If it seems reasonable to you, please go ahead and start implementing it.

FWIW, I'm on PTO this week, so it may take me a while to respond.

@hurtstotouchfire hurtstotouchfire added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Feb 27, 2023
@mphilbrick211
Copy link

@cdeery: I'm fine with loading data into new models, though I'm not sure exactly what they'd look like if they don't echo what's already in Mongo.

Some notes:

Course_run: CharField (Are they organized by course run or course? What about in Split Mongo?)

In the modulestore, pretty much everything is stored by the full course run (which in a lot of LMS is called just a "course"), and would be stored with a CourseKeyField.

Signature_image_path: CharField (Does this point back into Mongo? Do we need to migrate these as well?

We must be able to move the assets out of Mongo. One of the root motivations for this work was that we're removing the Old Mongo Modulestore's ability to serve static assets, which includes images used for certificates.

@NiedielnitsevIvan: Could you please take a look at @cdeery's proposed model and give feedback? If it seems reasonable to you, please go ahead and start implementing it.

FWIW, I'm on PTO this week, so it may take me a while to respond.

hi @NiedielnitsevIvan! Friendly check-in on this!

@mphilbrick211
Copy link

Hi @NiedielnitsevIvan! Friendly ping on this :)

@GlugovGrGlib GlugovGrGlib force-pushed the Ivan_Niedielnitsev/EDXOLDMNG-200/Extend-CourseCertificateViewSet-in-Credentials-to-include-certificate-signatures branch from 9b9ac0e to 800a07d Compare June 13, 2023 13:47
@GlugovGrGlib GlugovGrGlib reopened this Jun 13, 2023
@GlugovGrGlib
Copy link
Member

GlugovGrGlib commented Oct 4, 2023

It's decided to close this PR as for now because the decision regarding moving certificate signatures from LMS into Credentials IDA requires more discussions. When the decision is made, parts of this PR can be reused for the further work on transitioning OeX certificates into Credentials IDA.

@openedx-webhooks
Copy link

@NiedielnitsevIvan Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

1 similar comment
@openedx-webhooks
Copy link

@NiedielnitsevIvan Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

@itsjeyd itsjeyd added blocked by other work PR cannot be finished until other work is complete and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked by other work PR cannot be finished until other work is complete FC-0004 open-source-contribution PR author is not from Axim or 2U
Projects
Status: Done
Archived in project
Development

Successfully merging this pull request may close these issues.

9 participants