feat: add possibility to end group activity manually and introduce additional ended status #4303
Conversation
Current Aviator status
This PR was merged manually (without Aviator). Merging manually can negatively impact the performance of the queue. Consider using Aviator next time.
See the real-time status of this PR on the
Aviator webapp.
Use the Aviator Chrome Extension
to see the status of your PR within GitHub.
|
klicker-uzh
|
||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Run status |
|
| Run duration | 09m 30s |
| Commit |
|
| Committer | Julius Schlapbach |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
97
|
📝 WalkthroughWalkthroughThis pull request updates the version of the Changes
Possibly related PRs
Suggested reviewers
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 (4)
apps/frontend-pwa/src/pages/group/[groupId]/activity/[activityId].tsx (3)
31-32: LGTM: New state variables and useQuery modifications enhance real-time functionality.The addition of
activityEndedandsubmissionDisabledstate variables, along with thesubscribeToMorein theuseQueryhook, effectively supports the new feature of manually ending group activities and managing submission states.Consider adding comments to explain the purpose of these new state variables for better code readability.
Also applies to: 34-42
166-183: LGTM: Enhanced visual representation of group participants.The updated rendering of group participants with avatars and usernames provides a more engaging and informative display. The implementation includes a fallback to a default avatar and uses a responsive, visually appealing layout.
Consider adding an
alttext to the Image component that describes the participant, e.g.,alt={Avatar for ${participant.username}}, to improve accessibility.
256-256: LGTM: GroupActivityStack now responds to activity end state.The addition of the
activityEndedprop to the GroupActivityStack component and the update to its key prop effectively allow the component to respond to the activity's ended state. This aligns well with the PR objectives of handling ended activities.Consider renaming the
activityEndedprop tosubmissionDisabledfor consistency with the state variable name, unless there's a specific reason for the different naming.Also applies to: 258-259
cypress/cypress/e2e/J-group-activity-workflow.cy.ts (1)
634-657: LGTM: Comprehensive test for ending a group activityThis test case thoroughly covers the process of ending a group activity from the lecturer's perspective. It includes checks for confirmation modals and verifies the activity state change, which are crucial for ensuring the correct functionality of this feature.
Consider adding a check to verify that students can no longer submit responses after the activity is ended, to fully validate the impact of this action.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apps/frontend-pwa/src/pages/group/[groupId]/activity/[activityId].tsx (7 hunks)
- cypress/cypress/e2e/J-group-activity-workflow.cy.ts (8 hunks)
🧰 Additional context used
🔇 Additional comments (18)
apps/frontend-pwa/src/pages/group/[groupId]/activity/[activityId].tsx (9)
8-8: LGTM: New imports align with component enhancements.The new imports (
GroupActivityStatus,Toast,useEffect, andGroupActivitySubscriber) are consistent with the changes made to enhance the component's functionality, particularly in handling the new "ended" status and providing real-time updates.Also applies to: 14-14, 21-21, 23-23
44-51: LGTM: useEffect hook properly manages submission state based on activity status.The new
useEffecthook correctly updates thesubmissionDisabledstate based on the activity's status or theactivityEndedflag. This ensures that submissions are disabled when the activity ends, which aligns with the PR objectives.
88-89: LGTM: Improved data access with groupActivity variable.The introduction of the
groupActivityvariable, which consolidates data fromdata.groupActivityDetails, enhances code readability and reduces repetition throughout the component. This change is consistently applied, making the code more maintainable.Also applies to: 99-100, 128-128, 137-137, 166-183
105-109: LGTM: GroupActivitySubscriber enhances real-time functionality.The addition of the
GroupActivitySubscribercomponent with props foractivityId,subscribeToMore, andsetEndedGroupActivityaligns well with the PR objectives of enhancing real-time functionality for group activities.Could you provide more details about the
GroupActivitySubscribercomponent's implementation and its specific role in managing real-time updates?
113-120: LGTM: UserNotification enhances user feedback for ended activities.The addition of the
UserNotificationcomponent effectively improves user feedback by clearly indicating when an activity has ended or been graded. The component uses appropriate styling for a warning message and checks for bothEndedandGradedstatus, providing comprehensive feedback to the user.
186-201: LGTM: Improved activity start button logic and styling.The updates to the activity start button enhance both its functionality and appearance:
- The button is now correctly displayed only when the activity status is
Published.- Disabling the button when there's only one participant is a good safeguard to ensure group activities have multiple participants.
- The new styling improves the button's visual appeal, and the data-cy attribute enhances testability.
These changes contribute to a better user experience and align with the PR objectives.
270-281: LGTM: Toast component enhances user notification for ended activities.The addition of the Toast component provides an effective, non-intrusive way to notify users when an activity ends. Key points:
- It uses the
activityEndedstate to control its visibility.- The message includes the activity name, providing helpful context to the user.
- The toast is dismissible and has a reasonable duration (10 seconds).
- The implementation aligns well with the PR objectives of improving user feedback for ended activities.
Line range hint
285-301: LGTM: Static generation functions appropriately unchanged.The
getStaticPropsandgetStaticPathsfunctions remain unchanged, which is appropriate as they don't directly relate to the new features introduced in this PR. This maintains the existing static generation behavior of the page.
Line range hint
1-301: Overall LGTM: Changes effectively implement manual activity ending and enhance user experience.This PR successfully implements the ability to end group activities manually and introduces the additional ended status, aligning well with the stated objectives. Key improvements include:
- Enhanced real-time functionality through the GroupActivitySubscriber component and useQuery modifications.
- Improved user feedback with the addition of UserNotification and Toast components for ended activities.
- Better state management for ended activities and disabled submissions.
- Visual enhancements to the group participants display and activity start button.
The changes are well-structured, consistent, and contribute to a better user experience. Minor suggestions for improvements have been made in individual comments.
Great job on this implementation!
cypress/cypress/e2e/J-group-activity-workflow.cy.ts (9)
510-521: LGTM: Well-structured function to check disabled inputsThe
checkInputsDisabledfunction is a good addition. It systematically checks various input types (SC, MC, KPRIM, numerical, and free-text) to ensure they are disabled. This will be useful for verifying the state of inputs after a group activity has ended.
622-632: LGTM: Good test coverage for multi-group scenariosThis test case is a valuable addition as it verifies that students from different groups can start the group activity. It helps ensure that the group activity functionality works correctly across multiple groups.
659-674: LGTM: Essential test for submission visibility after activity endThis test case is crucial for ensuring that students can still view their submissions after a group activity has ended. It effectively uses the
checkInputsDisabledandcheckPersistentAnswersfunctions to validate the state of the submission, and also verifies the correct status message and the absence of the submit button.
676-692: LGTM: Crucial test for unsubmitted activities after endThis test case is essential for verifying the behavior of started but unsubmitted group activities after they've ended. It correctly checks that inputs are disabled, the appropriate status message is displayed, and submission is no longer possible. This ensures a consistent and fair experience for all students, regardless of their submission status when the activity ends.
694-705: LGTM: Critical test for preventing late startsThis test case is vital for ensuring that students cannot start a group activity after it has ended. It correctly verifies the absence of the start button and the presence of the appropriate status message. This test helps maintain the integrity of the activity timeline and ensures fair participation conditions for all students.
711-712: LGTM: Improved variable clarityThe separation of
groupActivityNameandactivityDisplayNameimproves code clarity and potentially aligns with changes in the application's naming conventions or data structure. This modification enhances the maintainability of the test case.
878-897: LGTM: Enhanced safety measures for group activity deletionThe changes to the deletion confirmation process significantly improve the safety of deleting group activities. The addition of disabled state checks for the confirmation button and the requirement for explicit confirmations for deleting started instances and submissions are excellent safeguards against accidental deletions. These enhancements align with best practices for handling destructive actions in user interfaces.
909-911: LGTM: Consistent cleanup processThe changes to the cleanup process align well with the enhanced deletion confirmation steps introduced earlier in the file. This ensures consistency in the group activity deletion process throughout the test suite, reinforcing the safety measures against accidental deletions during cleanup.
Line range hint
1-915: Overall: Excellent enhancements to group activity testingThe changes in this file significantly improve the test coverage and robustness of the group activity workflow. The new test cases address important scenarios such as multi-group interactions, activity ending, and various edge cases. The enhanced deletion confirmation process adds a crucial layer of safety against accidental data loss. These improvements will contribute to a more reliable and well-tested group activity feature.
klicker-uzh
|
||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Run status |
|
| Run duration | 09m 31s |
| Commit |
|
| Committer | Julius Schlapbach |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
Failures
|
0
|
|
Flaky
|
0
|
|
Pending
|
0
|
|
Skipped
|
0
|
|
Passing
|
97
|
Summary by CodeRabbit
New Features
GroupActivityExtensionButton,GroupActivityGradingLink,GroupActivityEndingModal, andGroupActivitySubscriber.Bug Fixes
Documentation