From 258aebed2031280c83dc065dd73224eae75a4248 Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Tue, 7 May 2013 12:58:49 -0400 Subject: [PATCH 1/7] Fix anonymoususer 500 error --- .../open_ended_grading/open_ended_notifications.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/lms/djangoapps/open_ended_grading/open_ended_notifications.py b/lms/djangoapps/open_ended_grading/open_ended_notifications.py index 6d5f2a3eb4..0f97ea2a85 100644 --- a/lms/djangoapps/open_ended_grading/open_ended_notifications.py +++ b/lms/djangoapps/open_ended_grading/open_ended_notifications.py @@ -11,6 +11,7 @@ from util.cache import cache import datetime from xmodule.x_module import ModuleSystem from mitxmako.shortcuts import render_to_string +import datetime log = logging.getLogger(__name__) @@ -121,17 +122,20 @@ def combined_notifications(course, user): success, notification_dict = get_value_from_cache(student_id, course_id, notification_type) if success: return notification_dict + if user.is_authenticated(): + last_login = user.last_login + else: + last_login = datetime.datetime.now() - min_time_to_query = user.last_login last_module_seen = StudentModule.objects.filter(student=user, course_id=course_id, - modified__gt=min_time_to_query).values('modified').order_by( + modified__gt=last_login).values('modified').order_by( '-modified') last_module_seen_count = last_module_seen.count() if last_module_seen_count > 0: last_time_viewed = last_module_seen[0]['modified'] - datetime.timedelta(seconds=(NOTIFICATION_CACHE_TIME + 60)) else: - last_time_viewed = user.last_login + last_time_viewed = last_login pending_grading = False From 9e03280f5065124fd2b8edcc8ecb835f80f82e0c Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Tue, 7 May 2013 15:33:15 -0400 Subject: [PATCH 2/7] Make peer grading fields stringy and fix js to avoid strange error --- .../js/src/peergrading/peer_grading.coffee | 23 ++++++++++--------- .../peergrading/peer_grading_problem.coffee | 1 - .../xmodule/xmodule/peer_grading_module.py | 12 +++++----- 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading.coffee b/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading.coffee index 45c678bad9..2ce7a09b92 100644 --- a/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading.coffee +++ b/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading.coffee @@ -8,20 +8,21 @@ class @PeerGrading @use_single_location = @peer_grading_container.data('use-single-location') @peer_grading_outer_container = $('.peer-grading-container') @ajax_url = @peer_grading_container.data('ajax-url') - @error_container = $('.error-container') - @error_container.toggle(not @error_container.is(':empty')) - - @message_container = $('.message-container') - @message_container.toggle(not @message_container.is(':empty')) - - @problem_button = $('.problem-button') - @problem_button.click @show_results - - @problem_list = $('.problem-list') - @construct_progress_bar() if @use_single_location @activate_problem() + else + @error_container = $('.error-container') + @error_container.toggle(not @error_container.is(':empty')) + + @message_container = $('.message-container') + @message_container.toggle(not @message_container.is(':empty')) + + @problem_button = $('.problem-button') + @problem_button.click @show_results + + @problem_list = $('.problem-list') + @construct_progress_bar() construct_progress_bar: () => problems = @problem_list.find('tr').next() diff --git a/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading_problem.coffee b/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading_problem.coffee index 9483932f80..001ef93001 100644 --- a/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading_problem.coffee +++ b/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading_problem.coffee @@ -27,7 +27,6 @@ class @PeerGradingProblemBackend else # if this post request fails, the error callback will catch it $.post(@ajax_url + cmd, data, callback) - .error => callback({success: false, error: "Error occured while performing this operation"}) mock: (cmd, data) -> if cmd == 'is_student_calibrated' diff --git a/common/lib/xmodule/xmodule/peer_grading_module.py b/common/lib/xmodule/xmodule/peer_grading_module.py index 35f2fa2d76..cf10e7c87c 100644 --- a/common/lib/xmodule/xmodule/peer_grading_module.py +++ b/common/lib/xmodule/xmodule/peer_grading_module.py @@ -11,7 +11,7 @@ from xmodule.raw_module import RawDescriptor from xmodule.modulestore.django import modulestore from .timeinfo import TimeInfo from xblock.core import Object, Integer, Boolean, String, Scope -from xmodule.fields import Date, StringyFloat +from xmodule.fields import Date, StringyFloat, StringyInteger, StringyBoolean from xmodule.open_ended_grading_classes.peer_grading_service import PeerGradingService, GradingServiceError, MockPeerGradingService from open_ended_grading_classes import combined_open_ended_rubric @@ -28,14 +28,14 @@ EXTERNAL_GRADER_NO_CONTACT_ERROR = "Failed to contact external graders. Please class PeerGradingFields(object): - use_for_single_location = Boolean(help="Whether to use this for a single location or as a panel.", + use_for_single_location = StringyBoolean(help="Whether to use this for a single location or as a panel.", default=USE_FOR_SINGLE_LOCATION, scope=Scope.settings) link_to_location = String(help="The location this problem is linked to.", default=LINK_TO_LOCATION, scope=Scope.settings) - is_graded = Boolean(help="Whether or not this module is scored.", default=IS_GRADED, scope=Scope.settings) + is_graded = StringyBoolean(help="Whether or not this module is scored.", default=IS_GRADED, scope=Scope.settings) due_date = Date(help="Due date that should be displayed.", default=None, scope=Scope.settings) grace_period_string = String(help="Amount of grace to give on the due date.", default=None, scope=Scope.settings) - max_grade = Integer(help="The maximum grade that a student can receieve for this problem.", default=MAX_SCORE, + max_grade = StringyInteger(help="The maximum grade that a student can receieve for this problem.", default=MAX_SCORE, scope=Scope.settings) student_data_for_location = Object(help="Student data for a given peer grading problem.", scope=Scope.user_state) @@ -93,9 +93,9 @@ class PeerGradingModule(PeerGradingFields, XModule): if not self.ajax_url.endswith("/"): self.ajax_url = self.ajax_url + "/" - if not isinstance(self.max_grade, (int, long)): + if not isinstance(self.max_grade, int): #This could result in an exception, but not wrapping in a try catch block so it moves up the stack - self.max_grade = int(self.max_grade) + raise TypeError("max_grade needs to be an integer.") def closed(self): return self._closed(self.timeinfo) From 1398b55713433adc0baca4bb5e957e1fe51602ee Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Tue, 7 May 2013 15:48:19 -0400 Subject: [PATCH 3/7] Comment touched modules --- .../js/src/peergrading/peer_grading.coffee | 2 + .../xmodule/xmodule/peer_grading_module.py | 2 +- .../open_ended_notifications.py | 40 ++++++++++++++----- 3 files changed, 33 insertions(+), 11 deletions(-) diff --git a/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading.coffee b/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading.coffee index 2ce7a09b92..676cc75d11 100644 --- a/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading.coffee +++ b/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading.coffee @@ -10,8 +10,10 @@ class @PeerGrading @ajax_url = @peer_grading_container.data('ajax-url') if @use_single_location + #If the peer grading element is linked to a single location, then activate the backend for that location @activate_problem() else + #Otherwise, activate the panel view. @error_container = $('.error-container') @error_container.toggle(not @error_container.is(':empty')) diff --git a/common/lib/xmodule/xmodule/peer_grading_module.py b/common/lib/xmodule/xmodule/peer_grading_module.py index cf10e7c87c..bb14eec8b5 100644 --- a/common/lib/xmodule/xmodule/peer_grading_module.py +++ b/common/lib/xmodule/xmodule/peer_grading_module.py @@ -93,8 +93,8 @@ class PeerGradingModule(PeerGradingFields, XModule): if not self.ajax_url.endswith("/"): self.ajax_url = self.ajax_url + "/" + #This could result in an exception, but not wrapping in a try catch block so it moves up the stack if not isinstance(self.max_grade, int): - #This could result in an exception, but not wrapping in a try catch block so it moves up the stack raise TypeError("max_grade needs to be an integer.") def closed(self): diff --git a/lms/djangoapps/open_ended_grading/open_ended_notifications.py b/lms/djangoapps/open_ended_grading/open_ended_notifications.py index 0f97ea2a85..1d6fa22929 100644 --- a/lms/djangoapps/open_ended_grading/open_ended_notifications.py +++ b/lms/djangoapps/open_ended_grading/open_ended_notifications.py @@ -105,6 +105,25 @@ def peer_grading_notifications(course, user): def combined_notifications(course, user): + """ + Show notifications to a given user for a given course. Get notifications from the cache if possible, + or from the grading controller server if not. + @param course: The course object for which we are getting notifications + @param user: The user object for which we are getting notifications + @return: A dictionary with boolean pending_grading (true if there is pending grading), img_path (for notification + image), and response (actual response from grading controller server). + """ + #Set up return values so that we can return them for error cases + pending_grading = False + img_path = "" + notifications={} + notification_dict = {'pending_grading': pending_grading, 'img_path': img_path, 'response': notifications} + + #We don't want to show anonymous users anything. + if not user.is_authenticated(): + return notification_dict + + #Define a mock modulesystem system = ModuleSystem( ajax_url=None, track_function=None, @@ -113,44 +132,44 @@ def combined_notifications(course, user): replace_urls=None, xblock_model_data= {} ) + #Initialize controller query service using our mock system controller_qs = ControllerQueryService(settings.OPEN_ENDED_GRADING_INTERFACE, system) student_id = unique_id_for_user(user) user_is_staff = has_access(user, course, 'staff') course_id = course.id notification_type = "combined" + #See if we have a stored value in the cache success, notification_dict = get_value_from_cache(student_id, course_id, notification_type) if success: return notification_dict - if user.is_authenticated(): - last_login = user.last_login - else: - last_login = datetime.datetime.now() + #Get the time of the last login of the user + last_login = user.last_login + + #Find the modules they have seen since they logged in last_module_seen = StudentModule.objects.filter(student=user, course_id=course_id, modified__gt=last_login).values('modified').order_by( '-modified') last_module_seen_count = last_module_seen.count() if last_module_seen_count > 0: + #The last time they viewed an updated notification (last module seen minus how long notifications are cached) last_time_viewed = last_module_seen[0]['modified'] - datetime.timedelta(seconds=(NOTIFICATION_CACHE_TIME + 60)) else: - last_time_viewed = last_login + #If they have not seen any modules since they logged in, then don't refresh + return {'pending_grading': False, 'img_path': img_path, 'response': notifications} - pending_grading = False - - img_path = "" try: + #Get the notifications from the grading controller controller_response = controller_qs.check_combined_notifications(course.id, student_id, user_is_staff, last_time_viewed) - log.debug(controller_response) notifications = json.loads(controller_response) if notifications['success']: if notifications['overall_need_to_check']: pending_grading = True except: #Non catastrophic error, so no real action - notifications = {} #This is a dev_facing_error log.exception( "Problem with getting notifications from controller query service for course {0} user {1}.".format( @@ -161,6 +180,7 @@ def combined_notifications(course, user): notification_dict = {'pending_grading': pending_grading, 'img_path': img_path, 'response': notifications} + #Store the notifications in the cache set_value_in_cache(student_id, course_id, notification_type, notification_dict) return notification_dict From e0d1eca6aa387196531300a9c96bcb11ed80e895 Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Tue, 7 May 2013 16:56:08 -0400 Subject: [PATCH 4/7] Delete two lines. without this, the xblock fields are never created in cases where the module is "fresh" (ie has no existing data) --- common/lib/xmodule/xmodule/combined_open_ended_module.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/common/lib/xmodule/xmodule/combined_open_ended_module.py b/common/lib/xmodule/xmodule/combined_open_ended_module.py index 239adcaa41..67ff206e89 100644 --- a/common/lib/xmodule/xmodule/combined_open_ended_module.py +++ b/common/lib/xmodule/xmodule/combined_open_ended_module.py @@ -203,9 +203,7 @@ class CombinedOpenEndedModule(CombinedOpenEndedFields, XModule): def save_instance_data(self): for attribute in self.student_attributes: - child_attr = getattr(self.child_module, attribute) - if child_attr != getattr(self, attribute): - setattr(self, attribute, getattr(self.child_module, attribute)) + setattr(self, attribute, getattr(self.child_module, attribute)) class CombinedOpenEndedDescriptor(CombinedOpenEndedFields, RawDescriptor): From 2648a19cc2b6c9e7449c583575539d31c2aba04e Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Tue, 7 May 2013 17:08:14 -0400 Subject: [PATCH 5/7] Fix check for use for single location --- .../lib/xmodule/xmodule/js/src/peergrading/peer_grading.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading.coffee b/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading.coffee index 676cc75d11..48980c7d88 100644 --- a/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading.coffee +++ b/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading.coffee @@ -9,7 +9,7 @@ class @PeerGrading @peer_grading_outer_container = $('.peer-grading-container') @ajax_url = @peer_grading_container.data('ajax-url') - if @use_single_location + if @use_single_location.toLowerCase() == "true" #If the peer grading element is linked to a single location, then activate the backend for that location @activate_problem() else From 530ac51c1c2c7fcd784e1c7bfac844feae296f3f Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Tue, 7 May 2013 17:33:34 -0400 Subject: [PATCH 6/7] Add .error callback back in --- .../xmodule/js/src/peergrading/peer_grading_problem.coffee | 1 + 1 file changed, 1 insertion(+) diff --git a/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading_problem.coffee b/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading_problem.coffee index 001ef93001..9483932f80 100644 --- a/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading_problem.coffee +++ b/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading_problem.coffee @@ -27,6 +27,7 @@ class @PeerGradingProblemBackend else # if this post request fails, the error callback will catch it $.post(@ajax_url + cmd, data, callback) + .error => callback({success: false, error: "Error occured while performing this operation"}) mock: (cmd, data) -> if cmd == 'is_student_calibrated' From 5cd9641f24ef49e1084b78db3d59cca2370f296a Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Tue, 7 May 2013 17:35:00 -0400 Subject: [PATCH 7/7] Update peer grading comment --- common/lib/xmodule/xmodule/peer_grading_module.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/peer_grading_module.py b/common/lib/xmodule/xmodule/peer_grading_module.py index bb14eec8b5..1ad31922f5 100644 --- a/common/lib/xmodule/xmodule/peer_grading_module.py +++ b/common/lib/xmodule/xmodule/peer_grading_module.py @@ -93,7 +93,7 @@ class PeerGradingModule(PeerGradingFields, XModule): if not self.ajax_url.endswith("/"): self.ajax_url = self.ajax_url + "/" - #This could result in an exception, but not wrapping in a try catch block so it moves up the stack + #StringyInteger could return None, so keep this check. if not isinstance(self.max_grade, int): raise TypeError("max_grade needs to be an integer.")