forked from openedx/edx-enterprise
-
Notifications
You must be signed in to change notification settings - Fork 0
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
fix: create CEA object when enrolling using a license flow #18
Merged
tecoholic
merged 5 commits into
0x29a/bb8626/enroll-enterprise-learners-in-invite-only-courses
from
0x29a/bb8808/cea-license-flow
Aug 12, 2024
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
b9e7e7e
fix: create CEA object when enrolling using a license flow
0x29a cf38e32
test: verify that allow_enrollment is called_correctly
0x29a 8b82cbe
fix: xmlsec issue
0x29a 7d010cb
build: enable CI for pull requests
0x29a c92b6af
style: fix some pycodestyle issues
0x29a File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to re-use this helper as is, but it was locking a DB row causing enrollment to fail here, because the LMS was trying to modify the locked row. That's why I had to use this API to create CEA in a separate transaction. If this approach is viable, we'll have to backport this endpoint to Palm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@0x29a It's surprising that we haven't run into this race condition before. Any idea why it might be occuring now?
Irrespective of the reasons, I think using the API is a fine approach to take, given that we get the course details in the previous statement using the API and not an import. It looks like the
enrollment_allowed
is a recent API addition (added 6/7 months ago). We would have probably used it if it existed earlier, as this allows creating CEA objects from other services like an admin MFE as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for checking this, @tecoholic.
This was bothering me as well, but I was unable to figure this out back then. It turns out that the view modified in openedx#1813 is not atomic. And the view modified in #14 doesn't send any internal HTTP requests in fact, so CEA objects are manipulated within a single transaction. In this PR we modify a view that is both atomic and sends a request to another atomic view, and they both try to modify CEA.
We can also change
View
withNonAtomicView
here, and this will simplify things a bit, but I'm not sure how risky this is.