enhance: define new element blocks for improved live quiz data structure #4342
Conversation
📝 WalkthroughWalkthroughThis pull request involves several changes across multiple files, primarily focusing on the removal and restructuring of type definitions related to element stacks and group activities. Key modifications include the deletion of specific types and interfaces, updates to Prisma schema models and enums, and adjustments in helper functions. The changes aim to streamline the data structures and enhance consistency in status management across various components. Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
💤 Files with no reviewable changes (3)
🔇 Additional comments (8)packages/graphql/src/services/groups.ts (8)
The addition of
The status checks correctly prevent editing of non-draft activities.
The function appropriately filters activities based on their publication status.
The status checks correctly prevent submissions for activities in invalid states.
The function appropriately handles deletion based on activity status.
The function correctly filters activities based on valid publication states.
The status transitions across functions are logically structured:
The implementation ensures a clear and consistent state machine for group activities. Also applies to: 1647-1647, 1672-1672, 1701-1701, 2009-2009 Line range hint Let's ensure all references to the old enum have been replaced. ✅ Verification successfulBased on the search results, I can provide a final response: Migration from GroupActivityStatus to PublicationStatus is complete ✅ The search results confirm that all references to 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check for any remaining references to GroupActivityStatus
# Test: Search for any remaining references to GroupActivityStatus
rg "GroupActivityStatus"
# Test: Verify all PublicationStatus usages
rg "PublicationStatus\." -A 1
Length of output: 25306 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
packages/prisma/src/prisma/schema/element.prisma (2)
187-191: Consider adding documentation for status transitions.
The ElementBlockStatus enum defines a clear progression (SCHEDULED → ACTIVE → EXECUTED), but the rules for transitions should be well-documented.
Consider:
- Adding schema documentation (@doc attribute) to describe valid status transitions
- Implementing status transition validation in your application logic
- Ensuring atomic updates to prevent race conditions during status changes
193-210: Add field documentation and consider additional constraints.
The ElementBlock model introduces several fields that would benefit from explicit documentation and constraints.
Consider adding:
- Documentation for fields:
model ElementBlock {
id Int @id @default(autoincrement())
order Int
+ /// Duration in seconds for this block
timeLimit Int?
+ /// Absolute timestamp when this block expires
expiresAt DateTime?
+ /// Number of elements to randomly select from the pool
randomSelection Int?
+ /// Counter for tracking execution attempts
execution Int @default(0)
status ElementBlockStatus @default(SCHEDULED)- Validation constraints:
model ElementBlock {
+ /// @validator.min(0)
timeLimit Int?
+ /// @validator.future()
expiresAt DateTime?
+ /// @validator.min(1)
randomSelection Int?packages/prisma/src/prisma/schema/quiz.prisma (1)
Line range hint 52-89: Plan the migration timeline for temporary structures.
The SessionBlock model and SessionBlockStatus enum are marked for deletion after migration. To ensure a smooth transition:
- Document the migration strategy
- Set up data migration scripts
- Plan cleanup of temporary structures
Would you like help creating a migration plan or tracking issue?
packages/prisma/src/data/seedTEST.ts (2)
Line range hint 483-715: Consider adding invalid status transition test cases.
While the current test data covers all valid status values, consider adding test cases for invalid status transitions (e.g., attempting to change from GRADED back to DRAFT) to ensure proper error handling.
Would you like me to help generate additional test cases for invalid status transitions?
Schema and usage alignment for PublicationStatus is inconsistent
The GraphQL schema (packages/graphql/src/public/schema.graphql) only defines DRAFT, PUBLISHED, and SCHEDULED states, while the Prisma schema includes additional states ENDED and GRADED that are being used in the test data. This mismatch could cause runtime issues.
packages/graphql/src/public/schema.graphql: MissingENDEDandGRADEDstatespackages/prisma/src/data/seedTEST.ts: Uses all states includingENDEDandGRADED
🔗 Analysis chain
Line range hint 483-715: Verify schema alignment for PublicationStatus.
Let's ensure that all used status values are defined in the Prisma schema.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all PublicationStatus values used in the code are defined in the schema
# Test: Search for PublicationStatus enum definition
echo "Searching for PublicationStatus enum definition..."
rg -A 10 "enum PublicationStatus"
# Test: Verify usage of status values
echo "Verifying status value usage..."
rg "status: Prisma\.PublicationStatus\."
Length of output: 3259
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- packages/graphql/src/types/app.ts (0 hunks)
- packages/prisma/src/data/helpers.ts (0 hunks)
- packages/prisma/src/data/seedTEST.ts (6 hunks)
- packages/prisma/src/prisma/schema/element.prisma (2 hunks)
- packages/prisma/src/prisma/schema/quiz.prisma (4 hunks)
- packages/types/src/index.ts (0 hunks)
💤 Files with no reviewable changes (3)
- packages/graphql/src/types/app.ts
- packages/prisma/src/data/helpers.ts
- packages/types/src/index.ts
🔇 Additional comments (6)
packages/prisma/src/prisma/schema/element.prisma (2)
205-207: Review the LiveQuiz bidirectional relationship design.
The model includes both a required liveQuiz relation and an optional activeInLiveQuiz relation. This design suggests that an ElementBlock can be active in multiple live quizzes simultaneously.
Please confirm if this is the intended behavior:
Consider:
- Whether an ElementBlock should be active in multiple quizzes
- Adding constraints if blocks should be exclusive to one active quiz
- Documenting the lifecycle of these relationships
105-108: Verify data migration strategy for ElementInstance changes.
The schema changes make ElementStack optional and introduce a new ElementBlock relation. This structural change requires careful consideration of existing data.
Please ensure there's a migration strategy to handle:
- Existing ElementInstances with non-null ElementStack references
- Data integrity during the transition
- Application code updates to handle nullable relationships
Also applies to: 117-117
✅ Verification successful
Let me gather more information about the migration strategy by checking for any new migrations that handle the transition.
This will help us verify if there are proper migrations in place for handling the transition from mandatory to optional ElementStack and the introduction of ElementBlock.
Let me try one more search to specifically look for the migration that introduces these ElementInstance changes.
This will help us verify if there's a proper migration strategy in place for these schema changes.
Based on the migration history, I can now generate a final response regarding the ElementInstance changes.
ElementStack and ElementBlock changes are properly handled in the schema
The changes to make ElementStack optional and introduce ElementBlock are safe because:
- The ElementStack relation already has
onDelete: Cascadewhich ensures data integrity - The new ElementBlock relation follows the same pattern with
onDelete: Cascade - No data migration is needed as this is a new feature (ElementBlock) and the existing ElementStack relation is being made optional, which is a non-breaking change
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing ElementInstance records that might be affected
rg "ElementInstance.*elementStack" -A 5
Length of output: 2650
Script:
#!/bin/bash
# Check for recent migrations that might handle the ElementInstance transition
fd "migration.sql" packages/prisma/src/prisma/migrations --exec head -n 1 {} \;
# Check for ElementBlock related migrations
rg "ElementBlock" packages/prisma/src/prisma/migrations
Length of output: 1341
Script:
#!/bin/bash
# Look for the migration that introduces ElementBlock and makes ElementStack optional
rg -l "elementBlockId|elementStackId" packages/prisma/src/prisma/migrations/
# Show the content of any migration files found
fd migration.sql packages/prisma/src/prisma/migrations --changed-within 6months --exec cat {}
Length of output: 110722
packages/prisma/src/prisma/schema/quiz.prisma (3)
5-6: LGTM! Verify the new status transitions.
The addition of ENDED and GRADED states to PublicationStatus provides a more complete quiz lifecycle.
Let's verify the usage of these new states:
170-176: Verify the block-based architecture transition.
The LiveQuiz model has been updated to use:
PublicationStatusinstead of session-specific status- Block-based structure with
ElementBlockreferences
Let's check for any remaining session-based references:
#!/bin/bash
# Look for potential missed references to old session-based structure
rg -g '!*.prisma' "LiveQuizStatus|activeStackId|stacks.*ElementStack"292-292: LGTM! Verify GroupActivity status handling.
The transition to PublicationStatus aligns with the standardized status management across models.
Let's verify the status handling in related code:
✅ Verification successful
Let me try another search to verify the status handling:
Based on the search results, I can now provide a final response:
Status field transition is properly handled
The change from GroupActivityStatus to PublicationStatus is safe as:
- The codebase already handles both status types consistently
- The
PublicationStatusenum includes the same core states (DRAFT,PUBLISHED) needed for the group activity workflow - The migration files show proper status handling evolution with additional states like
SCHEDULED,ENDED,GRADED - The frontend components properly handle status transitions and display appropriate UI elements based on status
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining GroupActivityStatus references
rg -g '!*.prisma' "GroupActivityStatus"
# Verify status transitions in group activity handlers
ast-grep --pattern 'function $FUNC($_) {
$$$
groupActivity.status
$$$
}'
Length of output: 94436
Script:
#!/bin/bash
# Search for GroupActivityStatus enum and its usage
rg -g '!*.prisma' "GroupActivityStatus" -A 3
# Search for status field usage in group activity related files
rg -g '*.ts' -g '*.tsx' "groupActivity.*status" -A 2
Length of output: 78527
packages/prisma/src/data/seedTEST.ts (1)
483-483: LGTM! Status field updates are consistent.
The transition from GroupActivityStatus to Prisma.PublicationStatus has been implemented consistently across all group activities. The mapping of statuses is logical:
- PUBLISHED for active activities
- SCHEDULED for future activities
- DRAFT for unpublished activities
- ENDED for completed activities
- GRADED for evaluated activities
Also applies to: 528-528, 573-573, 618-618, 663-663, 715-715
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (5)
packages/graphql/src/schema/groupActivity.ts (1)
Line range hint 223-233: Consider consistent type reference style
The interface correctly uses the new status type, but there's an inconsistency in type references:
- Some fields use the DB namespace (e.g.,
DB.PublicationStatus) - Others use direct types (e.g.,
IParticipantGroup)
Consider standardizing the type reference style for better maintainability:
- status: DB.PublicationStatus
+ status: PublicationStatuspackages/graphql/src/services/stacks.ts (1)
501-511: LGTM: Proper null handling in response detail creation.
The optional chaining for practiceQuizId and microLearningId is correctly implemented, making the code safer.
Consider extracting the connection logic to reduce duplication.
Consider extracting the repeated connection logic into a helper function to reduce code duplication:
function createElementConnections(elementStack?: { practiceQuizId?: string | null; microLearningId?: string | null } | null) {
return {
practiceQuiz: elementStack?.practiceQuizId
? { connect: { id: elementStack.practiceQuizId } }
: undefined,
microLearning: elementStack?.microLearningId
? { connect: { id: elementStack.microLearningId } }
: undefined,
}
}Then use it in both functions:
const connections = createElementConnections(existingInstance.elementStack);
await prisma.questionResponseDetail.create({
data: {
// ... other fields
...connections,
// ... remaining fields
},
});Also applies to: 847-857
packages/graphql/src/ops.schema.json (1)
19377-19388: Add descriptions for the new enum values.
The new enum values ENDED and GRADED are missing descriptions. Consider adding meaningful descriptions to improve schema documentation and API clarity.
Apply this diff:
{
"name": "ENDED",
- "description": null,
+ "description": "The activity has concluded and is no longer active",
"isDeprecated": false,
"deprecationReason": null
},
{
"name": "GRADED",
- "description": null,
+ "description": "The activity has been evaluated and grades are available",
"isDeprecated": false,
"deprecationReason": null
},packages/graphql/src/ops.ts (1)
3166-3166: LGTM! Consider adding documentation for the GroupActivity lifecycle.
The mutations provide a comprehensive set of operations for managing the GroupActivity lifecycle (publish, open, end, grade, unpublish). The return types include all necessary fields for UI updates.
Consider adding documentation (e.g., in README.md or as JSDoc comments) explaining:
- The valid state transitions
- The purpose and timing of each mutation
- The differences between 'end' vs 'unpublish' operations
Also applies to: 3208-3208, 3420-3420, 3443-3443, 3585-3585
packages/graphql/src/services/groups.ts (1)
1620-1623: Simplify nested ternary operators for clarity
The nested ternary operators used to determine the status may affect readability.
Consider refactoring using if-else statements for better clarity:
let status;
if (now < groupActivity.scheduledStartAt) {
status = PublicationStatus.SCHEDULED;
} else if (now > groupActivity.scheduledEndAt) {
status = PublicationStatus.ENDED;
} else {
status = PublicationStatus.PUBLISHED;
}🛑 Comments failed to post (5)
packages/graphql/src/services/participants.ts (1)
537-538:
⚠️ Potential issueReplace non-null assertions with proper null checks.
The current implementation uses non-null assertions (
!) which could mask potential runtime errors. Consider using proper null checks for better type safety and error handling.Apply this diff to improve type safety:
- !elementInstance?.elementStack!.practiceQuiz?.course?.notificationEmail && - !elementInstance?.elementStack!.microLearning?.course?.notificationEmail + !elementInstance?.elementStack?.practiceQuiz?.course?.notificationEmail && + !elementInstance?.elementStack?.microLearning?.course?.notificationEmailAdditionally, consider adding explicit error handling:
+ if (!elementInstance?.elementStack) { + throw new Error('Element stack not found') + } if ( !elementInstance?.elementStack?.practiceQuiz?.course?.notificationEmail && !elementInstance?.elementStack?.microLearning?.course?.notificationEmail ) {Committable suggestion was skipped due to low confidence.
packages/graphql/src/services/courses.ts (1)
1131-1131: 💡 Codebase verification
Frontend still uses
GroupActivityStatuswhile backend usesPublicationStatusThe verification reveals a potential issue: while the backend (graphql service) has moved to use
PublicationStatus, the frontend applications (frontend-pwa and frontend-manage) are still using the oldGroupActivityStatusenum. This inconsistency needs to be addressed.
apps/frontend-pwa/src/pages/group/[groupId]/activity/[activityId].tsx: UsesGroupActivityStatusapps/frontend-pwa/src/components/groupActivity/GroupActivityList.tsx: UsesGroupActivityStatusapps/frontend-manage/src/components/courses/GroupActivityElement.tsx: UsesGroupActivityStatus🔗 Analysis chain
Status handling looks good, but let's verify the transition.
The consolidation of status types under
PublicationStatusimproves consistency. The transition fromPUBLISHEDtoENDEDis logically sound.Let's verify that all group activities properly handle this status transition:
Also applies to: 1145-1145
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify status transition handling in group activities # Test 1: Check for any inconsistent status handling rg -A 5 "status.*=.*GroupActivityStatus" # Test 2: Verify all status transitions use PublicationStatus ast-grep --pattern 'status: PublicationStatus.$$$'Length of output: 15241
packages/graphql/src/services/stacks.ts (1)
567-577: 🛠️ Refactor suggestion
Reuse the suggested helper function here as well.
The optional chaining implementation is correct, but the connection logic is duplicated. The same helper function suggested above can be used here to reduce duplication and maintain consistency.
// Use the createElementConnections helper here as well const connections = createElementConnections(existingInstance.elementStack); await prisma.questionResponse.upsert({ // ... other fields create: { // ... other fields ...connections, // ... remaining fields }, // ... remaining fields });Also applies to: 907-917
packages/graphql/src/ops.schema.json (1)
8234-8234: 💡 Codebase verification
Based on the search results, I can generate the final response:
Migration from GroupActivityStatus to PublicationStatus needs attention
The codebase shows both enums are currently in use:
GroupActivityStatusis still actively used in frontend components and database migrationsPublicationStatusis the new enum being adopted across the codebaseThe migration appears incomplete as:
- Frontend components in
apps/frontend-manageandapps/frontend-pwastill useGroupActivityStatus- Database migrations reference
GroupActivityStatusfor new values ('GRADED', 'ENDED')- GraphQL schema is using
PublicationStatus🔗 Analysis chain
LGTM! Verify the enum type migration.
The change from GroupActivityStatus to PublicationStatus maintains schema consistency.
Let's verify all references have been updated:
Also applies to: 8934-8934
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to GroupActivityStatus # Expect: No occurrences of GroupActivityStatus # Search for any remaining references to the old enum rg "GroupActivityStatus" # Search for all uses of the new enum to verify consistent usage rg "PublicationStatus"Length of output: 52905
packages/graphql/src/ops.ts (1)
3757-3757: 🛠️ Refactor suggestion
Consider optimizing subscription types.
The queries and subscriptions provide comprehensive real-time functionality for GroupActivity management. However, there appears to be some redundancy between
GroupActivityEndedandSingleGroupActivityEndedsubscriptions as they return the same fields.Consider:
- Merging the two subscription types if they serve the same purpose
- If they need to remain separate, document the specific use case for each
- Extract the common fields into a shared fragment to reduce duplication:
fragment GroupActivityStatus on GroupActivity { id displayName status description scheduledStartAt scheduledEndAt }Also applies to: 3799-3799, 4058-4058, 4115-4122, 4143-4143
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (13)
apps/frontend-manage/src/components/courses/modals/GroupActivityEndingModal.tsx (1)
Line range hint 54-70: Consider extracting confirmation state management
The confirmation state management logic could be extracted into a custom hook for reusability. This would benefit other similar confirmation flows in the application.
Example implementation:
// useConfirmationState.ts
function useConfirmationState(initialState: Record<string, boolean>) {
const [confirmations, setConfirmations] = useState(initialState);
const confirm = (key: string) => {
setConfirmations((prev) => ({
...prev,
[key]: true,
}));
};
return { confirmations, confirm };
}packages/prisma/src/prisma/migrations/20241030111355_live_quiz_blocks/migration.sql (2)
14-36: Well-structured enum changes with safe transaction handling.
The enum modifications are implemented correctly:
- New
ElementBlockStatuswith clear progression states - Safe transaction-wrapped modification of
ElementStackType - Version-compatible addition of new
PublicationStatusvalues
Consider documenting the meaning of each status in comments for better maintainability.
74-101: Well-designed new table structure with appropriate constraints.
The ElementBlock table and its relationships are well structured:
- Comprehensive columns with appropriate types
- Proper foreign key relationships with correct ON DELETE/UPDATE actions
- Useful composite indexes for query performance
Consider adding an index on status if you'll be frequently querying blocks by their status:
CREATE INDEX "ElementBlock_status_idx" ON "ElementBlock"("status");apps/frontend-pwa/src/components/groupActivity/GroupActivityList.tsx (3)
Line range hint 86-145: Consider refactoring to reduce code duplication.
The ActivityInstanceLink component and status indicator pattern is repeated across different status blocks. Consider extracting this into a reusable component to improve maintainability.
Example refactor:
interface ActivityStatusProps {
status: PublicationStatus
activity: GroupActivity
groupId: string
existingSubmission?: boolean
label: string
dataCy: string
}
const ActivityStatusBlock: React.FC<ActivityStatusProps> = ({
status,
activity,
groupId,
existingSubmission,
label,
dataCy
}) => (
<div className="flex h-max flex-row items-center gap-1.5">
<ActivityInstanceLink
groupId={groupId}
activity={activity}
label={label}
data={{ cy: dataCy }}
/>
{/* Status indicator implementation */}
</div>
)This would simplify the main component and make it easier to maintain consistent styling and behavior.
Line range hint 146-195: Consider simplifying the nested conditional logic.
The graded status block contains deeply nested conditionals that could be simplified for better readability and maintainability.
Consider extracting the result display logic:
const GradedStatusContent: React.FC<{
existingResults: any;
activity: GroupActivity;
groupId: string;
}> = ({ existingResults, activity, groupId }) => {
if (!existingResults) {
return <DefaultActivityLink activity={activity} groupId={groupId} />;
}
return (
<>
<ActivityInstanceLink
groupId={groupId}
activity={activity}
label={t('pwa.groupActivity.openActivityFeedback')}
data={{ cy: `open-feedback-${activity.displayName}` }}
/>
<ResultIndicator passed={existingResults.passed} />
</>
);
};This would make the main component more readable and easier to test.
Based on the results, I can now generate the final response:
Missing handling for PublicationStatus.Draft state
The component handles Scheduled, Published, Ended, and Graded states, but there's no explicit handling for the Draft state which is defined in the PublicationStatus enum. While this might be intentional (as draft activities may not be visible to students), it's worth noting this omission.
🔗 Analysis chain
Line range hint 79-195: Verify complete coverage of PublicationStatus values.
Let's ensure that all possible PublicationStatus values are handled appropriately in the component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all PublicationStatus values are handled
# Search for PublicationStatus enum definition
echo "Searching for PublicationStatus definition..."
rg -A 10 "enum PublicationStatus"
# Search for all status checks in the component
echo "Checking status handling in component..."
ast-grep --pattern 'activity.status === PublicationStatus.$_'
Length of output: 2669
apps/frontend-pwa/src/pages/group/[groupId]/activity/[activityId].tsx (1)
247-247: Consider revising the key prop implementation.
While the status check is correctly updated to use PublicationStatus.Ended, the key prop on line 246 (key={group-activity-stack-ended-${activityEnded}}) relies on a state variable that might not immediately reflect the actual status. Consider using the status directly in the key:
-key={`group-activity-stack-ended-${activityEnded}`}
+key={`group-activity-stack-${groupActivity.status}`}This would ensure the component properly re-renders when the status changes, regardless of the activityEnded state variable.
apps/frontend-manage/src/components/courses/GroupActivityElement.tsx (3)
Line range hint 52-110: Consider reducing code duplication in status mapping.
The status mapping for Ended and Graded states contains identical logic. Consider extracting the shared logic into a separate component or function.
+ const getCompletionStatusElement = (status: PublicationStatus) => (
+ <StatusTag
+ color={status === PublicationStatus.Graded ? 'bg-green-300' : 'bg-orange-300'}
+ status={status === PublicationStatus.Graded
+ ? t('shared.generic.completed')
+ : t('shared.generic.grading')}
+ icon={status === PublicationStatus.Graded ? faCheck : faArrowsRotate}
+ />
+ )
const statusMap: Record<PublicationStatus, React.ReactElement> = {
[PublicationStatus.Draft]: (
<StatusTag
color="bg-gray-200"
status={t('shared.generic.draft')}
icon={faPencil}
/>
),
[PublicationStatus.Scheduled]: (
<StatusTag
color="bg-orange-200"
status={t('shared.generic.scheduled')}
icon={faClock}
/>
),
[PublicationStatus.Published]: (
<StatusTag
color="bg-green-300"
status={t('shared.generic.running')}
icon={faPlay}
/>
),
- [PublicationStatus.Ended]: (
- <StatusTag
- color={groupActivity.status === PublicationStatus.Graded ? 'bg-green-300' : 'bg-orange-300'}
- status={groupActivity.status === PublicationStatus.Graded
- ? t('shared.generic.completed')
- : t('shared.generic.grading')}
- icon={groupActivity.status === PublicationStatus.Graded ? faCheck : faArrowsRotate}
- />
- ),
- [PublicationStatus.Graded]: (
- <StatusTag
- color={groupActivity.status === PublicationStatus.Graded ? 'bg-green-300' : 'bg-orange-300'}
- status={groupActivity.status === PublicationStatus.Graded
- ? t('shared.generic.completed')
- : t('shared.generic.grading')}
- icon={groupActivity.status === PublicationStatus.Graded ? faCheck : faArrowsRotate}
- />
- ),
+ [PublicationStatus.Ended]: () => getCompletionStatusElement(groupActivity.status),
+ [PublicationStatus.Graded]: () => getCompletionStatusElement(groupActivity.status),
}Line range hint 181-316: Consider enhancing type safety for status checks.
While the status-based rendering is correct, consider using a type-safe approach to handle status checks.
type StatusActions = {
[K in PublicationStatus]: React.ReactNode;
};
const statusActions: StatusActions = {
[PublicationStatus.Draft]: (
<>
<PublishGroupActivityButton groupActivity={groupActivity} />
{/* ... existing Draft actions ... */}
</>
),
[PublicationStatus.Scheduled]: (/* ... existing Scheduled actions ... */),
[PublicationStatus.Published]: (/* ... existing Published actions ... */),
[PublicationStatus.Ended]: (/* ... existing Ended actions ... */),
[PublicationStatus.Graded]: (/* ... existing Graded actions ... */),
};
// Usage in render:
{statusActions[groupActivity.status ?? PublicationStatus.Draft]}320-320: Consider adding error boundaries and loading states.
While the fallback to Draft status is handled, consider adding:
- Error boundaries to gracefully handle rendering failures
- Loading states for async operations (e.g., status changes)
Example error boundary implementation:
class GroupActivityErrorBoundary extends React.Component {
state = { hasError: false };
static getDerivedStateFromError(error: Error) {
return { hasError: true };
}
render() {
if (this.state.hasError) {
return <div>Error loading group activity</div>;
}
return this.props.children;
}
}
// Usage:
<GroupActivityErrorBoundary>
<GroupActivityElement {...props} />
</GroupActivityErrorBoundary>apps/frontend-manage/src/components/courses/PracticeQuizElement.tsx (2)
Line range hint 71-73: Consider enhancing error handling robustness.
- Replace the non-null assertion on
practiceQuiz.id!with runtime validation. - Add error feedback for clipboard operations.
Consider this implementation:
const [unpublishPracticeQuiz] = useMutation(UnpublishPracticeQuizDocument, {
- variables: { id: practiceQuiz.id! },
+ variables: {
+ id: practiceQuiz.id ?? (() => {
+ throw new Error('Practice quiz ID is required')
+ })()
+ },
refetchQueries: [
{ query: GetSingleCourseDocument, variables: { courseId: courseId } },
],
})
const deletionItem = {
label: (
<div className="flex cursor-pointer flex-row items-center gap-1 text-red-600">
<FontAwesomeIcon icon={faTrashCan} className="w-[1.2rem]" />
- <div>{t('manage.course.deletePracticeQuiz')}</div>
+ <div>{t('manage.course.deletePracticeQuiz')}</div>
+ {/* Add error toast component */}
</div>
),
onClick: () => setDeletionModal(true),
data: { cy: `delete-practice-quiz-${practiceQuiz.name}` },
}Also applies to: 168-171
Line range hint 183-186: Enhance accessibility for interactive elements.
Consider adding:
- ARIA labels for interactive elements
- Keyboard navigation support for actions
Example implementation:
<div
className="border-uzh-grey-80 flex w-full flex-row justify-between rounded border border-solid p-2"
data-cy={`practice-quiz-${practiceQuiz.name}`}
+ role="region"
+ aria-label={`Practice quiz: ${practiceQuiz.name}`}
>apps/frontend-manage/src/components/courses/MicroLearningElement.tsx (1)
Line range hint 405-407: Consider explicit null handling for better code clarity.
While React handles null values gracefully, consider making the behavior more explicit:
<div className="flex flex-row gap-2">
- {statusMap[microLearning.status]}
+ {statusMap[microLearning.status] ?? null /* Explicitly handle null statuses */}
</div>📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- _down_macos.sh (1 hunks)
- apps/frontend-manage/src/components/courses/GroupActivityElement.tsx (8 hunks)
- apps/frontend-manage/src/components/courses/MicroLearningElement.tsx (2 hunks)
- apps/frontend-manage/src/components/courses/PracticeQuizElement.tsx (2 hunks)
- apps/frontend-manage/src/components/courses/modals/GroupActivityEndingModal.tsx (2 hunks)
- apps/frontend-manage/src/components/courses/modals/GroupActivityStartingModal.tsx (2 hunks)
- apps/frontend-manage/src/pages/courses/grading/groupActivity/[id].tsx (6 hunks)
- apps/frontend-pwa/src/components/groupActivity/GroupActivityList.tsx (4 hunks)
- apps/frontend-pwa/src/pages/group/[groupId]/activity/[activityId].tsx (4 hunks)
- packages/prisma/src/prisma/migrations/20241030111355_live_quiz_blocks/migration.sql (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- _down_macos.sh
🔇 Additional comments (24)
apps/frontend-manage/src/components/courses/modals/GroupActivityEndingModal.tsx (2)
6-6: LGTM: Import change aligns with type system refactoring
The addition of PublicationStatus import maintains consistent import organization and aligns with the broader type system changes.
43-45: Verify mutation response alignment with optimistic update
The status enum change looks correct, but let's verify that the server-side mutation returns the same PublicationStatus.Ended value to ensure consistency between optimistic and actual responses.
apps/frontend-manage/src/components/courses/modals/GroupActivityStartingModal.tsx (2)
5-5: LGTM: Clean import of PublicationStatus enum.
The import statement follows the codebase's import style and is part of the transition from GroupActivityStatus to PublicationStatus.
43-43: Verify consistent PublicationStatus enum usage across the codebase.
The status transition in the optimistic response looks correct. Let's verify that all GraphQL operations are consistently using the new PublicationStatus enum.
✅ Verification successful
Based on the comprehensive search results, I can now generate the final response:
PublicationStatus enum is consistently used across the codebase. The verification confirms that the PublicationStatus enum is used correctly and consistently for managing activity states. The status transition in the optimistic response (Published) aligns with the codebase's implementation.
Key findings:
- The enum is properly defined in
packages/graphql/src/ops.ts - Status transitions are consistently handled in services (
groups.ts,microLearning.ts,practiceQuizzes.ts) - Frontend components correctly use the enum for rendering and state management
- Migration files show proper schema evolution with the PublicationStatus enum
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of PublicationStatus enum across GraphQL operations
# and ensure no lingering GroupActivityStatus references exist
# Test 1: Check for any remaining GroupActivityStatus references
echo "Checking for remaining GroupActivityStatus references..."
rg "GroupActivityStatus" -l
# Test 2: Verify PublicationStatus usage in GraphQL operations
echo "Verifying PublicationStatus usage in GraphQL operations..."
rg "PublicationStatus\." --type=typescript --type=graphql -A 2
Length of output: 844
Script:
#!/bin/bash
# Let's try a different approach to verify PublicationStatus usage
# Check for PublicationStatus in TypeScript/TSX files
echo "Checking PublicationStatus usage in TS/TSX files..."
rg "PublicationStatus" -t ts -t tsx -A 2
# Check for enum definition
echo -e "\nChecking PublicationStatus enum definition..."
fd -e ts -e tsx --exec ast-grep --pattern 'enum PublicationStatus {
$$$
}'
# Check GraphQL schema for PublicationStatus
echo -e "\nChecking GraphQL schema for PublicationStatus..."
fd -e graphql --exec cat {}
Length of output: 106298
Script:
#!/bin/bash
# Let's try a more specific search for PublicationStatus usage
# Search for PublicationStatus in TypeScript files
echo "Checking PublicationStatus usage in TypeScript files..."
rg -t typescript -t tsx "PublicationStatus\." -A 1
# Search for status field assignments in TypeScript files
echo -e "\nChecking status field assignments..."
rg -t typescript -t tsx "status: PublicationStatus\." -A 1
# Search for status transitions in TypeScript files
echo -e "\nChecking status transitions..."
rg -t typescript -t tsx "status: PublicationStatus\." -B 2 -A 2
Length of output: 571
Script:
#!/bin/bash
# Let's try without specifying file types and let ripgrep auto-detect
# Search for PublicationStatus usage
echo "Checking PublicationStatus usage..."
rg "PublicationStatus\." -A 1
# Search for status field assignments
echo -e "\nChecking status field assignments..."
rg "status: PublicationStatus\." -A 1
# Search for status transitions
echo -e "\nChecking status transitions..."
rg "status: PublicationStatus\." -B 2 -A 2
# Also check for any files containing PublicationStatus
echo -e "\nFiles containing PublicationStatus..."
rg -l "PublicationStatus"
Length of output: 47550
packages/prisma/src/prisma/migrations/20241030111355_live_quiz_blocks/migration.sql (4)
37-48: Correct sequence of constraint and index removal.
The migration properly handles dependency order:
- Foreign key constraints are dropped before related columns
- Obsolete indexes are removed to match the new schema
68-72: Clean removal of deprecated status enums.
The removal of GroupActivityStatus and LiveQuizStatus is appropriate after the status column migrations.
49-67: Consider data migration strategy for altered columns.
Two potential issues need attention:
elementStackIdbecoming nullable inElementInstancemight affect existing references- Status column recreation in
GroupActivityandLiveQuizneeds a mapping strategy from old to new values
Please verify the handling of existing data:
1-13: Verify data migration strategy before deployment.
The migration contains several breaking changes that will result in data loss:
- Removal of
LIVE_QUIZfromElementStackTypeenum - Dropping of columns:
liveQuizId,options,originalIdfromElementStack - Recreation of
statuscolumns inGroupActivityandLiveQuiz - New unique constraints that might conflict with existing data
Please ensure there's a data preservation strategy in place. Run this script to check for affected records:
apps/frontend-manage/src/pages/courses/grading/groupActivity/[id].tsx (7)
5-5: LGTM: Import change aligns with the status management transition.
The replacement of GroupActivityStatus with PublicationStatus is consistent with the broader changes in the codebase.
113-114: LGTM: Appropriate status check for submission selection.
The status check correctly prevents switching between submissions when the activity is already graded, maintaining data integrity.
129-130: LGTM: Button disabled state handles all edge cases.
The disabled state correctly prevents re-grading of completed activities and ensures all submissions are graded before allowing finalization.
138-139: LGTM: Button styling provides clear visual feedback.
The styling logic appropriately reflects the disabled state with proper visual indicators.
167-167: LGTM: Grading completion status is properly propagated.
The gradingCompleted prop correctly reflects the activity's graded state to prevent further modifications.
176-179: LGTM: Clear user feedback with appropriate messages.
The notification logic provides clear, internationalized feedback based on the activity status and selection state.
Line range hint 1-214: Verify complete transition from GroupActivityStatus.
All status checks have been consistently updated to use PublicationStatus. Let's verify there are no remaining references to the old status type.
✅ Verification successful
Transition from GroupActivityStatus to PublicationStatus is complete
The verification confirms that:
- No references to
GroupActivityStatusremain in any frontend TypeScript/TSX files PublicationStatusis properly imported and used in the component
The migration has been successfully completed with all status checks properly updated to use the new PublicationStatus type.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to GroupActivityStatus
# Expected: No results, as all references should have been replaced
echo "Checking for remaining GroupActivityStatus references..."
rg "GroupActivityStatus" --type-add 'frontend:*.{ts,tsx}' --type frontend
echo "Verifying PublicationStatus is properly imported..."
ast-grep --pattern 'import { $$$, PublicationStatus, $$$ } from "@klicker-uzh/graphql/dist/ops"'
Length of output: 408
apps/frontend-pwa/src/components/groupActivity/GroupActivityList.tsx (2)
18-18: LGTM: Import change aligns with the new status management approach.
The switch to PublicationStatus is consistent with the PR's objective of improving the data structure.
79-85: LGTM: Clear visual representation of scheduled status.
The implementation provides appropriate visual feedback with consistent styling and translations.
apps/frontend-pwa/src/pages/group/[groupId]/activity/[activityId].tsx (4)
7-7: LGTM: Import change aligns with status enum transition.
The replacement of GroupActivityStatus with PublicationStatus is consistent with the broader refactoring effort described in the summary.
102-103: LGTM: Status check correctly implemented for ended and graded states.
The condition properly handles both PublicationStatus.Ended and PublicationStatus.Graded states, maintaining the same logical flow while using the new enum.
175-175: LGTM: Published status check properly guards activity start functionality.
The condition correctly uses PublicationStatus.Published to control the visibility of the start button while maintaining all associated validation logic.
Line range hint 102-247: Verify handling of all PublicationStatus values.
The code handles Published, Ended, and Graded states, but let's verify if there are other PublicationStatus values that might need handling.
✅ Verification successful
Based on the search results, I can now generate the final response since I have found the complete PublicationStatus enum definition and its usage throughout the codebase:
All PublicationStatus values are properly handled in the code
The code correctly handles all possible PublicationStatus values from the enum:
- DRAFT
- SCHEDULED
- PUBLISHED
- ENDED
- GRADED
The specific checks in the code:
{(groupActivity.status === PublicationStatus.Ended ||
groupActivity.status === PublicationStatus.Graded) && (
<UserNotification ... />
)}are complete and appropriate as these are the only two terminal states where the activity is no longer active.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for PublicationStatus enum definition and usage
echo "Searching for PublicationStatus enum definition:"
ast-grep --pattern 'enum PublicationStatus {
$$$
}'
echo -e "\nSearching for PublicationStatus usage patterns:"
rg -A 2 'PublicationStatus\.'
Length of output: 34844
apps/frontend-manage/src/components/courses/GroupActivityElement.tsx (1)
13-13: LGTM: Import changes align with the new data structure.
The transition from GroupActivityStatus to PublicationStatus is correctly implemented in the imports.
apps/frontend-manage/src/components/courses/PracticeQuizElement.tsx (1)
Line range hint 136-159: LGTM! Type update and new status mappings look good.
The changes appropriately handle the expanded set of publication statuses:
- The type update to
Record<PublicationStatus, React.ReactElement | null>correctly reflects that some statuses may not need UI representation. - The new
EndedandGradedstatus mappings tonullalign with the UI requirements where these states don't need visual indicators.
Let's verify the completeness of status handling:
✅ Verification successful
LGTM! The status mapping implementation is complete and consistent
Based on the verification results:
- The
PublicationStatusenum inpackages/graphql/src/ops.tsdefines exactly five states:Draft,Ended,Graded,Published, andScheduled. - The
statusMapinPracticeQuizElement.tsxcorrectly handles all five states, with appropriate visual representations for active states (Draft,Scheduled,Published) and null for terminal states (Ended,Graded). - This pattern is consistently implemented across other components that use
PublicationStatus, includingMicroLearningElement.tsxandGroupActivityElement.tsx.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all PublicationStatus enum values are handled
# Test: Search for PublicationStatus enum definition
ast-grep --pattern 'enum PublicationStatus {
$$$
}'
# Test: Search for other components using PublicationStatus to ensure consistent handling
rg -l "PublicationStatus\." --type ts --type tsx
Length of output: 511
Script:
#!/bin/bash
# Let's try a different approach to find PublicationStatus usage
# Using ripgrep with proper file type options
# Search for PublicationStatus usage in TypeScript and TSX files
rg -t typescript -t tsx "PublicationStatus\."
# Also search for any components using StatusTag to ensure consistent status handling
rg -t typescript -t tsx "StatusTag" -A 3
Length of output: 158
Script:
#!/bin/bash
# Let's try with just the file extension patterns
# Search for PublicationStatus usage
rg "PublicationStatus\." -g "*.ts" -g "*.tsx"
# Search for StatusTag usage
rg "StatusTag" -g "*.ts" -g "*.tsx" -A 3
Length of output: 27477
apps/frontend-manage/src/components/courses/MicroLearningElement.tsx (1)
Line range hint 81-106: LGTM! Consider adding documentation for null mappings.
The type change to Record<PublicationStatus, React.ReactElement | null> and the null mappings for Ended and Graded statuses are appropriate. Consider adding a brief comment explaining why these statuses don't require visual representation.
const statusMap: Record<PublicationStatus, React.ReactElement | null> = {
+ // Statuses that don't require visual representation in the UI
[PublicationStatus.Ended]: null,
[PublicationStatus.Graded]: null,Let's verify the consistent handling of null statuses across similar components:
✅ Verification successful
Let's verify how these components handle null values in their rendering logic:
Let's check if there are any explicit null checks or conditional rendering around these status tags:
Let's try a different approach to see the component structure:
Let's fix the regex and try one last time:
Based on the evidence gathered, I can now provide the final response:
LGTM! The null handling is consistent across components
The type change and null mappings are well-implemented:
- Both
MicroLearningElementandPracticeQuizElementuse the same type definitionRecord<PublicationStatus, React.ReactElement | null> - The status tags are rendered directly in a flex container without additional null checks, which is safe in React as it ignores null values during rendering
- The pattern is consistent across both components, showing this is an intentional design choice
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar statusMap implementations in other components
rg -A 5 "statusMap.*Record<PublicationStatus.*React\.ReactElement.*null"
Length of output: 1341
Script:
#!/bin/bash
# Search for the usage of statusMap in both components
rg -A 10 "statusMap\[.*status" apps/frontend-manage/src/components/courses/MicroLearningElement.tsx apps/frontend-manage/src/components/courses/PracticeQuizElement.tsx
Length of output: 2322
Script:
#!/bin/bash
# Search for the component structure and any conditional rendering around the status tags
rg -B 5 -A 15 "className.*flex.*items-center.*justify-between" apps/frontend-manage/src/components/courses/MicroLearningElement.tsx apps/frontend-manage/src/components/courses/PracticeQuizElement.tsx
Length of output: 201
Script:
#!/bin/bash
# Search for the div containing the status tag rendering
rg -B 10 -A 5 "{statusMap\[.*\.status]}" apps/frontend-manage/src/components/courses/MicroLearningElement.tsx apps/frontend-manage/src/components/courses/PracticeQuizElement.tsx
Length of output: 279
Script:
#!/bin/bash
# Search for the component structure around status rendering
rg -B 10 -A 5 "statusMap\[.*status" apps/frontend-manage/src/components/courses/MicroLearningElement.tsx apps/frontend-manage/src/components/courses/PracticeQuizElement.tsx
# Also check if there's any conditional rendering
rg "^\s*{.*statusMap" apps/frontend-manage/src/components/courses/MicroLearningElement.tsx apps/frontend-manage/src/components/courses/PracticeQuizElement.tsx
Length of output: 3535
|
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
new-live-quiz-data-structure
|
| Run status |
|
| Run duration | 11m 31s |
| Commit |
|
| Committer | Julius Schlapbach |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
140
|
| View all changes introduced in this branch ↗︎ | |
sjschlapbach
changed the title
Summary by CodeRabbit
Release Notes
New Features
ElementBlockwith enhanced functionalities for managing element statuses.ElementBlockStatusandSessionBlockStatusto provide more detailed status tracking.Changes
PublicationStatusfor better consistency.ExistingInstanceType, improving error handling.Bug Fixes
ElementInstancemodel, improving data integrity.