feat: Allow users to comment (#193)

orronai · web-flow · commit 19e1c6722df0 · 2020-10-03T11:00:48.000+03:00
* feat: Allow users to comment

- Users can now comment to their own solutions
- Users can now delete their own comments
- Add unittests
diff --git a/lms/lmsdb/models.py b/lms/lmsdb/models.py
@@ -74,6 +74,10 @@ def get_student_role(cls) -> 'Role':
     def get_staff_role(cls) -> 'Role':
         return cls.get(Role.name == RoleOptions.STAFF.value)
 
+    @classmethod
+    def get_admin_role(cls) -> 'Role':
+        return cls.get(Role.name == RoleOptions.ADMINISTRATOR.value)
+
     @classmethod
     def by_name(cls, name) -> 'Role':
         if name.startswith('_'):
@@ -755,6 +759,7 @@ def _by_file(cls, file_id: int):
             CommentText.id.alias('comment_id'), CommentText.text,
             SolutionFile.id.alias('file_id'),
             User.fullname.alias('author_name'),
+            User.role.alias('author_role'),
         ]
         return (
             cls
diff --git a/lms/lmsweb/config.py.example b/lms/lmsweb/config.py.example
@@ -29,6 +29,9 @@ ERRORS_CSV = 'errors.csv'
 # Shareable option
 SHAREABLE_SOLUTIONS = True
 
+# Users comments option
+USERS_COMMENTS = True
+
 # Babel config
 LANGUAGES = {
     'he': 'Hebrew',
diff --git a/lms/lmsweb/views.py b/lms/lmsweb/views.py
@@ -19,7 +19,7 @@
 from werkzeug.utils import redirect
 
 from lms.lmsdb.models import (
-    ALL_MODELS, Comment, CommentText, Exercise, RoleOptions,
+    ALL_MODELS, Comment, CommentText, Exercise, Role, RoleOptions,
     SharedSolution, Solution, SolutionFile, User, database,
 )
 from lms.lmsweb import babel, routes, webapp
@@ -215,8 +215,8 @@ def _create_comment(
 
     return jsonify({
         'success': 'true', 'text': comment_.comment.text,
-        'author_name': user.fullname, 'is_auto': False, 'id': comment_.id,
-        'line_number': line_number,
+        'author_name': user.fullname, 'author_role': user.role.id,
+        'is_auto': False, 'id': comment_.id, 'line_number': line_number,
     })
 
 
@@ -281,9 +281,20 @@ def comment():
     if act == 'fetch':
         return jsonify(Comment.by_file(file_id))
 
+    if (
+        not webapp.config.get('USERS_COMMENTS', False)
+        and not current_user.role.is_manager
+    ):
+        return fail(403, "You aren't allowed to access this page.")
+
     if act == 'delete':
         comment_id = int(request.args.get('commentId'))
         comment_ = Comment.get_or_none(Comment.id == comment_id)
+        if (
+            comment_.commenter.id != current_user.id
+            and not current_user.role.is_manager
+        ):
+            return fail(403, "You aren't allowed to access this page.")
         if comment_ is not None:
             comment_.delete_instance()
         return jsonify({'success': 'true'})
@@ -508,9 +519,17 @@ def _common_comments(exercise_id=None, user_id=None):
     Most common comments throughout all exercises.
     Filter by exercise id when specified.
     """
-    query = CommentText.filter(**{
-        CommentText.flake8_key.name: None,
-    }).select(CommentText.id, CommentText.text).join(Comment)
+    is_moderator_comments = (
+        (Comment.commenter.role == Role.get_staff_role().id)
+        | (Comment.commenter.role == Role.get_admin_role().id),
+    )
+    query = (
+        CommentText.select(CommentText.id, CommentText.text)
+        .join(Comment).join(User).join(Role).where(
+            CommentText.flake8_key.is_null(True),
+            is_moderator_comments,
+        ).switch(Comment)
+    )
 
     if exercise_id is not None:
         query = (
diff --git a/lms/static/comments.js b/lms/static/comments.js
@@ -27,9 +27,15 @@ function isUserGrader() {
   return ['staff', 'administrator'].includes(sessionStorage.getItem('role'));
 }
 
+function isSolverComment(commentData) {
+  const authorIsSolver = commentData.author_name === sessionStorage.getItem('solver');
+  const allowedComment = sessionStorage.getItem('allowedComment') === 'true';
+  return (authorIsSolver && allowedComment);
+}
+
 function formatCommentData(commentData) {
   let changedCommentText = `<span class="comment-author">${commentData.author_name}:</span> ${commentData.text}`;
-  if (isUserGrader()) {
+  if (isUserGrader() || isSolverComment(commentData)) {
     const deleteButton = `<i class="fa fa-trash grader-delete" aria-hidden="true" data-commentid="${commentData.id}" onclick="deleteComment(${window.fileId}, ${commentData.id});"></i>`;
     changedCommentText = `${deleteButton} ${changedCommentText}`;
   }
@@ -139,6 +145,8 @@ window.addEventListener('load', () => {
   window.solutionId = codeElement.id;
   window.fileId = codeElement.file;
   sessionStorage.setItem('role', codeElement.role);
+  sessionStorage.setItem('solver', codeElement.solver);
+  sessionStorage.setItem('allowedComment', codeElement.allowedComment);
   addLineSpansToPre(document.getElementsByTagName('code'));
   pullComments(window.fileId, treatComments);
 });
diff --git a/lms/templates/view.html b/lms/templates/view.html
@@ -67,7 +67,7 @@ <h1>{{ _('תצוגת תרגיל') }} {{ solution['exercise']['id'] }}: {{ soluti
           </div>
         </div>
       </div>
-      <div id="code-view" data-id="{{ solution['id'] }}" data-file="{{ current_file.id }}" data-exercise="{{ solution['exercise']['id'] }}" data-role="{{ role }}">
+      <div id="code-view" data-id="{{ solution['id'] }}" data-file="{{ current_file.id }}" data-exercise="{{ solution['exercise']['id'] }}" data-role="{{ role }}" data-solver="{{ solution['solver']['fullname'] }}" data-allowed-comment="{{ config['USERS_COMMENTS'] | lower }}">
         <pre><code id="user-code" class="language-{{ current_file | language_name }} line-numbers highlight">{{- current_file.code | trim(chars=' ') | e -}}</code></pre>
       </div>
       {% if test_results and not shared_url %}
@@ -137,8 +137,10 @@ <h2>{{ _('הערות בודק') }}</h2>
 <script src="{{ url_for('static', filename='prism.js') }}"></script>
 {% if not shared_url %}
 <script src="{{ url_for('static', filename='comments.js') }}"></script>
-{%- if is_manager %}
+{%- if is_manager or config.USERS_COMMENTS %}
 <script src="{{ url_for('static', filename='grader.js') }}"></script>
+{% endif -%}
+{%- if is_manager %}
 <script src="{{ url_for('static', filename='keyboard.js') }}"></script>
 {% endif -%}
 {% endif %}
diff --git a/lms/tests/conftest.py b/lms/tests/conftest.py
@@ -56,6 +56,7 @@ def celery_eager():
 @pytest.fixture(autouse=True, scope='session')
 def webapp_configurations():
     webapp.config['SHAREABLE_SOLUTIONS'] = True
+    webapp.config['USERS_COMMENTS'] = True
     webapp.secret_key = ''.join(
         random.choices(string.ascii_letters + string.digits, k=64),
     )
@@ -65,6 +66,14 @@ def disable_shareable_solutions():
     webapp.config['SHAREABLE_SOLUTIONS'] = False
 
 
+def disable_users_comments():
+    webapp.config['USERS_COMMENTS'] = False
+
+
+def enable_users_comments():
+    webapp.config['USERS_COMMENTS'] = True
+
+
 def get_logged_user(username: str) -> FlaskClient:
     client = webapp.test_client()
     client.post(
diff --git a/lms/tests/test_solutions.py b/lms/tests/test_solutions.py
@@ -188,6 +188,111 @@ def test_start_checking(
                 the_solution = Solution.get_by_id(solution.id)
                 assert the_solution.state == Solution.STATES.IN_CHECKING.name
 
+    @staticmethod
+    def test_user_comments(
+        exercise: Exercise,
+        student_user: User,
+    ):
+        solution = conftest.create_solution(exercise, student_user)
+
+        client = conftest.get_logged_user(student_user.username)
+        # Creating a comment
+        comment_response = client.post('/comments', data=json.dumps(dict(
+            fileId=solution.files[0].id, act='create', kind='text',
+            comment='hey', line=1,
+        )), content_type='application/json')
+        assert comment_response.status_code == 200
+
+        # Creating another comment
+        another_comment_response = client.post(
+            '/comments', data=json.dumps(dict(
+                fileId=solution.files[0].id, act='create', kind='text',
+                comment='noice', line=2,
+            )), content_type='application/json',
+        )
+        assert another_comment_response.status_code == 200
+
+        # Removing the second comment
+        json_response_another_comment = json.loads(
+            another_comment_response.get_data(as_text=True),
+        )
+        delete_response = client.get('/comments', query_string=dict(
+            fileId=solution.files[0].id, act='delete',
+            commentId=json_response_another_comment['id'],
+        ), content_type='application/json')
+        assert delete_response.status_code == 200
+
+        # Disabling users comments option
+        conftest.disable_users_comments()
+
+        # Trying to remove a comment
+        json_response_comment = json.loads(
+            comment_response.get_data(as_text=True),
+        )
+        delete_response = client.get('/comments', query_string=dict(
+            fileId=solution.files[0].id, act='delete',
+            commentId=json_response_comment['id'],
+        ), content_type='application/json')
+        assert delete_response.status_code == 403
+
+        # Trying to create a comment
+        disable_comment_response = client.post(
+            '/comments', data=json.dumps(dict(
+                fileId=solution.files[0].id, act='create', kind='text',
+                comment='well well well', line=2,
+            )), content_type='application/json',
+        )
+        assert disable_comment_response.status_code == 403
+
+    @staticmethod
+    def test_staff_and_user_comments(
+        exercise: Exercise,
+        student_user: User,
+        staff_user: User,
+    ):
+        solution = conftest.create_solution(exercise, student_user)
+
+        client = conftest.get_logged_user(staff_user.username)
+        # Enabling user comments option
+        conftest.enable_users_comments()
+        # Creating a comment
+        comment_response = client.post('/comments', data=json.dumps(dict(
+            fileId=solution.files[0].id, act='create', kind='text',
+            comment='try again', line=1,
+        )), content_type='application/json')
+        assert comment_response.status_code == 200
+
+        # Creating another comment
+        another_comment_response = client.post(
+            '/comments', data=json.dumps(dict(
+                fileId=solution.files[0].id, act='create', kind='text',
+                comment='hey', line=1,
+            )), content_type='application/json',
+        )
+        assert another_comment_response.status_code == 200
+
+        # Removing the second comment
+        json_response_another_comment = json.loads(
+            another_comment_response.get_data(as_text=True),
+        )
+        delete_response = client.get('/comments', query_string=dict(
+            fileId=solution.files[0].id, act='delete',
+            commentId=json_response_another_comment['id'],
+        ), content_type='application/json')
+        assert delete_response.status_code == 200
+
+        conftest.logout_user(client)
+        client2 = conftest.get_logged_user(student_user.username)
+        # Trying to remove a comment
+        json_response_comment = json.loads(
+            comment_response.get_data(as_text=True),
+        )
+        delete_response = client2.get('/comments', query_string=dict(
+            fileId=solution.files[0].id, act='delete',
+            commentId=json_response_comment['id'],
+        ), content_type='application/json')
+        assert delete_response.status_code == 403
+
     @staticmethod
     def test_share_solution_by_another_user(
         exercise: Exercise,
