Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Grade exams manually #1253

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from
Draft

Grade exams manually #1253

wants to merge 13 commits into from

Conversation

Maijjay
Copy link
Contributor

@Maijjay Maijjay commented Mar 19, 2024

No description provided.

.await?;

let mut list: Vec<ExerciseSlideSubmissionAndUserExerciseState> = Vec::new();
for sub in submissions {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be done without the loop to prevent the n+1 problem

FROM exercise_slide_submissions
WHERE exercise_id = $1
AND deleted_at IS NULL
AND created_at in (SELECT MAX(created_at) FROM exercise_slide_submissions GROUP BY user_id, exercise_slide_id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Matches all records that have the same created at.

Mayb you can use the SQL HAVING keyword here?

Ok(count.count.unwrap_or(0).try_into()?)
}

pub async fn exercise_slide_submissions_and_user_exercise_state_list_with_exercise_id(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

method name should say that it gets the latest submission per user

Vec::new();

// Check if student has any published grading results they can view at the exam page
for grading_decision in teachers_grading_decisions_list.into_iter() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to get rid of the n+1


let exercises = models::exercises::get_exercises_by_exam_id(&mut conn, *exam_id).await?;

for exercise in exercises.iter() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

n+1


let exercise_slide_submissions_and_user_exercise_state_list = payload.0;

for submission in exercise_slide_submissions_and_user_exercise_state_list.into_iter() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing this n+1 is not worth the effort

.getByRole("button")
.click()
await teacherPage.locator("#Justification").fill("Ok")
await teacherPage.getByRole("textbox", { name: "undefinedfalse" }).fill("1")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check this

teacherPage.getByRole("row", { name: "Grade Question 1 Graded 2 2 2" }),
).toBeVisible()
await expect(teacherPage.getByText("You have 2 unpublished grading results")).toBeVisible()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before publishing, verify that the students cannot see the results yet

const globalX = pivotPointRect.x - rect.width / 2 + pivotPointRect.width / 2
const globalY = pivotPointRect.y - rect.height + 10

/*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dead code

onClick={() => {
const confirmation = confirm(
// eslint-disable-next-line i18next/no-literal-string
`Do you want to publish all currently graded submissions?`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This string should be translated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants