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

Convert manual grading assessment question page to TS #10052

Merged
merged 36 commits into from
Jul 18, 2024

Conversation

jonatanschroeder
Copy link
Member

No description provided.

Copy link

codecov bot commented Jun 19, 2024

Codecov Report

Attention: Patch coverage is 91.57609% with 31 lines in your changes missing coverage. Please review.

Project coverage is 71.67%. Comparing base (3303c63) to head (ea73c9f).
Report is 2 commits behind head on master.

Files Patch % Lines
...arn/src/components/EditQuestionPointsScore.html.ts 84.73% 19 Missing and 1 partial ⚠️
...ding/assessmentQuestion/assessmentQuestion.html.ts 96.15% 5 Missing ⚠️
...smentInstance/instructorAssessmentInstance.html.ts 89.47% 4 Missing ⚠️
...rc/components/AssessmentOpenInstancesAlert.html.ts 95.65% 1 Missing ⚠️
...alGrading/assessmentQuestion/assessmentQuestion.ts 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10052      +/-   ##
==========================================
- Coverage   71.76%   71.67%   -0.09%     
==========================================
  Files         508      514       +6     
  Lines       80432    80955     +523     
  Branches     6792     6811      +19     
==========================================
+ Hits        57718    58028     +310     
- Misses      22256    22469     +213     
  Partials      458      458              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

github-actions bot commented Jun 19, 2024

All images

Image Platform Old Size New Size Change
prairielearn/executor:ea73c9f722fe96768a18e41a7051e9f2ce097a0e null 1178.81 MB 1178.88 MB 0.01%
prairielearn/prairielearn:ea73c9f722fe96768a18e41a7051e9f2ce097a0e linux/amd64 1178.80 MB 1178.88 MB 0.01%

csrfToken,
popoverId,
}: {
field: 'points' | 'auto_points' | 'manual_points' | 'score_perc';
Copy link
Member Author

Choose a reason for hiding this comment

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

This now combines the points and score forms, as they were very similar to each other, with the only difference being the suffix of the input element and the __action of the form (which is also combined, see below).

@@ -119,23 +122,6 @@ router.post(
} else {
res.send({});
}
} else if (req.body.__action === 'edit_question_score_perc') {
Copy link
Member Author

Choose a reason for hiding this comment

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

This action was removed and replaced with an update to the edit_question_points action that performs either the update of points or score percentage. The code was almost identical anyway. Same thing in the assessment instance page.

buttonId,
}: {
field: 'points' | 'auto_points' | 'manual_points' | 'score_perc';
instance_question: Omit<InstanceQuestion, 'modified_at'> & { modified_at: string };
Copy link
Member Author

Choose a reason for hiding this comment

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

modified_at is a string because it needs to have the same precision as the PG code that retrieves it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Checking my understanding: this is because Postgres dates have microsecond precision but JavaScript dates only have millisecond precision? Presumably we could clean this up by moving this check from SQL into JS?

$check_modified_at::TIMESTAMPTZ IS NOT NULL
AND $check_modified_at != iq.modified_at AS modified_at_conflict

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed. That would for sure be a valid alternative, but I'd prefer to do that in a separate PR.

@jonatanschroeder
Copy link
Member Author

Given that this PR will create a merging conflict with #9887 and it's easier to fix that conflict here than there, this will remain as a draft until that PR is merged.

@jonatanschroeder jonatanschroeder marked this pull request as ready for review June 21, 2024 18:31
Copy link
Contributor

@nwalters512 nwalters512 left a comment

Choose a reason for hiding this comment

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

I did my best to review this but there's a lot here and my eyes glazed over after a while. As always, please let me know if there's anything specific you want me to look at or if you need help manually testing any of these changes.

buttonId,
}: {
field: 'points' | 'auto_points' | 'manual_points' | 'score_perc';
instance_question: Omit<InstanceQuestion, 'modified_at'> & { modified_at: string };
Copy link
Contributor

Choose a reason for hiding this comment

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

Checking my understanding: this is because Postgres dates have microsecond precision but JavaScript dates only have millisecond precision? Presumably we could clean this up by moving this check from SQL into JS?

$check_modified_at::TIMESTAMPTZ IS NOT NULL
AND $check_modified_at != iq.modified_at AS modified_at_conflict

@jonatanschroeder jonatanschroeder added this pull request to the merge queue Jul 18, 2024
Merged via the queue into master with commit 4e7fdc2 Jul 18, 2024
9 checks passed
@jonatanschroeder jonatanschroeder deleted the mg-assessment-question-asset branch July 18, 2024 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants