From 66bfd9baaa53d3c4545676032500eb052a1f65e3 Mon Sep 17 00:00:00 2001 From: Or Ronai Date: Fri, 15 Oct 2021 13:12:18 +0300 Subject: [PATCH 1/2] fix: assessments buttons - Unfocus after clicking an assessment - Assessment is now changed after select and not after fully checked solution --- lms/lmsdb/models.py | 26 +++++++++++-------------- lms/lmsweb/views.py | 16 +++++++++++----- lms/models/solutions.py | 13 ++++++++----- lms/static/checker.js | 23 ++++++++++++++++------ tests/test_notifications.py | 2 +- tests/test_solutions.py | 38 +++++++++++++++++++++++++------------ 6 files changed, 74 insertions(+), 44 deletions(-) diff --git a/lms/lmsdb/models.py b/lms/lmsdb/models.py index fb7ba0b1..b0ac75f7 100644 --- a/lms/lmsdb/models.py +++ b/lms/lmsdb/models.py @@ -619,16 +619,11 @@ def view_solution(self) -> None: def start_checking(self) -> bool: return self.set_state(Solution.STATES.IN_CHECKING) - def set_state( - self, new_state: SolutionState, - assessment: Optional[SolutionAssessment] = None, **kwargs, - ) -> bool: + def set_state(self, new_state: SolutionState, **kwargs) -> bool: # Optional: filter the old state of the object # to make sure that no two processes set the state together requested_solution = (Solution.id == self.id) updates_dict = {Solution.state.name: new_state.name} - if assessment is not None: - updates_dict[Solution.assessment.name] = assessment changes = Solution.update( **updates_dict, **kwargs, ).where(requested_solution) @@ -771,19 +766,20 @@ def _base_next_unchecked(cls): cls.submission_timestamp.asc(), ) + def change_assessment(self, assessment_id: Optional[int] = None) -> bool: + assessment = SolutionAssessment.get_or_none( + SolutionAssessment.id == assessment_id, + ) + requested_solution = (Solution.id == self.id) + updates_dict = {Solution.assessment.name: assessment} + changes = Solution.update(**updates_dict).where(requested_solution) + return changes.execute() == 1 + def mark_as_checked( self, - assessment_id: Optional[int] = None, by: Optional[Union[User, int]] = None, ) -> bool: - assessment = SolutionAssessment.get_or_none( - SolutionAssessment.id == assessment_id, - ) - return self.set_state( - Solution.STATES.DONE, - assessment=assessment, - checker=by, - ) + return self.set_state(Solution.STATES.DONE, checker=by) @classmethod def next_unchecked(cls) -> Optional['Solution']: diff --git a/lms/lmsweb/views.py b/lms/lmsweb/views.py index 05b934d3..53dc94de 100644 --- a/lms/lmsweb/views.py +++ b/lms/lmsweb/views.py @@ -673,17 +673,23 @@ def shared_solution(shared_url: str, file_id: Optional[int] = None): ) -@webapp.route('/checked//', methods=['POST']) +@webapp.route('/assessment/', methods=['POST']) @login_required @managers_only -def done_checking(exercise_id, solution_id): +def assessment(solution_id: int): if request.method == 'POST': assessment_id = request.json.get('assessment') else: # it's a GET assessment_id = request.args.get('assessment') - is_updated = solutions.mark_as_checked( - solution_id, current_user.id, assessment_id, - ) + updated = solutions.change_assessment(solution_id, assessment_id) + return jsonify({'success': updated}) + + +@webapp.route('/checked//', methods=['POST']) +@login_required +@managers_only +def done_checking(exercise_id: int, solution_id: int): + is_updated = solutions.mark_as_checked(solution_id, current_user.id) next_solution = solutions.get_next_unchecked(exercise_id) next_solution_id = getattr(next_solution, 'id', None) return jsonify({'success': is_updated, 'next': next_solution_id}) diff --git a/lms/models/solutions.py b/lms/models/solutions.py index 6cf1c80f..541a6c39 100644 --- a/lms/models/solutions.py +++ b/lms/models/solutions.py @@ -65,13 +65,16 @@ def get_message_and_addressee( return msg, addressee -def mark_as_checked( - solution_id: int, checker_id: int, assessment_id: Optional[int] = None, +def change_assessment( + solution_id: int, assessment_id: Optional[int] = None, ) -> bool: checked_solution: Solution = Solution.get_by_id(solution_id) - is_updated = checked_solution.mark_as_checked( - assessment_id=assessment_id, by=checker_id, - ) + return checked_solution.change_assessment(assessment_id=assessment_id) + + +def mark_as_checked(solution_id: int, checker_id: int) -> bool: + checked_solution: Solution = Solution.get_by_id(solution_id) + is_updated = checked_solution.mark_as_checked(by=checker_id) msg = _( 'Your solution for the "%(subject)s" exercise has been checked.', subject=checked_solution.exercise.subject, diff --git a/lms/static/checker.js b/lms/static/checker.js index 628f63bc..d37addf9 100644 --- a/lms/static/checker.js +++ b/lms/static/checker.js @@ -1,7 +1,5 @@ function trackFinished(exerciseId, solutionId, element) { element.addEventListener('click', () => { - const assessment = document.querySelector('input[name="assessment"]:checked'); - const assessmentValue = (assessment !== null) ? assessment.value : null; const xhr = new XMLHttpRequest(); xhr.open('POST', `/checked/${exerciseId}/${solutionId}`, true); xhr.setRequestHeader('Content-Type', 'application/json'); @@ -20,9 +18,7 @@ function trackFinished(exerciseId, solutionId, element) { } }; - xhr.send(JSON.stringify({ - assessment: assessmentValue, - })); + xhr.send(JSON.stringify({})); }); } @@ -30,10 +26,25 @@ function changeAssessmentsAttributes(assessmentGroup, item) { if (item.value == assessmentGroup.dataset.checkedid) { item.removeAttribute('checked'); item.checked = false; - assessmentGroup.dataset.checkedid = 'None'; + assessmentGroup.dataset.checkedid = null; } else { assessmentGroup.dataset.checkedid = item.value; } + document.activeElement.blur(); + + const xhr = new XMLHttpRequest(); + xhr.open('POST', `/assessment/${solutionId}`, true); + xhr.setRequestHeader('Content-Type', 'application/json'); + xhr.responseType = 'json'; + xhr.onreadystatechange = () => { + if (xhr.readyState === 4) { + if (xhr.status !== 200) { + console.log(xhr.status); + } + } + }; + + xhr.send(JSON.stringify({assessment: assessmentGroup.dataset.checkedid})); } function trackAssessmentButtons() { diff --git a/tests/test_notifications.py b/tests/test_notifications.py index a9b77808..1d1f2704 100644 --- a/tests/test_notifications.py +++ b/tests/test_notifications.py @@ -175,7 +175,7 @@ def test_user_commented_after_check( client = conftest.get_logged_user(student_user.username) # Marking the solution as checked - solutions.mark_as_checked(solution.id, staff_user.id, 1) + solutions.mark_as_checked(solution.id, staff_user.id) solution = Solution.get_by_id(solution.id) # Sending comments after solution checked diff --git a/tests/test_solutions.py b/tests/test_solutions.py index 5137b87d..90e59952 100644 --- a/tests/test_solutions.py +++ b/tests/test_solutions.py @@ -132,7 +132,7 @@ def test_mark_as_checked( ): # Basic functionality assert solution.state == Solution.STATES.CREATED.name - marked = solutions.mark_as_checked(solution.id, staff_user.id, 2) + marked = solutions.mark_as_checked(solution.id, staff_user.id) # HELL WITH PEEWEE!!! solution = Solution.get_by_id(solution.id) assert marked @@ -142,7 +142,7 @@ def test_mark_as_checked( # Not duplicating things staff_user2 = conftest.create_staff_user(index=1) solution2 = conftest.create_solution(exercise, student_user) - marked = solutions.mark_as_checked(solution2.id, staff_user2.id, 3) + marked = solutions.mark_as_checked(solution2.id, staff_user2.id) solution2 = Solution.get_by_id(solution2.id) assert solution2.state == Solution.STATES.DONE.name assert solution2.checker == staff_user2 @@ -171,13 +171,13 @@ def test_get_next_unchecked( assert unchecked.exercise.id == solution1.exercise.id assert unchecked == solution1 - solutions.mark_as_checked(solution1.id, staff_user, 4) + solutions.mark_as_checked(solution1.id, staff_user) unchecked = solutions.get_next_unchecked(exercise.id) assert unchecked is not None assert unchecked.exercise.id == solution3.exercise.id assert unchecked == solution3 - solutions.mark_as_checked(solution3.id, staff_user, 1) + solutions.mark_as_checked(solution3.id, staff_user) unchecked = solutions.get_next_unchecked(exercise.id) assert unchecked is None @@ -185,7 +185,7 @@ def test_get_next_unchecked( assert unchecked is not None assert unchecked == solution2 - solutions.mark_as_checked(solution2.id, staff_user, 2) + solutions.mark_as_checked(solution2.id, staff_user) unchecked = solutions.get_next_unchecked() assert unchecked is None @@ -563,7 +563,7 @@ def test_last_view_status( solution = Solution.get_by_id(solution.id) assert solution.last_status_view == SolutionStatusView.NOT_CHECKED.name - solutions.mark_as_checked(solution.id, staff_user.id, 3) + solutions.mark_as_checked(solution.id, staff_user.id) solution = Solution.get_by_id(solution.id) assert solution.last_status_view == SolutionStatusView.NOT_CHECKED.name client.get(f'/view/{solution.id}') @@ -590,8 +590,6 @@ def test_done_checking( client = conftest.get_logged_user(staff_user.username) response = client.post( f'/checked/{solution.exercise.id}/{solution.id}', - data=json.dumps({'assessment': 1}), - content_type='application/json', ) assert response.status_code == 200 @@ -636,14 +634,30 @@ def test_solutions_of_user( solution: Solution, _assessments, ): conftest.create_usercourse(student_user, course) - solutions.mark_as_checked(solution.id, staff_user.id, 2) + client = conftest.get_logged_user(staff_user.username) + client.post( + f'/assessment/{solution.id}', + data=json.dumps({'assessment': 2}), + content_type='application/json', + ) solution = Solution.get_by_id(solution.id) + assert solution.assessment.name == 'Nice' + + client.post( + f'/assessment/{solution.id}', + data=json.dumps({'assessment': None}), + content_type='application/json', + ) exercise2 = conftest.create_exercise(course, 2) solution2 = conftest.create_solution(exercise2, student_user) - solutions.mark_as_checked(solution2.id, staff_user.id) + client.post( + f'/assessment/{solution2.id}', + data=json.dumps({'assessment': 3}), + content_type='application/json', + ) solution2 = Solution.get_by_id(solution2.id) exercises = solution.of_user(student_user.id, from_all_courses=True) - assert exercises[0].get('assessment') == 'Nice' - assert exercises[1].get('assessment') is None + assert exercises[0].get('assessment') is None + assert exercises[1].get('assessment') == 'Try again' From e10165deb95de0ba94b8bf558cefd0963379debd Mon Sep 17 00:00:00 2001 From: Or Ronai Date: Fri, 15 Oct 2021 13:19:15 +0300 Subject: [PATCH 2/2] removed unnecessary condition --- lms/lmsweb/views.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lms/lmsweb/views.py b/lms/lmsweb/views.py index 53dc94de..9eeb6709 100644 --- a/lms/lmsweb/views.py +++ b/lms/lmsweb/views.py @@ -677,10 +677,7 @@ def shared_solution(shared_url: str, file_id: Optional[int] = None): @login_required @managers_only def assessment(solution_id: int): - if request.method == 'POST': - assessment_id = request.json.get('assessment') - else: # it's a GET - assessment_id = request.args.get('assessment') + assessment_id = request.json.get('assessment') updated = solutions.change_assessment(solution_id, assessment_id) return jsonify({'success': updated})