bugfix remove course pre requisite only if is not entrance exam by melvinsoft · Pull Request #22529 · openedx/openedx-platform

melvinsoft · 2019-12-13T14:41:08Z

This is a bug fix for Entrance Exams management in Schedule & Details page.

The error is easy to reproduce:
You need to make sure that the following FEATURES flags are enabled:

"FEATURES":
    "MILESTONES_APP": true,
    "ENABLE_PREREQUISITE_COURSES": true,
    "ENTRANCE_EXAMS": true

Go to or create a new course, then go to "Schedule and Details" , enable an entrance exam for the course, you can leave the default score or change it.
Go to Content -> Outline and fill the entrance exam with a couple of courses, then create a normal unit in the course.
Go to the LMS and Enroll in the course with a new learner (without any staff privilege). You will only be able to see the entrance exam.
Go to Studio again, same course and then "Schedule and Details" change the enrollment date or any other data and save.

When you go back to the LMS, you will find out that the entrance exam is disabled now, and you can see all the course content, but in the "Schedule and Details" is still active.

What happens in the background, is that there is a bug in the settings handlers that is there is any pre requisite course set, iterates over all pre requisites and deletes them, but the problem is Entrance Exams are a type of pre requisite since they belong to the milestones app, and is getting deleted too.

The fix check the milestone namespace, and if it's an Entrance Exam skips it.

Co-Authored-By: Omar Al-Ithawi i@omardo.com

openedx-webhooks · 2019-12-13T14:41:20Z

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

natabene · 2019-12-13T16:44:22Z

jenkins run all

natabene · 2019-12-13T18:57:42Z

@melvinsoft Thank you for your contribution, please let me know once all tests are green.

OmarIthawi

Thanks Maxi! I've added some minor changes. Please let me know whenever this is ready for review again.

OmarIthawi · 2019-12-14T12:05:23Z

                        course_milestones = milestones_api.get_course_milestones(course_key=course_key, relationship="requires")
                        for milestone in course_milestones:
-                            remove_prerequisite_course(course_key, milestone)
+                            ee_milestone_namespace = generate_milestone_namespace(


ee is not immediately clear. Please refactor as suggested below 👇

melvinsoft · 2019-12-30T16:05:21Z

jenkins run all

natabene · 2019-12-30T16:07:15Z

jenkins run all

natabene · 2020-01-02T18:32:46Z

jenkins run all

natabene · 2020-01-07T18:13:54Z

jenkins run all

melvinsoft · 2020-01-10T22:46:42Z

jenkins run all

natabene · 2020-01-13T15:47:11Z

jenkins run all

natabene · 2020-02-18T17:53:20Z

@melvinsoft Can you please check the failing tests?

OmarIthawi · 2020-02-19T07:57:16Z

Thanks Natalia for the ping. @melvinsoft I've rebased the branch meanwhile hoping the Jenkins tests will be a bit better,

melvinsoft · 2020-02-19T10:03:03Z

@natabene Thanks for reaching out, and thanks @OmarIthawi I hope test are better now after the rebase, if not we'll take care of the errors.

natabene · 2020-02-19T16:17:20Z

jenkins run all

OmarIthawi · 2020-02-19T18:17:24Z

@melvinsoft I've fixed all of the quality checks that I can fix, please check the second commit to see what I have done.

There's only one that I couldn't fix quickly, which is pylint complaining about the if statement we added, too many nested if statements are not acceptable by pylint.

@natabene We could use the reviewer advice on how to refactor the code to avoid nested if statements. Could you please move this to Engineering Review?

natabene · 2020-02-19T19:07:44Z

@OmarIthawi Sure, I will line this up.

melvinsoft · 2020-02-19T19:15:43Z

@OmarIthawi Thank you for the changes, and thanks for helping moving this PR along.

natabene · 2020-03-03T18:27:41Z

jenkins run all

melvinsoft · 2020-03-03T19:13:02Z

@natabene Looks like all checks have passed now!

@OmarIthawi Thanks for pylint fixes!

OmarIthawi

...

natabene · 2022-09-01T02:27:55Z

@melvinsoft Sorry for delay, this is good to go now from CLA perspective. The check is green.

natabene · 2022-09-01T02:28:46Z

Who can take it over the finish line and merge (once ready)?

mariajgrimaldi · 2022-09-02T19:01:02Z

I can review @natabene

OmarIthawi · 2022-09-05T12:57:20Z

Thanks @mariajgrimaldi and @natabene. The PR should be ready for review and merge.

@shadinaif would you mind confirm that?

mariajgrimaldi · 2022-09-08T13:45:57Z

Let me know! @shadinaif @OmarIthawi

mariajgrimaldi · 2022-09-09T14:19:13Z

Agreed. It'd more clear if you use entrance_exam_namespace or something similar. Also, can you add a substantial comment explaining why you're adding this change? In case there's a similar issue in the future

And that makes me wonder, do you think there are other milestones that this piece of code could remove? or just entrance exam?

mariajgrimaldi · 2022-09-09T14:19:26Z

Suggested change

if not milestone["namespace"] == ee_milestone_namespace:

if milestone["namespace"] != ee_milestone_namespace:

mariajgrimaldi · 2022-10-07T18:45:02Z

Hello! Any updates here?

shadinaif · 2022-10-08T01:36:15Z

Thanks @mariajgrimaldi for the reminder

mariajgrimaldi · 2022-10-10T18:49:12Z

+        assert milestones_helpers.any_unfulfilled_milestones(self.course.id, self.user.id), \
+            'The entrance exam should be required.'
+
+        data_date = {
+            'entrance_exam_enabled': 'true',
+            'entrance_exam_minimum_score_pct': '60',
+            'syllabus': 'none',
+            'short_description': 'empty',
+            'overview': '',
+            'effort': '',
+            'intro_video': '',
+            'start_date': '2018-01-01',
+            'end_date': '{year}-12-31'.format(year=datetime.datetime.now().year + 4),
+        }
+        response = self.client.post(
+            settings_details_url,
+            data=json.dumps(data_date),
+            content_type='application/json',
+            HTTP_ACCEPT='application/json'
+        )
+        assert response.status_code == 200
+        assert milestones_helpers.any_unfulfilled_milestones(self.course.id, self.user.id), \
+            'The entrance exam should be required.'
+


Is this test duplicated? What changes from the 1st post call to the 2nd one? If it's just the date:
so we don't have to duplicate this, we could use the ddt library, something like this:

@ddt.data( first_dict, second_dict ) def test_entrance_after_changing_other_setting(self, edit_course_data):

Thank you for the review! it's not duplicated, we need to verify that saving new settings will not delete the entrance exam we set before. I've adjusted the code to show that

mariajgrimaldi

Hello! I tested a before & after (attached here), and this is working as you described. Thank you so much. I'm happy with this but consider making the tests shorter as I mentioned in the comment (if this makes sense to you folks).

After creating a course, enabling the entrance exam and then updating the date in the course:

After doing the same but with the fix:

I have a question: How can people fix this without creating a new course? For me, it worked to turn off and on again the entrance exam.

mariajgrimaldi · 2022-10-19T13:23:25Z

@shadinaif: hello! Do these recommendations make sense to you? If not, please leave a comment explaining and I'll merge it since the implementation looks good to me.

mariajgrimaldi · 2022-11-10T14:05:22Z

@shadinaif hello again! Please, refer to my previous comment so we can merge :)

See details in the related PR Update cms/djangoapps/contentstore/tests/test_course_settings.py Co-Authored-By: Omar Al-Ithawi <i@omardo.com> Update cms/djangoapps/contentstore/tests/test_course_settings.py Co-Authored-By: Omar Al-Ithawi <i@omardo.com>

Reformatting for an old code, no harm done

We've made a change in an old code. Pylint is not happy about that old code we don't need to refactor it now, lets just disable the error message for it

openedx-webhooks · 2022-11-17T12:20:43Z

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

edx-pipeline-bot · 2022-11-17T13:34:28Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

edx-pipeline-bot · 2022-11-17T14:11:10Z

EdX Release Notice: This PR has been deployed to the production environment.

OmarIthawi · 2022-11-17T17:15:28Z

@mariajgrimaldi @natabene @shadinaif and @melvinsoft. Thank you so much for closing this bug fix!

It's been a while since this was opened. Thanks for making it to the end 🎉 🎉 🎉

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Dec 13, 2019

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels Dec 13, 2019

OmarIthawi suggested changes Dec 14, 2019

View reviewed changes

melvinsoft force-pushed the maxi/fix-entrance-exam-course-details branch from 529907a to 01fa77f Compare January 10, 2020 22:39

OmarIthawi force-pushed the maxi/fix-entrance-exam-course-details branch from 01fa77f to fcffb2e Compare February 19, 2020 07:56

openedx-webhooks added engineering review and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Feb 19, 2020

openedx-webhooks added awaiting prioritization and removed engineering review labels Mar 3, 2020

OmarIthawi reviewed Mar 4, 2020

View reviewed changes

natabene changed the title ~~bugfix remove course pre requisite only if is not entrance exam~~ bugfix remove course pre requisite only if is not entrance exam Sep 1, 2022

natabene assigned mariajgrimaldi Sep 6, 2022

mariajgrimaldi requested changes Sep 9, 2022

View reviewed changes

shadinaif force-pushed the maxi/fix-entrance-exam-course-details branch 2 times, most recently from 6193915 to 4ce975c Compare October 8, 2022 01:34

shadinaif force-pushed the maxi/fix-entrance-exam-course-details branch from 4ce975c to 76e724c Compare October 8, 2022 01:57

mariajgrimaldi reviewed Oct 10, 2022

View reviewed changes

mariajgrimaldi approved these changes Oct 10, 2022

View reviewed changes

shadinaif force-pushed the maxi/fix-entrance-exam-course-details branch from 76e724c to ef93b2e Compare November 16, 2022 13:15

melvinsoft and others added 3 commits November 16, 2022 16:17

fix: pylint issues

65f0e48

Reformatting for an old code, no harm done

fix: Disable linter error to avoid refactoring an old code

e68af88

We've made a change in an old code. Pylint is not happy about that old code we don't need to refactor it now, lets just disable the error message for it

shadinaif force-pushed the maxi/fix-entrance-exam-course-details branch from ef93b2e to e68af88 Compare November 16, 2022 13:19

mariajgrimaldi merged commit 5fa0528 into openedx:master Nov 17, 2022

OmarIthawi deleted the maxi/fix-entrance-exam-course-details branch November 17, 2022 14:02

	if not milestone["namespace"] == ee_milestone_namespace:
	if milestone["namespace"] != ee_milestone_namespace:

Uh oh!

Conversation

melvinsoft commented Dec 13, 2019

Uh oh!

openedx-webhooks commented Dec 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

natabene commented Dec 13, 2019

Uh oh!

natabene commented Dec 13, 2019

Uh oh!

OmarIthawi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melvinsoft commented Dec 30, 2019

Uh oh!

natabene commented Dec 30, 2019

Uh oh!

natabene commented Jan 2, 2020

Uh oh!

natabene commented Jan 7, 2020

Uh oh!

melvinsoft commented Jan 10, 2020

Uh oh!

natabene commented Jan 13, 2020

Uh oh!

natabene commented Feb 18, 2020

Uh oh!

OmarIthawi commented Feb 19, 2020

Uh oh!

melvinsoft commented Feb 19, 2020

Uh oh!

natabene commented Feb 19, 2020

Uh oh!

OmarIthawi commented Feb 19, 2020

Uh oh!

natabene commented Feb 19, 2020

Uh oh!

melvinsoft commented Feb 19, 2020

Uh oh!

natabene commented Mar 3, 2020

Uh oh!

melvinsoft commented Mar 3, 2020

Uh oh!

OmarIthawi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natabene commented Sep 1, 2022

Uh oh!

natabene commented Sep 1, 2022

Uh oh!

mariajgrimaldi commented Sep 2, 2022

Uh oh!

OmarIthawi commented Sep 5, 2022

Uh oh!

mariajgrimaldi commented Sep 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mariajgrimaldi commented Oct 7, 2022

openedx-webhooks commented Dec 13, 2019 •

edited

Loading

OmarIthawi left a comment •

edited

Loading

mariajgrimaldi left a comment •

edited

Loading

mariajgrimaldi commented Oct 19, 2022 •

edited

Loading