From fc5716d9b1406799565afbac3b5965443a45081e Mon Sep 17 00:00:00 2001 From: Ahmed Jazzar Date: Fri, 24 Mar 2017 09:36:24 +0200 Subject: [PATCH] Fixed milestone issues In settings_handler and any_unfulfilled_milestones (for mobile APIs) --- AUTHORS | 1 + .../tests/test_course_settings.py | 28 +++++++++++++++++++ cms/djangoapps/contentstore/views/course.py | 7 +++++ common/djangoapps/util/milestones_helpers.py | 9 ++++-- .../util/tests/test_milestones_helpers.py | 27 ++++++++++++------ 5 files changed, 60 insertions(+), 12 deletions(-) diff --git a/AUTHORS b/AUTHORS index 14525689f8..39a819cf42 100644 --- a/AUTHORS +++ b/AUTHORS @@ -259,6 +259,7 @@ Muhammad Rehan Shawn Milochik Afeef Janjua Jacek Bzdak +Ahmed Jazzar Jillian Vogel Dan Powell Mariana Araújo diff --git a/cms/djangoapps/contentstore/tests/test_course_settings.py b/cms/djangoapps/contentstore/tests/test_course_settings.py index fdd14d9bb0..bfa1f985a5 100644 --- a/cms/djangoapps/contentstore/tests/test_course_settings.py +++ b/cms/djangoapps/contentstore/tests/test_course_settings.py @@ -15,6 +15,7 @@ from milestones.tests.utils import MilestonesTestCaseMixin from mock import Mock, patch from contentstore.utils import reverse_course_url, reverse_usage_url +from milestones.models import MilestoneRelationshipType from models.settings.course_grading import CourseGradingModel, GRADING_POLICY_CHANGED_EVENT_TYPE, hash_grading_policy from models.settings.course_metadata import CourseMetadata from models.settings.encoder import CourseSettingsEncoder @@ -22,6 +23,7 @@ from openedx.core.djangoapps.models.course_details import CourseDetails from openedx.core.djangoapps.self_paced.models import SelfPacedConfiguration from student.roles import CourseInstructorRole, CourseStaffRole from student.tests.factories import UserFactory +from util import milestones_helpers from xblock_django.models import XBlockStudioConfigurationFlag from xmodule.fields import Date from xmodule.modulestore import ModuleStoreEnum @@ -100,6 +102,9 @@ class CourseDetailsViewTest(CourseTestCase, MilestonesTestCaseMixin): resp = self.client.ajax_post(url, payload) self.compare_details_with_encoding(json.loads(resp.content), details.__dict__, field + str(val)) + MilestoneRelationshipType.objects.get_or_create(name='requires') + MilestoneRelationshipType.objects.get_or_create(name='fulfills') + @staticmethod def convert_datetime_to_iso(datetime_obj): """ @@ -172,6 +177,9 @@ class CourseDetailsViewTest(CourseTestCase, MilestonesTestCaseMixin): @mock.patch.dict("django.conf.settings.FEATURES", {'ENABLE_PREREQUISITE_COURSES': True}) def test_pre_requisite_course_update_and_fetch(self): + self.assertFalse(milestones_helpers.any_unfulfilled_milestones(self.course.id, self.user.id), + msg='The initial empty state should be: no prerequisite courses') + url = get_url(self.course.id) resp = self.client.get_json(url) course_detail_json = json.loads(resp.content) @@ -190,6 +198,9 @@ class CourseDetailsViewTest(CourseTestCase, MilestonesTestCaseMixin): course_detail_json = json.loads(resp.content) self.assertEqual(pre_requisite_course_keys, course_detail_json['pre_requisite_courses']) + self.assertTrue(milestones_helpers.any_unfulfilled_milestones(self.course.id, self.user.id), + msg='Should have prerequisite courses') + # remove pre requisite course course_detail_json['pre_requisite_courses'] = [] self.client.ajax_post(url, course_detail_json) @@ -197,6 +208,9 @@ class CourseDetailsViewTest(CourseTestCase, MilestonesTestCaseMixin): course_detail_json = json.loads(resp.content) self.assertEqual([], course_detail_json['pre_requisite_courses']) + self.assertFalse(milestones_helpers.any_unfulfilled_milestones(self.course.id, self.user.id), + msg='Should not have prerequisite courses anymore') + @mock.patch.dict("django.conf.settings.FEATURES", {'ENABLE_PREREQUISITE_COURSES': True}) def test_invalid_pre_requisite_course(self): url = get_url(self.course.id) @@ -268,6 +282,14 @@ class CourseDetailsViewTest(CourseTestCase, MilestonesTestCaseMixin): @unittest.skipUnless(settings.FEATURES.get('ENTRANCE_EXAMS', False), True) def test_entrance_exam_created_updated_and_deleted_successfully(self): + """ + This tests both of the entrance exam settings and the `any_unfulfilled_milestones` helper. + + Splitting the test requires significant refactoring `settings_handler()` view. + """ + self.assertFalse(milestones_helpers.any_unfulfilled_milestones(self.course.id, self.user.id), + msg='The initial empty state should be: no entrance exam') + settings_details_url = get_url(self.course.id) data = { 'entrance_exam_enabled': 'true', @@ -299,6 +321,9 @@ class CourseDetailsViewTest(CourseTestCase, MilestonesTestCaseMixin): self.assertTrue(course.entrance_exam_enabled) self.assertEquals(course.entrance_exam_minimum_score_pct, .80) + self.assertTrue(milestones_helpers.any_unfulfilled_milestones(self.course.id, self.user.id), + msg='The entrance exam should be required.') + # Delete the entrance exam data['entrance_exam_enabled'] = "false" response = self.client.post( @@ -312,6 +337,9 @@ class CourseDetailsViewTest(CourseTestCase, MilestonesTestCaseMixin): self.assertFalse(course.entrance_exam_enabled) self.assertEquals(course.entrance_exam_minimum_score_pct, None) + self.assertFalse(milestones_helpers.any_unfulfilled_milestones(self.course.id, self.user.id), + msg='The entrance exam should not be required anymore') + @unittest.skipUnless(settings.FEATURES.get('ENTRANCE_EXAMS', False), True) def test_entrance_exam_store_default_min_score(self): """ diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 7e837dcf05..f306c6f9ea 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -51,6 +51,7 @@ from course_action_state.managers import CourseActionStateItemNotFoundError from course_action_state.models import CourseRerunState, CourseRerunUIStateManager from course_creators.views import add_user_with_status_unrequested, get_course_creator_status from edxmako.shortcuts import render_to_response +from milestones import api as milestones_api from models.settings.course_grading import CourseGradingModel from models.settings.course_metadata import CourseMetadata from models.settings.encoder import CourseSettingsEncoder @@ -73,6 +74,7 @@ from util.milestones_helpers import ( is_entrance_exams_enabled, is_prerequisite_courses_enabled, is_valid_course_key, + remove_prerequisite_course, set_prerequisite_courses ) from util.organizations_helpers import add_organization_course, get_organization_by_short_name, organizations_enabled @@ -1075,6 +1077,11 @@ def settings_handler(request, course_key_string): if not all(is_valid_course_key(course_key) for course_key in prerequisite_course_keys): return JsonResponseBadRequest({"error": _("Invalid prerequisite course key")}) set_prerequisite_courses(course_key, prerequisite_course_keys) + else: + # None is chosen, so remove the course prerequisites + course_milestones = milestones_api.get_course_milestones(course_key=course_key, relationship="requires") + for milestone in course_milestones: + remove_prerequisite_course(course_key, milestone) # If the entrance exams feature has been enabled, we'll need to check for some # feature-specific settings and handle them accordingly diff --git a/common/djangoapps/util/milestones_helpers.py b/common/djangoapps/util/milestones_helpers.py index bbe54b3dcf..257e16e477 100644 --- a/common/djangoapps/util/milestones_helpers.py +++ b/common/djangoapps/util/milestones_helpers.py @@ -384,9 +384,12 @@ def any_unfulfilled_milestones(course_id, user_id): """ Returns a boolean if user has any unfulfilled milestones """ if not settings.FEATURES.get('MILESTONES_APP'): return False - return bool( - get_course_milestones_fulfillment_paths(course_id, {"id": user_id}) - ) + + fulfillment_paths = milestones_api.get_course_milestones_fulfillment_paths(course_id, {'id': user_id}) + + # Returns True if any of the milestones is unfulfilled. False if + # values is empty or all values are. + return any(fulfillment_paths.values()) def get_course_milestones_fulfillment_paths(course_id, user_id): diff --git a/common/djangoapps/util/tests/test_milestones_helpers.py b/common/djangoapps/util/tests/test_milestones_helpers.py index 63263ef810..163c7d2d14 100644 --- a/common/djangoapps/util/tests/test_milestones_helpers.py +++ b/common/djangoapps/util/tests/test_milestones_helpers.py @@ -3,15 +3,18 @@ Tests for the milestones helpers library, which is the integration point for the """ import ddt +from django.conf import settings from milestones.exceptions import InvalidCourseKeyException, InvalidUserException from mock import patch -from util import milestones_helpers +from milestones import api as milestones_api +from milestones.models import MilestoneRelationshipType from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory +from util import milestones_helpers -@patch.dict('django.conf.settings.FEATURES', {'MILESTONES_APP': False}) +@patch.dict(settings.FEATURES, {'MILESTONES_APP': False}) @ddt.ddt class MilestonesHelpersTestCase(ModuleStoreTestCase): """ @@ -33,11 +36,14 @@ class MilestonesHelpersTestCase(ModuleStoreTestCase): self.user = {'id': '123'} - self.milestone = { + self.milestone = milestones_api.add_milestone({ 'name': 'Test Milestone', 'namespace': 'doesnt.matter', 'description': 'Testing Milestones Helpers Library', - } + }) + + MilestoneRelationshipType.objects.get_or_create(name='requires') + MilestoneRelationshipType.objects.get_or_create(name='fulfills') @ddt.data( (False, False, False), @@ -115,13 +121,16 @@ class MilestonesHelpersTestCase(ModuleStoreTestCase): response = milestones_helpers.get_service() self.assertIsNone(response) - @patch.dict('django.conf.settings.FEATURES', {'MILESTONES_APP': True}) + @patch.dict(settings.FEATURES, {'MILESTONES_APP': True}) def test_any_unfulfilled_milestones(self): """ - Tests any_unfulfilled_milestones for invalid arguments with - the app enabled - """ + Tests any_unfulfilled_milestones for invalid arguments with the app enabled. + """ + + # Should not raise any exceptions + milestones_helpers.any_unfulfilled_milestones(self.course.id, self.user['id']) + with self.assertRaises(InvalidCourseKeyException): - milestones_helpers.any_unfulfilled_milestones(None, self.user) + milestones_helpers.any_unfulfilled_milestones(None, self.user['id']) with self.assertRaises(InvalidUserException): milestones_helpers.any_unfulfilled_milestones(self.course.id, None)