From 57f14bded165408a550c81e1c3ff179bd47dd374 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 9 Apr 2014 15:06:11 -0400 Subject: [PATCH] Allow extra fields returned from comments service Previously, an error was raised if the comments service returned data including an unexpected field, which unnecessarily complicated the release path for new features, since the list of allowed fields would need to be modified before cs_comments_service could be modified, and only then could edx-platform take advantage of the new CS feature. We still log a warning if an unexpected field is returned, so we will still be able to tell if the CS returns a corrupt response. JIRA: FOR-180 --- .../django_comment_client/base/views.py | 42 ++++++++++--------- lms/lib/comment_client/comment.py | 8 ++-- lms/lib/comment_client/models.py | 22 +++++++--- lms/lib/comment_client/thread.py | 18 ++++---- lms/lib/comment_client/user.py | 10 ++--- 5 files changed, 56 insertions(+), 44 deletions(-) diff --git a/lms/djangoapps/django_comment_client/base/views.py b/lms/djangoapps/django_comment_client/base/views.py index cca662ab9e..673684ff79 100644 --- a/lms/djangoapps/django_comment_client/base/views.py +++ b/lms/djangoapps/django_comment_client/base/views.py @@ -88,14 +88,15 @@ def create_thread(request, course_id, commentable_id): if 'body' not in post or not post['body'].strip(): return JsonError(_("Body can't be empty")) - thread = cc.Thread(**extract(post, ['body', 'title'])) - thread.update_attributes(**{ - 'anonymous': anonymous, - 'anonymous_to_peers': anonymous_to_peers, - 'commentable_id': commentable_id, - 'course_id': course_id, - 'user_id': request.user.id, - }) + thread = cc.Thread( + anonymous=anonymous, + anonymous_to_peers=anonymous_to_peers, + commentable_id=commentable_id, + course_id=course_id, + user_id=request.user.id, + body=post["body"], + title=post["title"] + ) user = cc.User.from_django_user(request.user) @@ -118,7 +119,7 @@ def create_thread(request, course_id, commentable_id): group_id = user_group_id if group_id: - thread.update_attributes(group_id=group_id) + thread.group_id = group_id thread.save() @@ -149,7 +150,8 @@ def update_thread(request, course_id, thread_id): if 'body' not in request.POST or not request.POST['body'].strip(): return JsonError(_("Body can't be empty")) thread = cc.Thread.find(thread_id) - thread.update_attributes(**extract(request.POST, ['body', 'title'])) + thread.body = request.POST["body"] + thread.title = request.POST["title"] thread.save() if request.is_ajax(): return ajax_content_response(request, course_id, thread.to_dict()) @@ -166,7 +168,6 @@ def _create_comment(request, course_id, thread_id=None, parent_id=None): if 'body' not in post or not post['body'].strip(): return JsonError(_("Body can't be empty")) - comment = cc.Comment(**extract(post, ['body'])) course = get_course_with_access(request.user, course_id, 'load') if course.allow_anonymous: @@ -179,14 +180,15 @@ def _create_comment(request, course_id, thread_id=None, parent_id=None): else: anonymous_to_peers = False - comment.update_attributes(**{ - 'anonymous': anonymous, - 'anonymous_to_peers': anonymous_to_peers, - 'user_id': request.user.id, - 'course_id': course_id, - 'thread_id': thread_id, - 'parent_id': parent_id, - }) + comment = cc.Comment( + anonymous=anonymous, + anonymous_to_peers=anonymous_to_peers, + user_id=request.user.id, + course_id=course_id, + thread_id=thread_id, + parent_id=parent_id, + body=post["body"] + ) comment.save() if post.get('auto_subscribe', 'false').lower() == 'true': user = cc.User.from_django_user(request.user) @@ -235,7 +237,7 @@ def update_comment(request, course_id, comment_id): comment = cc.Comment.find(comment_id) if 'body' not in request.POST or not request.POST['body'].strip(): return JsonError(_("Body can't be empty")) - comment.update_attributes(**extract(request.POST, ['body'])) + comment.body = request.POST["body"] comment.save() if request.is_ajax(): return ajax_content_response(request, course_id, comment.to_dict()) diff --git a/lms/lib/comment_client/comment.py b/lms/lib/comment_client/comment.py index c09c7d9c98..3082b1dcc8 100644 --- a/lms/lib/comment_client/comment.py +++ b/lms/lib/comment_client/comment.py @@ -52,14 +52,14 @@ class Comment(models.Model): else: raise CommentClientRequestError("Can only flag/unflag threads or comments") params = {'user_id': user.id} - request = perform_request( + response = perform_request( 'put', url, params, metric_tags=self._metric_tags, metric_action='comment.abuse.flagged' ) - voteable.update_attributes(request) + voteable._update_from_response(response) def unFlagAbuse(self, user, voteable, removeAll): if voteable.type == 'thread': @@ -73,14 +73,14 @@ class Comment(models.Model): if removeAll: params['all'] = True - request = perform_request( + response = perform_request( 'put', url, params, metric_tags=self._metric_tags, metric_action='comment.abuse.unflagged' ) - voteable.update_attributes(request) + voteable._update_from_response(response) def _url_for_thread_comments(thread_id): diff --git a/lms/lib/comment_client/models.py b/lms/lib/comment_client/models.py index 5bb3205641..5a9eb740f3 100644 --- a/lms/lib/comment_client/models.py +++ b/lms/lib/comment_client/models.py @@ -1,6 +1,11 @@ +import logging + from .utils import extract, perform_request, CommentClientRequestError +log = logging.getLogger(__name__) + + class Model(object): accessible_fields = ['id'] @@ -70,7 +75,7 @@ class Model(object): metric_tags=self._metric_tags, metric_action='model.retrieve' ) - self.update_attributes(**response) + self._update_from_response(response) @property def _metric_tags(self): @@ -93,12 +98,17 @@ class Model(object): def find(cls, id): return cls(id=id) - def update_attributes(self, *args, **kwargs): - for k, v in kwargs.items(): + def _update_from_response(self, response_data): + for k, v in response_data.items(): if k in self.accessible_fields: self.__setattr__(k, v) else: - raise AttributeError("Field {0} does not exist".format(k)) + log.warning( + "Unexpected field {field_name} in model {model_name}".format( + field_name=k, + model_name=self.__class__.__name__ + ) + ) def updatable_attributes(self): return extract(self.attributes, self.updatable_fields) @@ -135,14 +145,14 @@ class Model(object): metric_action='model.insert' ) self.retrieved = True - self.update_attributes(**response) + self._update_from_response(response) self.after_save(self) def delete(self): url = self.url(action='delete', params=self.attributes) response = perform_request('delete', url, metric_tags=self._metric_tags, metric_action='model.delete') self.retrieved = True - self.update_attributes(**response) + self._update_from_response(response) @classmethod def url_with_id(cls, params={}): diff --git a/lms/lib/comment_client/thread.py b/lms/lib/comment_client/thread.py index 335f24c9c5..95fbdec084 100644 --- a/lms/lib/comment_client/thread.py +++ b/lms/lib/comment_client/thread.py @@ -98,7 +98,7 @@ class Thread(models.Model): metric_action='model.retrieve', metric_tags=self._metric_tags ) - self.update_attributes(**response) + self._update_from_response(response) def flagAbuse(self, user, voteable): if voteable.type == 'thread': @@ -108,14 +108,14 @@ class Thread(models.Model): else: raise CommentClientRequestError("Can only flag/unflag threads or comments") params = {'user_id': user.id} - request = perform_request( + response = perform_request( 'put', url, params, metric_action='thread.abuse.flagged', metric_tags=self._metric_tags ) - voteable.update_attributes(request) + voteable._update_from_response(response) def unFlagAbuse(self, user, voteable, removeAll): if voteable.type == 'thread': @@ -129,38 +129,38 @@ class Thread(models.Model): if removeAll: params['all'] = True - request = perform_request( + response = perform_request( 'put', url, params, metric_tags=self._metric_tags, metric_action='thread.abuse.unflagged' ) - voteable.update_attributes(request) + voteable._update_from_response(response) def pin(self, user, thread_id): url = _url_for_pin_thread(thread_id) params = {'user_id': user.id} - request = perform_request( + response = perform_request( 'put', url, params, metric_tags=self._metric_tags, metric_action='thread.pin' ) - self.update_attributes(request) + self._update_from_response(response) def un_pin(self, user, thread_id): url = _url_for_un_pin_thread(thread_id) params = {'user_id': user.id} - request = perform_request( + response = perform_request( 'put', url, params, metric_tags=self._metric_tags, metric_action='thread.unpin' ) - self.update_attributes(request) + self._update_from_response(response) def _url_for_flag_abuse_thread(thread_id): diff --git a/lms/lib/comment_client/user.py b/lms/lib/comment_client/user.py index 6d7f44f916..a2e7e42b4c 100644 --- a/lms/lib/comment_client/user.py +++ b/lms/lib/comment_client/user.py @@ -56,14 +56,14 @@ class User(models.Model): else: raise CommentClientRequestError("Can only vote / unvote for threads or comments") params = {'user_id': self.id, 'value': value} - request = perform_request( + response = perform_request( 'put', url, params, metric_action='user.vote', metric_tags=self._metric_tags + ['target.type:{}'.format(voteable.type)], ) - voteable.update_attributes(request) + voteable._update_from_response(response) def unvote(self, voteable): if voteable.type == 'thread': @@ -73,14 +73,14 @@ class User(models.Model): else: raise CommentClientRequestError("Can only vote / unvote for threads or comments") params = {'user_id': self.id} - request = perform_request( + response = perform_request( 'delete', url, params, metric_action='user.unvote', metric_tags=self._metric_tags + ['target.type:{}'.format(voteable.type)], ) - voteable.update_attributes(request) + voteable._update_from_response(response) def active_threads(self, query_params={}): if not self.course_id: @@ -141,7 +141,7 @@ class User(models.Model): ) else: raise - self.update_attributes(**response) + self._update_from_response(response) def _url_for_vote_comment(comment_id):