Add Publish Grading Feature #965

emptygx · 2023-07-07T19:48:41Z

To be merged with Frontend #2856

For implementation decisions, see Frontend PR.

Changes

Assessment Table: Add new is_grading_published field which determines if students can see their grades.
Assessment Config Table: Add new is_grading_auto_published field which determines if grades are auto published for auto graded assessments.
Backend changes xp/grading details, e.g. grader, autograding_status, back to default if is_grading_published is false. This prevents students from checking response from server to see their grades even though the Frontend hides it.
Notifications Types: Removed autograded and graded types. Added published and unpublished grading types.
Notifications now created when grades are published/unpublished
Implement relevent functions for publishing/unpublishing grades
Admins are able to publish/unpublish all graded submissions
Autograder can autopublish grades when all answers have a success autograding_status
Updated seed to accommodate changes

Current submissions will have the is_grading_published field populated as `true` if all answers have been graded i.e. have a grader_id, while the rest will default to `false`. Future submissions will have the field with default value `false` upon creation.

- Update existing endpoints and queries related to assessment retrieval - Add new routes and endpoints to handle new publish grading feature - Add notification for new feature

YaleChen299 · 2023-07-17T08:09:41Z

Hi! Please fix the format and CI and tag me again when the PR is ready. Thank you!

emptygx · 2023-07-17T08:53:07Z

Hi! Please fix the format and CI and tag me again when the PR is ready. Thank you!

@YaleChen299
Hello! Sorry, I have fixed the formatting. Thank you!

YaleChen299 · 2023-07-17T16:54:25Z

Hi! Please fix the format and CI and tag me again when the PR is ready. Thank you!

@YaleChen299 Hello! Sorry, I have fixed the formatting. Thank you!

No worries! Can you check the CI again? I will approve the CI run when I see then.

coveralls · 2023-07-23T10:04:47Z

coverage: 95.323% (-0.1%) from 95.43%
when pulling b366bc9 on emptygx:add-publish-grading
into d94f60a on source-academy:master.

YaleChen299

Hi @emptygx, I have fixed the formatting and credo issues. However, a few tests are not passing. I suspect that the behaviour of user_total_xp is affected by the behaviour of published/unpublished grades.

What is the expected behaviour for the published/unpublished grades? Is it that users will not see their grades if unpublished?

Could you look into these test cases and fix them? Please document the correct behaviours in terms of the test cases, i.e. for any behaviours that may be affected by the publish grading feature, create at least one positive and one negative test case.

Thank you so much for wrapping up this properly! I think this feature would make the grading activities much more user-friendly!

YaleChen299 · 2023-07-24T05:41:32Z

lib/cadet/assessments/assessments.ex

@@ -298,6 +308,11 @@ defmodule Cadet.Assessments do
      |> where([s], s.student_id == ^cr.id)
      |> select([s], [:assessment_id, :status])

+    grading_published_status =


this subquery can be merged with the previous one.

YaleChen299 · 2023-07-24T05:41:56Z

lib/cadet/assessments/assessments.ex

@@ -309,11 +324,15 @@ defmodule Cadet.Assessments do
        on: a.id == sa.assessment_id
      )
      |> join(:left, [a, _], s in subquery(submission_status), on: a.id == s.assessment_id)
-      |> select([a, sa, s], %{
+      |> join(:left, [a, _, _], gp in subquery(grading_published_status),


similarly here, this can be merged with the other query.

YaleChen299 · 2023-07-24T05:51:35Z

lib/cadet/assessments/assessments.ex

        a
-        | xp: sa.xp,
+        | xp: fragment("CASE WHEN ? THEN ? ELSE 0 END", gp.is_grading_published, sa.xp),


I personally feel that this hiding logic should be done at the Controller level, before sending the response to the front end, since technically the xp is not zero to the backend's knowledge(this will not affect the current behaviours but this will make the code more maintainable in the future as other feature might need to use this xp value).

YaleChen299 · 2023-07-24T05:52:59Z

lib/cadet/assessments/assessments.ex

+        # and return all answers
+        |> select([a, s], %{
+          a
+          | xp: fragment("CASE WHEN ? THEN ? ELSE 0 END", s.is_grading_published, a.xp),


Similar comment. I think the hiding logic should be done at the controller layer as other logic might use the xp value. However, I am okay with this logic at the moment since our models are already filled with a lot of controller logic, and this is a bit troublesome to do at the controller level. What do you think?

I found this to be the easier and more straightforward way to filter the data, but if maintainability may be an issue, then doing it at the controller level might be better like you said. I am not too sure how to go about that though and may need some guidance!

I agree that making this filtering at the controller level is a bit more troublesome. I think what we can do is at least separate this filtering from the main subquery selection. If you can make it into a function to be piped through with sematic name like filter_published_xp, I will be happy to accept that.

lib/cadet/accounts/notification_type.ex

YaleChen299 · 2023-07-24T06:41:27Z

lib/cadet/assessments/assessments.ex

+            role == :admin or bypass or
+              Cadet.Accounts.Query.avenger_of?(cr, submission.student_id)} do
+      Multi.new()
+      |> Multi.run(


I think there is no need to use multi and transaction here since there is only a single update. May II ask what was the reason for this?

lib/cadet_web/admin_controllers/admin_assets_controller.ex

lib/cadet/devices/devices.ex

lib/cadet_web/admin_controllers/admin_grading_controller.ex

YaleChen299 · 2023-07-24T07:05:11Z

(part of frontend issue #2522)

Currently, the grading states design and business logic cause ambiguity with resubmission of assessments being assumed to be graded immediately, and with grades being displayed when they should not.

This feature aims to solve this issue by introducing a new grading state published and a new backend boolean is_grading_published, allowing explicit publishing of student's grades on the frontend and preventing the fetching of grades if they have yet to be published, hence creating a better logic flow.

This pull request contains changes to be made to the backend to enable this new feature.

Re-thinking about the problem that we are trying to solve, I think we need to auto un-publish the grades when we unsubmit.
The main point of publish grading is to solve resubmitted submissions shown as graded to graders so that grader will not know if the student is graded after resubmission.
This logic is a bit different then whether the grade is publish to the student or not. Please correct me if i am wrong! Thanks!

emptygx · 2023-07-24T09:59:06Z

Re-thinking about the problem that we are trying to solve, I think we need to auto un-publish the grades when we unsubmit. The main point of publish grading is to solve resubmitted submissions shown as graded to graders so that grader will not know if the student is graded after resubmission. This logic is a bit different then whether the grade is publish to the student or not. Please correct me if i am wrong! Thanks!

Yes, I agree that should be part of the feature, which has already been implemented as part of the logic on the front end, which auto-unpublishes grades whenever an assessment is unsubmitted. Sorry for not mentioning that in the PR description, will make sure to add it into future documentation! Should I add it to the PR description as well?

Oh, thanks for the clarification! In that case what if the unsubmit went through successfully but the unpublish request failed? Will it be better for that to be handled in one single request? @RichDom2185

…precated ones

Outdated review

This commit prevents admin/staff from individually publishing/unpublishing grades for students.

Ensures total ordering of migration files are preserved.

RichDom2185

Hi, thanks for working on this! Just some comments below, thanks!

lib/cadet/assessments/assessments.ex

RichDom2185 · 2024-04-11T18:53:08Z

lib/cadet/assessments/assessments.ex

+      answers_query =
+        Answer
+        |> group_by([ans], ans.submission_id)
+        |> select([ans], %{
+          submission_id: ans.submission_id,
+          graded_count: filter(count(ans.id), not is_nil(ans.grader_id)),
+          autograded_count: filter(count(ans.id), ans.autograding_status == :success)
+        })
+
+      question_query =
+        Question
+        |> group_by([q], q.assessment_id)
+        |> join(:inner, [q], a in Assessment, on: q.assessment_id == a.id)
+        |> select([q, a], %{
+          assessment_id: q.assessment_id,
+          question_count: count(q.id)
+        })


Correct me if I'm wrong, but since these are subqueries, they'll run first in entirety before any downstream filtering.

So in order to avoid the ~30s query from the joining of the large table, should we filter by course ID here first?

I managed to filter the Question table subquery but the Answer table subquery requires quite a few joins to obtain the course_id to filter by. I was wondering if it is worth the multiple joins since I'm not too sure about costs in SQL.

Joins:
Answer -> Question -> Assessment (course_id)

I also thought of combining the queries together since I already use Question -> Assessment but you cant use aggregate functions e.g. filter and count

GabrielCWT · 2024-04-07T10:10:14Z

lib/cadet/jobs/autograder/grading_job.ex

@@ -258,6 +260,14 @@ defmodule Cadet.Autograder.GradingJob do
    )
  end

-  defp grade_submission_question_answer_lists(_, [], [], _, _) do
+  defp grade_submission_question_answer_lists(submission_id, [], [], _, _) do


Would it be better to query as follows?

submission = Submission |> join(:inner, [s], a in assoc(s, :assessment)) |> join(:inner, [_, a], c in assoc(a, :config)) |> preload([_, a, c], assessment: {a, config: c}) |> Repo.get(submission_id) is_grading_auto_published = submission.assessment.config.is_grading_auto_published

GabrielCWT · 2024-04-07T10:12:41Z

lib/cadet/jobs/autograder/result_store_worker.ex

+      |> Answer.autograding_changeset(changes)
+      |> Repo.update()
+
+    submission = Repo.get(Submission, submission_id)


Same question as before.

RichDom2185

LGTM! Let's merge this first to not block the other team's PRs. As to your question, let's discuss it after this is merged.

Thanks for the work!

emptygx added 5 commits July 3, 2023 11:01

Merge branch 'master' into add-publish-grading

8125ac1

Add and update endpoints for new publish grading feature

e286902

- Update existing endpoints and queries related to assessment retrieval - Add new routes and endpoints to handle new publish grading feature - Add notification for new feature

Fix missing return tuple values in assessments.ex

d174fbd

Fix notification and formatting

bc36b60

emptygx marked this pull request as ready for review July 12, 2023 09:46

emptygx added 2 commits July 16, 2023 18:43

Merge branch 'master' into add-publish-grading

5dcc425

Fix indentation and minor error

2153280

RichDom2185 requested review from YaleChen299 and RichDom2185 July 17, 2023 06:57

Fix formatting

1891d72

fix formatting

55c5a36

YaleChen299 previously requested changes Jul 23, 2023

View reviewed changes

YaleChen299 reviewed Jul 24, 2023

View reviewed changes

YaleChen299 added 2 commits July 24, 2023 14:20

fix credo long line

af683fd

fix credo nesting alias

cf98cff

YaleChen299 force-pushed the add-publish-grading branch from 3ce9105 to cf98cff Compare July 24, 2023 06:22

YaleChen299 reviewed Jul 24, 2023

View reviewed changes

lib/cadet/accounts/notification_type.ex Outdated Show resolved Hide resolved

YaleChen299 reviewed Jul 24, 2023

View reviewed changes

lib/cadet_web/admin_controllers/admin_assets_controller.ex Outdated Show resolved Hide resolved

YaleChen299 reviewed Jul 24, 2023

View reviewed changes

lib/cadet/devices/devices.ex Outdated Show resolved Hide resolved

YaleChen299 reviewed Jul 24, 2023

View reviewed changes

lib/cadet_web/admin_controllers/admin_grading_controller.ex Show resolved Hide resolved

YaleChen299 self-assigned this Jul 25, 2023

Fix formatting and code quality

568cc6f

GabrielCWT added 3 commits March 27, 2024 23:47

Merge branch 'master' into add-publish-grading

a2a23f7

feat: Implement published and unpublished notifications and remove de…

5b91c12

…precated ones

feat: Include isGradingAutoPublished in response for assessment configs

64b5305

fix: Include sending of notifications when publishing/unpublishing all

21c65f6

This was referenced Apr 2, 2024

Dashboard routes: update from ungraded tracking by group to unpublished tracking by group #1099

Open

Dashboard: update from ungraded tracking by group to unpublished tracking source-academy/frontend#2898

Open

GabrielCWT added 8 commits April 2, 2024 19:55

docs: Add docs for functions implemented

596d3c3

chore: Update wording for tests

340de3a

chore: Remove unused variables

b2ab4b0

feat: Implement test for unpublish and publish all routes

a25f9f7

Merge branch 'master' into add-publish-grading

9d9069c

feat: Implement guard for publish/unpublish grades

fd1508b

This commit prevents admin/staff from individually publishing/unpublishing grades for students.

refactor: Change notification types in swagger_schema

4ac87af

chore: Add comment in seed

93d6bd2

thortol mentioned this pull request Apr 8, 2024

Add sorting for grading #1107

Merged

7 tasks

GabrielCWT and others added 6 commits April 8, 2024 21:30

refactor: remove redundant lines

bd9abf9

Merge branch 'master' into add-publish-grading

c34d265

Merge branch 'master' into add-publish-grading

365cfb4

Redate migrations

ce6ec53

Ensures total ordering of migration files are preserved.

Revert unnecessary changes

710213c

Revert more unnecessary changes

593708e

RichDom2185 reviewed Apr 11, 2024

View reviewed changes

GabrielCWT reviewed Apr 12, 2024

View reviewed changes

GabrielCWT and others added 4 commits April 12, 2024 11:55

refactor: Move duplicate code into helper function

d00b686

chore: Fix typo

e355d54

refactor: Change control flow structure

541fac9

Merge branch 'master' into add-publish-grading

b366bc9

RichDom2185 approved these changes Apr 13, 2024

View reviewed changes

RichDom2185 merged commit 3cf8d7d into source-academy:master Apr 13, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Publish Grading Feature #965

Add Publish Grading Feature #965

emptygx commented Jul 7, 2023 •

edited by GabrielCWT

Loading

YaleChen299 commented Jul 17, 2023

emptygx commented Jul 17, 2023

YaleChen299 commented Jul 17, 2023

coveralls commented Jul 23, 2023 •

edited

Loading

YaleChen299 left a comment •

edited

Loading

YaleChen299 Jul 24, 2023

YaleChen299 Jul 24, 2023

YaleChen299 Jul 24, 2023

YaleChen299 Jul 24, 2023 •

edited

Loading

emptygx Jul 25, 2023

YaleChen299 Jul 28, 2023

YaleChen299 Jul 24, 2023

YaleChen299 commented Jul 24, 2023

emptygx commented Jul 24, 2023 •

edited by YaleChen299

Loading

RichDom2185 left a comment

RichDom2185 Apr 11, 2024

GabrielCWT Apr 12, 2024

GabrielCWT Apr 7, 2024

GabrielCWT Apr 7, 2024

RichDom2185 left a comment

Add Publish Grading Feature #965

Add Publish Grading Feature #965

Conversation

emptygx commented Jul 7, 2023 • edited by GabrielCWT Loading

Changes

YaleChen299 commented Jul 17, 2023

emptygx commented Jul 17, 2023

YaleChen299 commented Jul 17, 2023

coveralls commented Jul 23, 2023 • edited Loading

YaleChen299 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YaleChen299 Jul 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YaleChen299 commented Jul 24, 2023

emptygx commented Jul 24, 2023 • edited by YaleChen299 Loading

RichDom2185 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichDom2185 left a comment

Choose a reason for hiding this comment

emptygx commented Jul 7, 2023 •

edited by GabrielCWT

Loading

coveralls commented Jul 23, 2023 •

edited

Loading

YaleChen299 left a comment •

edited

Loading

YaleChen299 Jul 24, 2023 •

edited

Loading

emptygx commented Jul 24, 2023 •

edited by YaleChen299

Loading