Add submision conversion feature for individual submission and by module #991

muhammadfaiz12 · 2022-02-21T09:28:07Z

Late submission conversion

EDIT-767

Description

What?

This feature convert a late submission to a normal submission without late penalty.

Why?

Teaching staff could convert a late submission

How?

Removing late penalties and changing the submission status

Testing

Remember to add or update unit tests for new features and changes.

How to test your changes in A-plus
How to test accessibility

What type of test did you run?

I test the fature manually and checking wether a late submission was actually converted and get the grade without penalities.
Did you test the changes in

Chrome
Firefox
This pull request cannot be tested in the browser.

Think of what is affected by these changes and could become broken

Translation

Did you modify or add new strings in the user interface? (Read about how to create translation)

Programming style

Did you follow our style guides?
Did you use Python type hinting in all functions that you added or edited? (type hints for function parameters and return values)

Have you updated the README or other relevant documentation?

documents inside the doc directory.
README.md.
Aplus Manual.
Other documentation (mention below which documentation).

Is it Done?

Reviewer has finished the code review
After the review, the developer has made changes accordingly
Customer/Teacher has accepted the implementation of the feature

Clean up your git commit history before submitting the pull request!

PasiSa

Here are some comments from me.

The commit log message could be a bit more verbose in my mind. "Submission conversion feature" is not so clear if you do not know what this relates to. After a short title line, perhaps sentence or two to clarify what the feature does would be useful. You could also add reference to the issue: "Closes #892", to link the commit with the original issue (and automatically close it).

Could you write short step-by-step instructions, for example as a follow-up comment in this pull request feed, on how to test this feature? (See recent discussion on EDIT Teams channel about improving our documentation on testing)

PasiSa · 2022-02-22T08:32:21Z

exercise/staff_views.py

+    A POST-only view that updates a student's late or unofficial submission
+    to normal submission. Changed the status and remove the penalty
+    """
+    def post(self, request: HttpRequest, *args: Any, **kwargs: Any) -> HttpResponse:


Is this missing access_mode = ACCESS.ASSISTANT?

Well, i'm not sure, but if you think i should add that then i could do that

Actually, it should be ACCESS.GRADING so that it takes the setting exercise.allow_assistant_grading True/False into account.

exercise/staff_views.py

exercise/templates/exercise/staff/_assessment_panel.html

locale/en/LC_MESSAGES/django.po

PasiSa · 2022-02-22T08:40:01Z

locale/fi/LC_MESSAGES/django.po


+#: exercise/templates/exercise/_user_results.html
+msgid "CONVERT_SUBMISSION_BY_MODULE"
+msgstr "Convert Late Submissions in Finnish"


I can help with Finnish translations, once the terminology is fixed (relating to above comment on English texts)

Yes, thanks for the help

I will add Finnish translations now that I am fixing many details in this PR.

conversion EDIT-767

markkuriekkinen

Nice work with your first feature!

I will fix the issues in this pull request myself due to time constraints. I commented on some issues below. There are coding style issues, some significant bugs, lacking access control and user interface improvements.

You could have made more unit tests than just a single very simple test.

markkuriekkinen · 2022-03-31T12:34:03Z

deviations/viewbase.py

@@ -1,4 +1,5 @@
 from itertools import groupby
+from lib2to3.pytree import convert


Isn't this import unused? Remove this line.

markkuriekkinen · 2022-03-31T12:38:05Z

deviations/viewbase.py

 from django.utils.translation import ugettext_lazy as _, ngettext
+from django.utils import timezone
+from django.utils.dateparse import parse_datetime
+import datetime


The correct coding style is to place standard imports (built-in Python modules) before the third-party libraries. You can move import datetime to the top above all other imports.

markkuriekkinen · 2022-04-01T10:33:05Z

exercise/tests.py

+    def test_submission_late_conversion(self):
+        convert_submission_url = self.late_submission.get_url('submission-conversion')
+        response = self.client.get(convert_submission_url)
+        self.assertEqual(response.status_code, 302)
+        self.assertTrue(self.late_submission.late_penalty_applied is None)
+


It would make sense to add more tests than just this. For example, there could be unit tests that directly call the method Submission.convert_penalized_submission() without making HTTP requests with self.client.

markkuriekkinen · 2022-04-01T10:35:12Z

exercise/templates/exercise/_user_results.html

+
+		{% if module.late_allowed and module.late_percent > 0 %}
+
+		{% endif %}
+


Why is this empty if here?

markkuriekkinen · 2022-04-01T10:37:50Z

exercise/urls.py

        staff_views.FetchMetadataView.as_view(),
        name="exercise-metadata"),
-
+    url(EDIT_URL_PREFIX+r'convert/module/$',


Suggested change

url(EDIT_URL_PREFIX+r'convert/module/$',

url(EDIT_URL_PREFIX + r'convert/module/$',

markkuriekkinen · 2022-04-06T10:30:59Z

deviations/viewbase.py

+    new_date = form_data.get('new_date')
+    for submitter in submitters:
+        for exercise in exercises:        
+            submissions = exercise.get_submissions_for_student(submitter, exclude_errors=True)


These database queries could be made more efficient. Now, this fetches also normal, graded submissions that couldn't be approved. There could be many such submissions, but they are not needed here.

Note that late submissions do not always have any late penalty and thus, it is not enough to check submissions with submission.late_penalty_applied is not None. If the module does not allow late submissions, but the exercise category allows unofficial submissions, then late submissions become unofficial without any late penalty (submission.late_penalty_applied is None).

If the teacher adds deviations to many exercises and students at once, then making database queries separately for each exercise and student pair might be needlessly slow. Making hundreds of database queries for a single HTTP request is never good. It is possible to fetch more data in a single query.

markkuriekkinen · 2022-04-06T16:22:53Z

exercise/staff_views.py

+
+    user_kw = 'user_id'
+    module_kw = 'module_id'
+    access_mode = ACCESS.ASSISTANT


We need to also check exercise.allow_assistant_grading separately.

markkuriekkinen · 2022-04-06T16:23:22Z

exercise/staff_views.py

+
+        #getting module and user by id.
+        self.module = get_object_or_404(
+                        CourseModule,


Bad indentation.

markkuriekkinen · 2022-04-06T16:26:56Z

exercise/staff_views.py

+            exercises = BaseExercise.objects.filter(course_module=self.module)
+
+        for exercise in exercises:
+            submissions = exercise.get_submissions_for_student(self.student.userprofile, exclude_errors=True)


The database queries can be made more efficient. It is not necessary to iterate over all the normal submissions.

Note that if a course module does not allow late submissions, but unofficial submissions are allowed in the exercise category, then late submissions become unofficial without any late penalty. Though, since the user interface has choices "late only" or "unofficial only", then "late only" should imply that it does not pick any unofficial submissions. Note that unofficial submissions may have late penalty set when the module allows late submissions.

markkuriekkinen · 2022-04-06T16:29:11Z

locale/fi/LC_MESSAGES/django.po


+#: exercise/templates/exercise/_user_results.html
+msgid "CONVERT_SUBMISSION_BY_MODULE"
+msgstr "Convert Late Submissions in Finnish"


I will add Finnish translations now that I am fixing many details in this PR.

Course staff may approve late and/or unofficial submissions so that any point penalties are removed from those submissions and they become normal, graded submissions (with the "ready" status). Single submissions may be approved or all submissions from one student in one assignment or one whole course module. The whole assignment or module approval may also be set to target only late submissions or only unofficial submissions. The user interface for this feature is visible in the inspect submission view when inspecting a late or unofficial submission. Part of apluslms#892. "The first pull request" in the comment: apluslms#892 (comment) This work started in Muhammad's pull request apluslms#991. Markku has refactored the code, improved the database queries and the details overall. Co-authored-by: Muhammad Wahjoe <faiz00.muhammad@gmail.com>

Course staff may approve late and/or unofficial submissions so that any point penalties are removed from those submissions and they become normal, graded submissions (with the "ready" status). Single submissions may be approved or all submissions from one student in one assignment or one whole course module. The whole assignment or module approval may also be set to target only late submissions or only unofficial submissions. The user interface for this feature is visible in the inspect submission view when inspecting a late or unofficial submission. Part of #892. "The first pull request" in the comment: #892 (comment) This work started in Muhammad's pull request #991. Markku has refactored the code, improved the database queries and the details overall. Co-authored-by: Muhammad Wahjoe <faiz00.muhammad@gmail.com>

TODO: * +1 max submissions deviation button in the inspect submission view: should this also check if any submission should be approved immediately? * test all the changes * deviations/viewbase.py approve_unofficial_submissions(): implementation unfinished * clean up the code * success messages to the user in the user interface? When deviations are added, the teacher may choose to also immediately approve such existing submissions that are covered by the deviation. For example, if the submission was late, but with the new personal deadline it is not late, then it would be approved. Fixes apluslms#892. "The second pull request" in the comment: apluslms#892 (comment) This work started in Muhammad's pull request apluslms#991. Markku has refactored the code, improved the database queries and the details overall. Co-authored-by: Muhammad Wahjoe <faiz00.muhammad@gmail.com>

markkuriekkinen · 2022-05-06T13:34:43Z

The merged commit 7c65106 implements the basic submission approval in the inspect view: a single submission or all submissions in one assignment or module.

Approving when adding deviations is part of the unfinished draft pull request #1029.

Closing this PR because it had too many issues. The impoved version is done outside this PR.

Course staff may approve late and/or unofficial submissions so that any point penalties are removed from those submissions and they become normal, graded submissions (with the "ready" status). Single submissions may be approved or all submissions from one student in one assignment or one whole course module. The whole assignment or module approval may also be set to target only late submissions or only unofficial submissions. The user interface for this feature is visible in the inspect submission view when inspecting a late or unofficial submission. Part of apluslms#892. "The first pull request" in the comment: apluslms#892 (comment) This work started in Muhammad's pull request apluslms#991. Markku has refactored the code, improved the database queries and the details overall. Co-authored-by: Muhammad Wahjoe <faiz00.muhammad@gmail.com>

PasiSa reviewed Feb 22, 2022

View reviewed changes

PasiSa mentioned this pull request Feb 24, 2022

Check validity of exercise points after admitting deadline deviation or extra attempts #892

Open

PasiSa assigned muhammadfaiz12 Feb 24, 2022

markkuriekkinen linked an issue Feb 24, 2022 that may be closed by this pull request

Check validity of exercise points after admitting deadline deviation or extra attempts #892

Open

muhammadfaiz12 force-pushed the feat/grade-conversion branch from 7184082 to 0611131 Compare March 7, 2022 08:27

markkuriekkinen self-requested a review March 9, 2022 12:26

markkuriekkinen assigned markkuriekkinen and unassigned muhammadfaiz12 Mar 9, 2022

Add submision conversion feature for individual submission and by module

d5fec19

conversion EDIT-767

muhammadfaiz12 force-pushed the feat/grade-conversion branch from 0611131 to d5fec19 Compare March 27, 2022 21:33

markkuriekkinen suggested changes Apr 6, 2022

View reviewed changes

markkuriekkinen mentioned this pull request May 6, 2022

Approve late/unofficial submissions when adding deviations #1029

Draft

18 tasks

markkuriekkinen closed this May 6, 2022

		@@ -1,4 +1,5 @@
		from itertools import groupby
		from lib2to3.pytree import convert


		{% if module.late_allowed and module.late_percent > 0 %}

		{% endif %}

	url(EDIT_URL_PREFIX+r'convert/module/$',
	url(EDIT_URL_PREFIX + r'convert/module/$',

Add submision conversion feature for individual submission and by module #991

Add submision conversion feature for individual submission and by module #991

Uh oh!

Conversation

muhammadfaiz12 commented Feb 21, 2022

Description

Testing

Translation

Programming style

Have you updated the README or other relevant documentation?

Is it Done?

Uh oh!

PasiSa left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markkuriekkinen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markkuriekkinen commented May 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

PasiSa left a comment •

edited

Loading

markkuriekkinen commented May 6, 2022 •

edited

Loading