Fix JSON payload size when publishing exam results, and misc fixes #1341
Conversation
WalkthroughThis pull request introduces several new components and functionalities related to a "Partner" block in a WordPress block editor context. Key additions include the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
shared-module
course-material
headless-lms
cms
main-frontend
labels
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (7)
services/main-frontend/src/pages/manage/exams/[id]/questions.tsx (2)
189-194: Use.forEachinstead of.mapfor side effectsIn the
totalPublishedfunction,.mapis used for side effects without utilizing the returned array. This is not the intended use of.mapand can lead to confusion. Use.forEachwhen you are not using the result array.Apply this diff to use
.forEachinstead:- submissions.map((sub) => { + submissions.forEach((sub) => {
Line range hint
83-84: Separate statements inonSuccesshandler for clarityThe
onSuccesshandler uses the comma operator to separate statements, which can reduce readability and may lead to unintended behavior.Consider separating the statements explicitly:
- getAllSubmissions.refetch(), checkPublishable() + getAllSubmissions.refetch() + checkPublishable()services/cms/src/blocks/Partners/index.tsx (1)
9-16: Inconsistent naming: Consider using plural form for consistencyThe block is titled "Partner Section" while related files and components use "Partners" (plural). For consistency and clarity, consider renaming the block title to "Partners Section" to match the naming convention used elsewhere.
Apply this diff:
- title: "Partner Section", + title: "Partners Section",shared-module/packages/common/src/components/Partner.tsx (2)
78-81: Strengthen type safety for Logo interfaceConsider making the Logo interface more strict by requiring specific attribute types.
Apply this diff:
interface Logo { clientId: string attributes: { - url: string - alt: string + url: string & { __brand: 'ImageURL' } + alt: string & { __brand: 'ImageAlt' } } }
Line range hint
36-48: Add ARIA attributes for better accessibilityThe PartnerBox should have appropriate ARIA attributes for better screen reader support.
Apply this diff:
const PartnerBox = styled.div` display: flex; flex-wrap: wrap; flex-direction: row; justify-content: center; align-items: center; align-content: center; gap: 10px; + role: "list"; + aria-label: "Partner logos"; img { display: flex; align-self: center; } `services/main-frontend/src/services/backend/exams.ts (1)
98-99: LGTM: Optimized payload size for grade releaseExcellent optimization! The function now sends only the necessary IDs instead of full submission objects, which will significantly reduce the JSON payload size when releasing grades.
Consider adding JSDoc to document this optimization:
+/** + * Releases grades for an exam by sending only the teacher grading decision IDs + * instead of full submission objects to optimize payload size. + * @param examId - The ID of the exam + * @param teacherGradingDecisionIds - Array of teacher grading decision IDs to release + */ export const releaseGrades = async (examId: string, teacherGradingDecisionIds: string[]) => {services/headless-lms/models/src/teacher_grading_decisions.rs (1)
250-275: LGTM! Consider adding ORDER BY for consistent results.The implementation is correct and follows the established patterns. For consistency with other query functions in this file, consider adding an ORDER BY clause to ensure deterministic results.
SELECT id, user_exercise_state_id, created_at, updated_at, deleted_at, score_given, teacher_decision AS "teacher_decision: _", justification, hidden FROM teacher_grading_decisions WHERE id = ANY($1) AND deleted_at IS NULL +ORDER BY created_at DESC
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
services/cms/src/blocks/Partners/PartnersEditor.tsx(1 hunks)services/cms/src/blocks/Partners/PartnersSave.tsx(1 hunks)services/cms/src/blocks/Partners/index.tsx(1 hunks)services/cms/src/blocks/index.tsx(3 hunks)services/course-material/src/components/ContentRenderer/index.tsx(2 hunks)services/course-material/src/components/ContentRenderer/moocfi/PartnersBlock.tsx(1 hunks)services/headless-lms/models/.sqlx/query-2c8c3495c6e898d23d0a73cb3861d511b64e8a06fb5cb8382c9457d494f03d2d.json(1 hunks)services/headless-lms/models/.sqlx/query-64749812d6f37a24c02dd98db9bdde37d63c7136d4248f9837c095f27adc8bfd.json(1 hunks)services/headless-lms/models/src/teacher_grading_decisions.rs(1 hunks)services/headless-lms/models/src/user_exercise_states.rs(1 hunks)services/headless-lms/server/src/controllers/main_frontend/exams.rs(2 hunks)services/main-frontend/src/pages/join.tsx(2 hunks)services/main-frontend/src/pages/manage/exams/[id]/questions.tsx(4 hunks)services/main-frontend/src/services/backend/exams.ts(1 hunks)shared-module/packages/common/src/components/Partner.tsx(4 hunks)
🔇 Additional comments (12)
services/cms/src/blocks/Partners/PartnersSave.tsx (1)
1-11: Component PartnerSave is properly implemented
The PartnerSave component correctly uses InnerBlocks.Content to render any child blocks, ensuring that nested content is saved and rendered properly.
services/cms/src/blocks/Partners/PartnersEditor.tsx (2)
13-46: LGTM! Well-structured component with proper styling and translations
The component follows React best practices, properly handles translations, and uses emotion for styling. The BlockWrapper integration ensures proper WordPress block editor functionality.
11-11: Consider allowing additional block types
The current implementation only allows 'core/image' blocks. Consider if other block types might be useful for partners' content, such as links or descriptions.
Let's check what block types are commonly used in similar contexts:
✅ Verification successful
Based on the search results, I can now generate the final response as we have sufficient context about block type restrictions across the codebase:
The restriction to only 'core/image' blocks is appropriate
Looking at similar components in the codebase, the Partners block follows a pattern where blocks with specific visual purposes have intentionally limited block types. For example:
AsideWithImagealso only allows "core/image"TableBoxonly allows "core/table"LogoLinkallows "core/image" and "core/columns"
The Partners block appears to be a visual component for displaying partner logos, similar to LogoLink and AsideWithImage. The restriction to only image blocks maintains the component's focused purpose of displaying partner logos in a consistent manner.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other block type restrictions in the codebase
rg "ALLOWED.*BLOCKS.*=.*\[.*\]" -A 2
Length of output: 9830
services/headless-lms/models/.sqlx/query-2c8c3495c6e898d23d0a73cb3861d511b64e8a06fb5cb8382c9457d494f03d2d.json (2)
41-44: Verify completeness of teacher_decision_type enum
Please verify that all possible decision types are covered in the enum.
Let's check for other occurrences of these decision types:
✅ Verification successful
Enum values for teacher_decision_type are complete and consistent
Based on the search results, the enum values in the SQL type definition are complete and consistent across the codebase:
- The enum values are defined in the migration file (
20220721192205_teacher_grading_decision.up.sql) - These values are used consistently in the Rust models (
teacher_grading_decisions.rs) - The frontend components (
AnswersRequiringAttentionItem.tsx) and translations (in multiple languages) align with these decision types - No other decision types were found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other uses of these decision types
rg -A 2 "teacher_decision.*type|full-points|zero-points|custom-points|suspected-plagiarism"
Length of output: 13521
2-3: Consider adding an index for performance
The query filters on id and deleted_at. Consider adding a composite index if one doesn't exist already.
Let's check the existing indexes:
services/main-frontend/src/pages/join.tsx (2)
13-13: LGTM: Enhanced component security and error handling
Good addition of authentication check and error boundary wrapper to protect the join course functionality.
Also applies to: 16-16
88-88: LGTM: Proper implementation of HOC composition
The order of HOC composition is correct - withErrorBoundary wraps withSignedIn, which wraps JoinCoursePage.
services/headless-lms/models/.sqlx/query-64749812d6f37a24c02dd98db9bdde37d63c7136d4248f9837c095f27adc8bfd.json (2)
2-3: LGTM: Efficient batch fetching query design
The query efficiently fetches multiple user exercise states using PostgreSQL's ANY operator with an array parameter, which is ideal for reducing payload size when dealing with multiple records.
51-93: Verify the completeness of enum values
The custom enum types (grading_progress, activity_progress, reviewing_stage) are crucial for exam results. Please verify that all possible states are covered.
✅ Verification successful
Let me gather more information about the enum definitions to ensure completeness.
Let me search for the Rust enum definitions to verify the completeness.
Let me search for one more piece of information to complete the verification.
Let me try one final search to get the complete ReviewingStage enum definition.
Based on all the gathered information, I can now generate the final response:
✓ Enum values are complete and consistent
The enum values in the SQL type definitions match their corresponding Rust implementations:
grading_progress: fully-graded, pending, pending-manual, failed, not-readyactivity_progress: initialized, started, in-progress, submitted, completedreviewing_stage: not_started, peer_review, self_review, waiting_for_peer_reviews, waiting_for_manual_grading, reviewed_and_locked
The values are used consistently across the codebase, as evidenced by the SQL migrations and Rust implementations. The enums cover all necessary states for exam results tracking.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any enum value references in the codebase that might not be covered
# Search for grading progress references
echo "Checking grading progress values:"
rg -i "grading.*progress.*=.*['\"](\w+)['\"]" --type rust
# Search for activity progress references
echo "Checking activity progress values:"
rg -i "activity.*progress.*=.*['\"](\w+)['\"]" --type rust
# Search for reviewing stage references
echo "Checking reviewing stage values:"
rg -i "reviewing.*stage.*=.*['\"](\w+)['\"]" --type rust
Length of output: 1249
Script:
#!/bin/bash
# Search for enum definitions in Rust files
echo "Searching for enum definitions:"
ast-grep --pattern 'enum $name {
$$$
}'
# Search for SQL enum type definitions
echo "Searching for SQL enum types:"
fd -e sql . -x cat {}
Length of output: 72644
Script:
#!/bin/bash
# Search for enum definitions in Rust files
echo "Searching for enum definitions in user_exercise_states.rs:"
rg -A 10 "^pub enum (GradingProgress|ActivityProgress|ReviewingStage)" services/headless-lms/models/src/user_exercise_states.rs
# Search for SQL enum type definitions
echo "Searching for SQL enum type definitions:"
rg -A 10 "CREATE TYPE (grading_progress|activity_progress|reviewing_stage) AS ENUM" --type sql
Length of output: 3863
Script:
#!/bin/bash
# Search for the rest of the ReviewingStage enum definition
echo "Searching for complete ReviewingStage enum:"
rg -A 15 "pub enum ReviewingStage" services/headless-lms/models/src/user_exercise_states.rs
# Search for reviewing_stage enum type definition in SQL
echo "Searching for reviewing_stage enum type definition:"
rg -B 5 -A 10 "CREATE TYPE reviewing_stage AS ENUM" --type sql
Length of output: 1454
Script:
#!/bin/bash
# Search for the complete ReviewingStage enum with more context
echo "Searching for complete ReviewingStage enum with more context:"
rg -A 20 "pub enum ReviewingStage" services/headless-lms/models/src/user_exercise_states.rs
# Also search for reviewing_stage in the SQL file
echo "Searching for reviewing_stage in SQL file:"
rg -A 10 "reviewing_stage" services/headless-lms/migrations/20210609111555_add_user_exercise_states.up.sql
Length of output: 1847
services/cms/src/blocks/index.tsx (1)
40-40: LGTM! Block registration looks good.
The PartnersBlock is correctly imported and registered in both block type maps following the established pattern.
Also applies to: 94-94, 117-117
services/course-material/src/components/ContentRenderer/index.tsx (1)
67-67: LGTM! Block renderer registration looks good.
The PartnersBlock is correctly imported and registered in the block renderer map following the established pattern and maintaining alphabetical order.
Also applies to: 160-160
services/headless-lms/models/src/user_exercise_states.rs (1)
681-681: LGTM! The GROUP BY clause addition is correct.
The addition of GROUP BY user_id ensures that points are correctly aggregated per user, maintaining data consistency.
Also applies to: 682-699
| let teacher_grading_decisions = | ||
| models::teacher_grading_decisions::get_by_ids(&mut conn, &teacher_grading_decision_ids) | ||
| .await?; | ||
| teacher_grading_decisions::update_teacher_grading_decision_hidden_field( | ||
| &mut conn, | ||
| submission.teacher_grading_decision.unwrap().id, | ||
| false, | ||
| ) | ||
|
|
||
| let user_exercise_state_mapping = models::user_exercise_states::get_by_ids( | ||
| &mut conn, | ||
| &teacher_grading_decisions | ||
| .iter() | ||
| .map(|x| x.user_exercise_state_id) | ||
| .collect::<Vec<Uuid>>(), | ||
| ) | ||
| .await? | ||
| .into_iter() | ||
| .map(|x| (x.id, x)) | ||
| .collect::<HashMap<Uuid, UserExerciseState>>(); | ||
|
|
||
| let mut tx = conn.begin().await?; | ||
| for teacher_grading_decision in teacher_grading_decisions.iter() { | ||
| let user_exercise_state = user_exercise_state_mapping | ||
| .get(&teacher_grading_decision.user_exercise_state_id) | ||
| .ok_or_else(|| { | ||
| ControllerError::new( | ||
| ControllerErrorType::InternalServerError, | ||
| "User exercise state not found for a teacher grading decision", | ||
| None, | ||
| ) | ||
| })?; | ||
|
|
||
| teacher_grading_decisions::update_teacher_grading_decision_hidden_field( | ||
| &mut tx, | ||
| teacher_grading_decision.id, | ||
| false, | ||
| ) |
There was a problem hiding this comment.
Validate that teacher grading decisions belong to the specified exam
Currently, the code retrieves teacher_grading_decisions based on the provided IDs but does not verify that these decisions are associated with the specified exam_id. This could lead to unintended behavior or security issues if incorrect or malicious IDs are provided. Consider adding a validation step to ensure all teacher_grading_decisions belong to the specified exam.
Apply this diff to add the validation:
+ // Validate that all teacher grading decisions belong to the specified exam
+ for decision in &teacher_grading_decisions {
+ if decision.exam_id != *exam_id {
+ return Err(ControllerError::new(
+ ControllerErrorType::BadRequest,
+ "Teacher grading decision does not belong to the specified exam.",
+ None,
+ ));
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
| const totalAnswered = useCallback( | ||
| (id: string) => { | ||
| const submissions = allSubmissionsList?.[id] | ||
| if (submissions) { | ||
| return submissions.length | ||
| } else { | ||
| return "0" | ||
| } | ||
| }, | ||
| [allSubmissionsList], | ||
| ) |
There was a problem hiding this comment.
Ensure consistent return types in totalAnswered function
The totalAnswered function returns a number when submissions exist but returns a string "0" when there are no submissions. This inconsistency can lead to type-related issues or unexpected behavior.
Apply this diff to return a consistent number:
- return "0"
+ return 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const totalAnswered = useCallback( | |
| (id: string) => { | |
| const submissions = allSubmissionsList?.[id] | |
| if (submissions) { | |
| return submissions.length | |
| } else { | |
| return "0" | |
| } | |
| }, | |
| [allSubmissionsList], | |
| ) | |
| const totalAnswered = useCallback( | |
| (id: string) => { | |
| const submissions = allSubmissionsList?.[id] | |
| if (submissions) { | |
| return submissions.length | |
| } else { | |
| return 0 | |
| } | |
| }, | |
| [allSubmissionsList], | |
| ) |
| const totalPublished = useCallback( | ||
| (id: string) => { | ||
| if (!getExam.data?.grade_manually) { | ||
| return <div>0</div> | ||
| } | ||
| const submissions = allSubmissionsList?.[id] | ||
| let count = 0 | ||
| if (submissions) { | ||
| submissions.map((sub) => { | ||
| if (sub.teacher_grading_decision?.hidden === true) { | ||
| count = count + 1 | ||
| } | ||
| }) | ||
| } | ||
| return count | ||
| }, | ||
| [allSubmissionsList, getExam.data?.grade_manually], | ||
| ) |
There was a problem hiding this comment.
Ensure consistent return types in totalPublished function
The totalPublished function returns a JSX element <div> when grade_manually is false and a number otherwise. This inconsistency can cause issues when the returned value is used elsewhere expecting a consistent type.
Consider returning a number consistently and handle the rendering in the component where needed.
Apply this diff:
- return <div>0</div>
+ return 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const totalPublished = useCallback( | |
| (id: string) => { | |
| if (!getExam.data?.grade_manually) { | |
| return <div>0</div> | |
| } | |
| const submissions = allSubmissionsList?.[id] | |
| let count = 0 | |
| if (submissions) { | |
| submissions.map((sub) => { | |
| if (sub.teacher_grading_decision?.hidden === true) { | |
| count = count + 1 | |
| } | |
| }) | |
| } | |
| return count | |
| }, | |
| [allSubmissionsList, getExam.data?.grade_manually], | |
| ) | |
| const totalPublished = useCallback( | |
| (id: string) => { | |
| if (!getExam.data?.grade_manually) { | |
| return 0 | |
| } | |
| const submissions = allSubmissionsList?.[id] | |
| let count = 0 | |
| if (submissions) { | |
| submissions.map((sub) => { | |
| if (sub.teacher_grading_decision?.hidden === true) { | |
| count = count + 1 | |
| } | |
| }) | |
| } | |
| return count | |
| }, | |
| [allSubmissionsList, getExam.data?.grade_manually], | |
| ) |
| const totalGraded = useCallback( | ||
| (id: string) => { | ||
| if (!getExam.data?.grade_manually) { | ||
| return <div>{totalAnswered(id)}</div> | ||
| } | ||
| const submissions = allSubmissionsList?.[id] | ||
| if (submissions) { | ||
| return submissions.filter((sub) => sub.teacher_grading_decision).length | ||
| } else { | ||
| return "0" | ||
| } | ||
| }, | ||
| [getExam.data?.grade_manually, allSubmissionsList, totalAnswered], | ||
| ) |
There was a problem hiding this comment.
Ensure consistent return types in totalGraded function
The totalGraded function returns a number in some cases and a JSX element <div> in others. This inconsistency can cause rendering issues or unexpected behavior in components that use this function.
Consider returning a consistent type (preferably a number) and handle the rendering separately in the JSX.
Apply this diff:
- return <div>{totalAnswered(id)}</div>
+ return totalAnswered(id)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const totalGraded = useCallback( | |
| (id: string) => { | |
| if (!getExam.data?.grade_manually) { | |
| return <div>{totalAnswered(id)}</div> | |
| } | |
| const submissions = allSubmissionsList?.[id] | |
| if (submissions) { | |
| return submissions.filter((sub) => sub.teacher_grading_decision).length | |
| } else { | |
| return "0" | |
| } | |
| }, | |
| [getExam.data?.grade_manually, allSubmissionsList, totalAnswered], | |
| ) | |
| const totalGraded = useCallback( | |
| (id: string) => { | |
| if (!getExam.data?.grade_manually) { | |
| return totalAnswered(id) | |
| } | |
| const submissions = allSubmissionsList?.[id] | |
| if (submissions) { | |
| return submissions.filter((sub) => sub.teacher_grading_decision).length | |
| } else { | |
| return "0" | |
| } | |
| }, | |
| [getExam.data?.grade_manually, allSubmissionsList, totalAnswered], | |
| ) |
| const gradedCheck = useCallback( | ||
| (id: string) => { | ||
| if (!getExam.data?.grade_manually) { | ||
| return ( | ||
| <div | ||
| className={css` | ||
| color: #32bea6; | ||
| `} | ||
| > | ||
| {t("status-graded")} | ||
| </div> | ||
| ) | ||
| } else if (countGraded === 0) { | ||
| return ( | ||
| <div | ||
| className={css` | ||
| color: #f76d82; | ||
| `} | ||
| > | ||
| {t("status-ungraded")} | ||
| </div> | ||
| ) | ||
| } else if (submissions.length > countGraded) { | ||
| return ( | ||
| <div | ||
| className={css` | ||
| color: #ffce54; | ||
| `} | ||
| > | ||
| {t("status-in-progress")} | ||
| {t("label-graded-automatically")} | ||
| </div> | ||
| ) | ||
| } | ||
| } else { | ||
| return " " | ||
| } | ||
| } | ||
|
|
||
| const totalAnswered = (id: string) => { | ||
| const submissions = allSubmissionsList?.[id] | ||
| if (submissions) { | ||
| return submissions.length | ||
| } else { | ||
| return "0" | ||
| } | ||
| } | ||
| const submissions = allSubmissionsList?.[id] | ||
| if (submissions) { | ||
| const countGraded = submissions.filter((sub) => sub.teacher_grading_decision).length | ||
| if (submissions.length === countGraded) { | ||
| return ( | ||
| <div | ||
| className={css` | ||
| color: #32bea6; | ||
| `} | ||
| > | ||
| {t("status-graded")} | ||
| </div> | ||
| ) | ||
| } else if (countGraded === 0) { | ||
| return ( | ||
| <div | ||
| className={css` | ||
| color: #f76d82; | ||
| `} | ||
| > | ||
| {t("status-ungraded")} | ||
| </div> | ||
| ) | ||
| } else if (submissions.length > countGraded) { | ||
| return ( | ||
| <div | ||
| className={css` | ||
| color: #ffce54; | ||
| `} | ||
| > | ||
| {t("status-in-progress")} | ||
| </div> | ||
| ) | ||
| } | ||
| } else { | ||
| return " " | ||
| } | ||
| }, | ||
| [getExam.data, allSubmissionsList, t], | ||
| ) |
There was a problem hiding this comment.
Ensure consistent return types in gradedCheck function
The gradedCheck function returns JSX elements in most cases but returns a string " " when no submissions are found. This inconsistency can cause rendering issues.
Consider returning a JSX element consistently, such as an empty <div> or <span>.
Apply this diff:
- return " "
+ return <span> </span>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const gradedCheck = useCallback( | |
| (id: string) => { | |
| if (!getExam.data?.grade_manually) { | |
| return ( | |
| <div | |
| className={css` | |
| color: #32bea6; | |
| `} | |
| > | |
| {t("status-graded")} | |
| </div> | |
| ) | |
| } else if (countGraded === 0) { | |
| return ( | |
| <div | |
| className={css` | |
| color: #f76d82; | |
| `} | |
| > | |
| {t("status-ungraded")} | |
| </div> | |
| ) | |
| } else if (submissions.length > countGraded) { | |
| return ( | |
| <div | |
| className={css` | |
| color: #ffce54; | |
| `} | |
| > | |
| {t("status-in-progress")} | |
| {t("label-graded-automatically")} | |
| </div> | |
| ) | |
| } | |
| } else { | |
| return " " | |
| } | |
| } | |
| const totalAnswered = (id: string) => { | |
| const submissions = allSubmissionsList?.[id] | |
| if (submissions) { | |
| return submissions.length | |
| } else { | |
| return "0" | |
| } | |
| } | |
| const submissions = allSubmissionsList?.[id] | |
| if (submissions) { | |
| const countGraded = submissions.filter((sub) => sub.teacher_grading_decision).length | |
| if (submissions.length === countGraded) { | |
| return ( | |
| <div | |
| className={css` | |
| color: #32bea6; | |
| `} | |
| > | |
| {t("status-graded")} | |
| </div> | |
| ) | |
| } else if (countGraded === 0) { | |
| return ( | |
| <div | |
| className={css` | |
| color: #f76d82; | |
| `} | |
| > | |
| {t("status-ungraded")} | |
| </div> | |
| ) | |
| } else if (submissions.length > countGraded) { | |
| return ( | |
| <div | |
| className={css` | |
| color: #ffce54; | |
| `} | |
| > | |
| {t("status-in-progress")} | |
| </div> | |
| ) | |
| } | |
| } else { | |
| return " " | |
| } | |
| }, | |
| [getExam.data, allSubmissionsList, t], | |
| ) | |
| const gradedCheck = useCallback( | |
| (id: string) => { | |
| if (!getExam.data?.grade_manually) { | |
| return ( | |
| <div | |
| className={css` | |
| color: #32bea6; | |
| `} | |
| > | |
| {t("label-graded-automatically")} | |
| </div> | |
| ) | |
| } | |
| const submissions = allSubmissionsList?.[id] | |
| if (submissions) { | |
| const countGraded = submissions.filter((sub) => sub.teacher_grading_decision).length | |
| if (submissions.length === countGraded) { | |
| return ( | |
| <div | |
| className={css` | |
| color: #32bea6; | |
| `} | |
| > | |
| {t("status-graded")} | |
| </div> | |
| ) | |
| } else if (countGraded === 0) { | |
| return ( | |
| <div | |
| className={css` | |
| color: #f76d82; | |
| `} | |
| > | |
| {t("status-ungraded")} | |
| </div> | |
| ) | |
| } else if (submissions.length > countGraded) { | |
| return ( | |
| <div | |
| className={css` | |
| color: #ffce54; | |
| `} | |
| > | |
| {t("status-in-progress")} | |
| </div> | |
| ) | |
| } | |
| } else { | |
| return <span> </span> | |
| } | |
| }, | |
| [getExam.data, allSubmissionsList, t], | |
| ) |
| props, | ||
| ) => { | ||
| return ( | ||
| <BreakFromCentered sidebar={false}> | ||
| {/* eslint-disable-next-line @typescript-eslint/no-explicit-any */} | ||
| <Sponsor logos={props.data.innerBlocks as any} /> | ||
| <Partner logos={props.data.innerBlocks as any} /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using any type assertion to maintain type safety
Casting props.data.innerBlocks to any defeats the benefits of TypeScript's type checking. It can lead to runtime errors that TypeScript is designed to prevent. Define the appropriate type for props.data.innerBlocks to maintain type safety.
For example:
- <Partner logos={props.data.innerBlocks as any} />
+ <Partner logos={props.data.innerBlocks as LogoProps[]} />Ensure that LogoProps is correctly defined and imported, and that innerBlocks conforms to this type.
Committable suggestion skipped: line range outside the PR's diff.
| <PartnerLogo width={width} key={clientId}> | ||
| <img src={attributes.url} alt={attributes.alt} /> | ||
| </SponsorLogo> | ||
| </PartnerLogo> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add loading and error handling for images
Consider adding loading="lazy" for better performance and fallback handling for failed image loads.
Apply this diff:
- <img src={attributes.url} alt={attributes.alt} />
+ <img
+ src={attributes.url}
+ alt={attributes.alt}
+ loading="lazy"
+ onError={(e) => {
+ e.currentTarget.style.display = 'none';
+ }}
+ />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <PartnerLogo width={width} key={clientId}> | |
| <img src={attributes.url} alt={attributes.alt} /> | |
| </SponsorLogo> | |
| </PartnerLogo> | |
| <PartnerLogo width={width} key={clientId}> | |
| <img | |
| src={attributes.url} | |
| alt={attributes.alt} | |
| loading="lazy" | |
| onError={(e) => { | |
| e.currentTarget.style.display = 'none'; | |
| }} | |
| /> | |
| </PartnerLogo> |
| pub async fn get_by_ids( | ||
| conn: &mut PgConnection, | ||
| ids: &[Uuid], | ||
| ) -> ModelResult<Vec<UserExerciseState>> { | ||
| let res = sqlx::query_as!( | ||
| UserExerciseState, | ||
| r#" | ||
| SELECT id, | ||
| user_id, | ||
| exercise_id, | ||
| course_instance_id, | ||
| exam_id, | ||
| created_at, | ||
| updated_at, | ||
| deleted_at, | ||
| score_given, | ||
| grading_progress AS "grading_progress: _", | ||
| activity_progress AS "activity_progress: _", | ||
| reviewing_stage AS "reviewing_stage: _", | ||
| selected_exercise_slide_id | ||
| FROM user_exercise_states | ||
| WHERE id = ANY($1) | ||
| "#, | ||
| &ids | ||
| ) | ||
| .fetch_all(conn) | ||
| .await?; | ||
| Ok(res) | ||
| } |
There was a problem hiding this comment.
Add deleted_at check to prevent returning soft-deleted records.
The query should include a WHERE clause to filter out soft-deleted records, consistent with other functions in this file.
Apply this diff to add the check:
pub async fn get_by_ids(
conn: &mut PgConnection,
ids: &[Uuid],
) -> ModelResult<Vec<UserExerciseState>> {
let res = sqlx::query_as!(
UserExerciseState,
r#"
SELECT id,
user_id,
exercise_id,
course_instance_id,
exam_id,
created_at,
updated_at,
deleted_at,
score_given,
grading_progress AS "grading_progress: _",
activity_progress AS "activity_progress: _",
reviewing_stage AS "reviewing_stage: _",
selected_exercise_slide_id
FROM user_exercise_states
-WHERE id = ANY($1)
+WHERE id = ANY($1)
+ AND deleted_at IS NULL
"#,
&ids
)
.fetch_all(conn)
.await?;
Ok(res)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub async fn get_by_ids( | |
| conn: &mut PgConnection, | |
| ids: &[Uuid], | |
| ) -> ModelResult<Vec<UserExerciseState>> { | |
| let res = sqlx::query_as!( | |
| UserExerciseState, | |
| r#" | |
| SELECT id, | |
| user_id, | |
| exercise_id, | |
| course_instance_id, | |
| exam_id, | |
| created_at, | |
| updated_at, | |
| deleted_at, | |
| score_given, | |
| grading_progress AS "grading_progress: _", | |
| activity_progress AS "activity_progress: _", | |
| reviewing_stage AS "reviewing_stage: _", | |
| selected_exercise_slide_id | |
| FROM user_exercise_states | |
| WHERE id = ANY($1) | |
| "#, | |
| &ids | |
| ) | |
| .fetch_all(conn) | |
| .await?; | |
| Ok(res) | |
| } | |
| pub async fn get_by_ids( | |
| conn: &mut PgConnection, | |
| ids: &[Uuid], | |
| ) -> ModelResult<Vec<UserExerciseState>> { | |
| let res = sqlx::query_as!( | |
| UserExerciseState, | |
| r#" | |
| SELECT id, | |
| user_id, | |
| exercise_id, | |
| course_instance_id, | |
| exam_id, | |
| created_at, | |
| updated_at, | |
| deleted_at, | |
| score_given, | |
| grading_progress AS "grading_progress: _", | |
| activity_progress AS "activity_progress: _", | |
| reviewing_stage AS "reviewing_stage: _", | |
| selected_exercise_slide_id | |
| FROM user_exercise_states | |
| WHERE id = ANY($1) | |
| AND deleted_at IS NULL | |
| "#, | |
| &ids | |
| ) | |
| .fetch_all(conn) | |
| .await?; | |
| Ok(res) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
services/headless-lms/models/.sqlx/query-67a13986d2adbce227c6ebbb7402f7e18980990010b4b784f6a6257a76b40b56.json (1)
100-102: Consider adding a batch size limit.The query accepts an array of UUIDs without a size limit. Consider adding a LIMIT clause or implementing application-level batching to prevent potential performance issues with large arrays.
services/headless-lms/server/src/controllers/main_frontend/exams.rs (2)
289-296: Documentation needs to be more specific about the response.The function documentation should specify the expected response status codes and error conditions.
-POST `/api/v0/main-frontend/exam/:exam_id/release-grades` - Publishes grading results of an exam by updating user_exercise_states according to teacher_grading_decisons and changes teacher_grading_decisions hidden field to false. Takes teacher grading decision ids as input. +POST `/api/v0/main-frontend/exam/:exam_id/release-grades` - Publishes grading results of an exam by updating user_exercise_states according to teacher_grading_decisons and changes teacher_grading_decisions hidden field to false. + +Input: Array of teacher grading decision IDs +Returns: 200 OK on success +Errors: +- 400 Bad Request: If any teacher grading decision doesn't belong to the specified exam +- 500 Internal Server Error: If user exercise state is not found for a teacher grading decision
319-346: Batch update operations for better performance.Instead of updating records one by one in a loop, consider using batch operations for better performance.
let mut tx = conn.begin().await?; + // Validate all decisions belong to the exam first + for teacher_grading_decision in teacher_grading_decisions.iter() { + let user_exercise_state = user_exercise_state_mapping + .get(&teacher_grading_decision.user_exercise_state_id) + .ok_or_else(|| { + ControllerError::new( + ControllerErrorType::InternalServerError, + "User exercise state not found for a teacher grading decision", + None, + ) + })?; + + if user_exercise_state.exam_id != Some(*exam_id) { + return Err(ControllerError::new( + ControllerErrorType::BadRequest, + "Teacher grading decision does not belong to the specified exam.", + None, + )); + } + } + + // Batch update teacher grading decisions + teacher_grading_decisions::batch_update_hidden_field( + &mut tx, + &teacher_grading_decision_ids, + false, + ).await?; + + // Batch update user exercise states + user_exercise_state_updater::batch_update( + &mut tx, + &teacher_grading_decisions + .iter() + .map(|x| x.user_exercise_state_id) + .collect::<Vec<Uuid>>(), + ).await?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
services/headless-lms/models/.sqlx/query-67a13986d2adbce227c6ebbb7402f7e18980990010b4b784f6a6257a76b40b56.json(1 hunks)services/headless-lms/models/src/user_exercise_states.rs(1 hunks)services/headless-lms/server/src/controllers/main_frontend/exams.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services/headless-lms/models/src/user_exercise_states.rs
🔇 Additional comments (1)
services/headless-lms/server/src/controllers/main_frontend/exams.rs (1)
331-337: Validation matches previous review comment.
The code now includes the suggested validation to ensure teacher grading decisions belong to the specified exam.
| @@ -0,0 +1,120 @@ | |||
| { | |||
| "db_name": "PostgreSQL", | |||
| "query": "\nSELECT id,\n user_id,\n exercise_id,\n course_instance_id,\n exam_id,\n created_at,\n updated_at,\n deleted_at,\n score_given,\n grading_progress AS \"grading_progress: _\",\n activity_progress AS \"activity_progress: _\",\n reviewing_stage AS \"reviewing_stage: _\",\n selected_exercise_slide_id\nFROM user_exercise_states\nWHERE id = ANY($1)\nAND deleted_at IS NULL\n", | |||
There was a problem hiding this comment.
💡 Codebase verification
Consider adding an index on the id column for optimal query performance
The query filters by id = ANY($1) but the table's primary key is a composite key on (user_id, exercise_id, course_instance_id). While there are several indexes on the table:
- Composite index on (user_id, exercise_id)
- Composite index on (deleted_at, user_id)
- Composite index on (deleted_at, exercise_id, reviewing_stage)
- Single index on (course_instance_id)
None of these indexes optimally support the query's id = ANY($1) filter. The deleted_at IS NULL condition is covered by existing indexes, but for optimal performance, consider adding a dedicated index on the id column.
🔗 Analysis chain
Verify index usage for optimal query performance.
The query filters on id and deleted_at columns. To ensure optimal performance, verify that appropriate indexes exist.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for indexes on the user_exercise_states table
# Expected: Index on id (primary key) and potentially on deleted_at
# Search for CREATE INDEX statements
rg -A 2 'CREATE.*INDEX.*user_exercise_states'
# Search for table definition to verify primary key
rg -A 20 'CREATE TABLE.*user_exercise_states'
Length of output: 3274
| let user_exercise_state_mapping = models::user_exercise_states::get_by_ids( | ||
| &mut conn, | ||
| &teacher_grading_decisions | ||
| .iter() | ||
| .map(|x| x.user_exercise_state_id) | ||
| .collect::<Vec<Uuid>>(), | ||
| ) | ||
| .await? | ||
| .into_iter() | ||
| .map(|x| (x.id, x)) | ||
| .collect::<HashMap<Uuid, UserExerciseState>>(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling empty input gracefully.
The code should handle the case when an empty array of IDs is provided.
+ if teacher_grading_decision_ids.is_empty() {
+ return token.authorized_ok(web::Json(()));
+ }
+
let user_exercise_state_mapping = models::user_exercise_states::get_by_ids(Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
New Features
PartnerEditorandPartnerSavecomponents for enhanced editing and saving of partner-related content in the WordPress block editor.ContentRenderercapabilities to include the newPartnersBlock.JoinCoursePageto ensure only signed-in users can access the page and handle errors more effectively.Bug Fixes
release_gradesfunction to verify user exercise states and grading decisions.Refactor
GradingPagecomponent for better performance and clarity.