-
Notifications
You must be signed in to change notification settings - Fork 48
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
[ENT-5039] - Moodle - Inactivate courses instead of true delete #1881
Conversation
Thanks for the pull request, @hamzawaleed01! 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:
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. Once you've signed the CLA, please allow 1 business day for it to be processed. After this time, you can re-run the CLA check by adding a comment here that you have signed it. If the problem persists, you can tag the |
f0d3c8e
to
f85a9a1
Compare
8532f6f
to
403ff13
Compare
@johnnagro @alex-sheehan-edx need your review on this one 🧠 |
3f73dee
to
1b89a54
Compare
…to hamza/ENT-5039
1b89a54
to
52fdedd
Compare
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.
Had one thought about overall approach but it's more of a thought/exploration/improvement. Your code follows the pattern established and I believe gets the job done 👍
ofc don't forget the premerge checklist before you merge 👍 |
a7b9101
to
bde875f
Compare
…to hamza/ENT-5039
bde875f
to
5906470
Compare
…to hamza/ENT-5039
…to hamza/ENT-5039
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.
looks great 😄
'courseids[]': moodle_course_id | ||
} | ||
response = self._wrapped_post(params) | ||
serialized_data['wsfunction'] = 'core_course_update_courses' |
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.
while I think it's possible for us to shove this into say the prepare_items_For_transmission
or the prepare items for delete methods, I don't think it's worth the effort. Good as is! 👍
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.
welcome to EDX 😄
Ticket: ENT-5039
Merge checklist:
requirements/*.txt
files)base.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./manage.py makemigrations
has been run./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.