-
Notifications
You must be signed in to change notification settings - Fork 47
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
feat: handle enrollments for "Invite Only" courses #1748
base: master
Are you sure you want to change the base?
feat: handle enrollments for "Invite Only" courses #1748
Conversation
Thanks for the pull request, @tecoholic! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently unmaintained. To get help with finding a technical reviewer, tag the community contributions project manager for this PR in a comment and let them know that your changes are ready for review:
Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:
When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
@tecoholic it looks like the branch has conflicts, can you have a look? |
49ea86a
to
534af99
Compare
@e0d I have rebased the PR branch and bumped the version in |
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 tested this: verified that the forced enrollment works correctly for invitation-only courses
- I read through the code
- I checked for accessibility issues: n/a
- Includes documentation: n/a
- I made sure any change in configuration variables is reflected in the corresponding client's
configuration-secure
repository: n/a
@tecoholic Looks like there are some conflicts that need to be resolved, can you have a look? |
534af99
to
d8b24f4
Compare
@e0d I have rebased it on master and fixed the conflicts. Can you kindly approve the tests to run? |
@tecoholic Tests approved. |
@e0d Looks like all checks have passed. Can you kindly see if this can be reviewed and merged? |
Hi @macdiesel and @openedx/enterprise-titans! Could someone please review and merge this if all looks good? Thanks! |
@mphilbrick211 Hi, can you kindly get someone to take a look at this? |
Hi @macdiesel and @openedx/enterprise-titans! Friendly ping on this :) In the meantime, @tecoholic, would you mind resolving the branch conflicts? Thanks! |
84dca10
to
110bdb0
Compare
@mphilbrick211 Done. All good w.r.t conflicts. The CI tests require approval to run. |
Hi @tecoholic - tests are running again, but there are some more branch conflicts that have popped up. |
@mphilbrick211 I think the conflicts will continue to happen on the changelog and the version number as long as things are changing on the master and it would be a daily task to keep resolving them. So, I am think we could do either of the 2 things:
What do you think? |
@e0d Hi, I have resolved the conflict. If it runs into conflict again, I guess it wouldn't be possible for you to make changes as Demid pointed out. Would like me to squash the changed into a single commit, that would allow you to cherry-pick and resolve conflicts locally and merge? |
@tecoholic I don't think I'm going to be able to take over conflict management. I did notice that a number of the OpenCraft PRs are broken because of a doc formatting issue. See my commit here. |
d4b2e1b
to
a83e1fc
Compare
@e0d Hi, since the conflicts have been occurring on the version bump and the changelog, I have removed them from the PR and rebased it to the latest master. Kindly see if this helps. |
@tecoholic, since the upstream review for this didn't start yet, you can squash the commits. It will be easier to cherry-pick this to our Palm branch. |
a83e1fc
to
0e06d74
Compare
@mphilbrick211 this branch will need a rebase when the reviewers are ready to look at it, but the tests are all green. Removing the label. |
Hi @openedx/enterprise-titans! Following up to see if we could please get this reviewed/merged. Thanks! |
Hi @tecoholic! Some branch conflicts have popped up - would you mind taking a looK? |
@mphilbrick211, I believe it would make sense to move forward with this document first. Without a timeline for these reviews, the branch conflicts will appear regularly. cc: @tecoholic |
Sounds good - thanks, @Agrendalath! |
Hi @openedx/enterprise-titans! Is there an ETA on when someone could please review this? Thanks! |
Hi @openedx/enterprise-titans - following up on this! |
0e06d74
to
074b4b2
Compare
When creating pending enrollments for non-existant users, we also check to see if the course is "invite_only". If the course is invite only, then we create corresponding CourseEnrollmentAllowed objects. This fixes the issue when the enterprise creates pending enrollment, but the user cannot enroll to the course as platform rejects the enrollment request due to missing CEA for the user.
074b4b2
to
2d8c2d4
Compare
@openedx/enterprise-titans The PR has been rebased on master and all the checks are passing. Kindly review. cc: @e0d |
Hi @openedx/2u-titans! Are you still reviewing pull requests? If so, could you please review / merge this for us? Thanks! |
Description
This PR introduces some improvements to the 'Manage Learners' page of Enterprise Customer admin that will allow enrolling learners into "Invite Only" courses.
Refer #1745 for full details about the issue.
Testing instructions
Merge checklist:
requirements/*.txt
files) - No new requirements addedbase.in
if needed in production but edx-platform doesn't install ittest-master.in
if edx-platform pins it, with a matching versionmake upgrade && make requirements
have been run to regenerate requirementsmake static
has been run to update webpack bundling if any static content was updated - Running make static didn't introduce any changes./manage.py makemigrations
has been run - No DB migrations introduced./manage.py lms makemigrations
in the shell.Post merge:
(so basically once your build finishes, after maybe a minute you should see the new version in PyPi automatically (on refresh))
make upgrade
in edx-platform will look for the latest version in PyPi.