-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: ensure that participant group names cannot be empty #4280
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.
|
📝 WalkthroughWalkthroughThe pull request introduces several enhancements to the handling of participant groups within the application. Key modifications include the addition of input validation for group names and form submissions across multiple components. The changes ensure that group names are trimmed of whitespace, preventing empty submissions. Additionally, new localization keys have been added to support group-related functionalities in both German and English. 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 (
|
Quality Gate failedFailed conditions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (13)
apps/frontend-pwa/src/components/participant/groups/GroupCreationBlock.tsx (1)
29-35
: LGTM: Validation schema implemented correctly.The validation schema effectively ensures that group names cannot be empty, aligning with the PR objective. Good job on using
trim()
to prevent whitespace-only names and internationalizing the error message.Consider simplifying the schema slightly:
validationSchema={Yup.object().shape({ value: Yup.string() .trim() .required(t('pwa.groups.nameRequired')), })}This achieves the same result more concisely, as Yup's
trim()
method automatically removes leading and trailing whitespace before validation.apps/frontend-pwa/src/components/course/EditableGroupName.tsx (2)
45-49
: LGTM: Additional check for empty group namesThis change adds an extra layer of protection against saving empty group names, which is good defensive programming. However, it might be redundant given the button's disabled state.
Consider combining the empty check with the button click handler to reduce code duplication:
<Button disabled={groupNameValue.trim() === ''} onClick={async () => { - if (groupNameValue.trim() === '') { - setEditMode(false) - return - } - + const trimmedName = groupNameValue.trim(); + if (trimmedName === '') return; await renameParticipantGroup({ variables: { groupId, - name: groupNameValue.trim(), + name: trimmedName, }, }) setEditMode(false) }} loading={submitting} className={{ root: 'h-7' }} > {t('shared.generic.save')} </Button>This refactoring maintains the safety check while slightly reducing code complexity.
Line range hint
1-74
: Overall assessment: Effective implementation of empty group name preventionThe changes in this file successfully address the PR objective of ensuring that participant group names cannot be empty. The implementation is thorough and considers multiple scenarios:
- UI level: The save button is disabled when the group name is empty or contains only whitespace.
- Logic level: An additional check prevents the rename operation if the group name is empty, even if the button's disabled state is bypassed.
- Data level: The group name is trimmed before being sent to the mutation, ensuring no leading or trailing whitespace.
These multi-layered checks provide robust protection against empty group names while maintaining a good user experience.
Consider adding a unit test to verify this behavior, ensuring that empty group names are consistently prevented across all levels of the application.
apps/frontend-pwa/src/components/participant/groups/GroupJoinBlock.tsx (2)
33-39
: Validation schema looks good, with a minor improvement suggestion.The validation schema effectively ensures that the input is not empty, numeric, and exactly 6 characters long, which aligns well with the PR objective. The use of internationalized error messages is commendable.
Consider refactoring the numeric validation for better readability:
validationSchema={Yup.object().shape({ value: Yup.string() .required(t('pwa.groups.pinRequired')) - .test('is-numeric', t('pwa.groups.pinNumeric'), (value) => { - return !isNaN(Number(value)) && value.length === 6 - }), + .matches(/^\d{6}$/, t('pwa.groups.pinNumeric')), })}This change uses a regular expression to validate both the numeric nature and the length in one step, which is more concise and potentially more efficient.
🧰 Tools
🪛 Biome
[error] 37-37: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
Line range hint
40-65
: Consider simplifying the onSubmit function.The validation schema ensures that the input is a valid 6-digit number. Given this, we can simplify the
onSubmit
function:onSubmit={async (value) => { const result = await joinParticipantGroup({ variables: { courseId: courseId, - code: Number(value) >> 0, + code: parseInt(value, 10), }, // ... rest of the function }) // ... rest of the function }}This change:
- Removes the redundant
Number()
conversion.- Replaces the bitwise operation with
parseInt()
, which is more explicit and maintainable.- Specifies base 10 to avoid potential issues with leading zeros.
These modifications make the code more readable and align better with the added validation.
🧰 Tools
🪛 Biome
[error] 37-37: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
apps/frontend-pwa/src/components/participant/groups/GroupAction.tsx (2)
24-24
: Consider removing unnecessary prop fromGroupActionButtonProps
The
validationSchema
prop with typenever
seems unnecessary for the button variant of the component. Since it's not used in button mode, it's better to remove it to keep the interface clean and avoid confusion.Consider applying this change:
interface GroupActionButtonProps extends GroupActionProps { onClick: () => void explanation: string onSubmit?: never - validationSchema?: never placeholder?: never textSubmit?: never }
69-71
: LGTM: Improved form handling and validationThe changes to the Formik setup significantly improve form handling and validation:
- Trimming the input value prevents submission of whitespace-only strings.
- Passing
validationSchema
enables custom validation rules.- Setting
validateOnMount
ensures immediate validation on component mount.These improvements align well with the PR objective of ensuring group names are not empty.
Consider adding a check to prevent submission of an empty string after trimming:
- onSubmit={async (values) => await onSubmit(values.value.trim())} + onSubmit={async (values) => { + const trimmedValue = values.value.trim(); + if (trimmedValue) { + await onSubmit(trimmedValue); + } + }}This extra check adds an additional layer of protection against empty submissions.
packages/i18n/messages/en.ts (3)
Line range hint
460-522
: New course-related translations look good, with a minor suggestion for consistency.The newly added translations in the
courses
section are clear, concise, and well-written. They cover a wide range of course-related features, including group creation, leaderboards, and other functionalities. This will greatly enhance the user experience for course participants.For improved consistency, consider capitalizing "Let's go!" in the following translations:
- noGroups: "No groups have been created yet. Let's go!", - noGroupPoints: "No group points have been collected yet. Let's go!", + noGroups: "No groups have been created yet. Let's Go!", + noGroupPoints: "No group points have been collected yet. Let's Go!",This would align with the capitalization style used in other exclamatory phrases throughout the translations.
Line range hint
694-736
: Group activity translations are comprehensive and well-written, with a minor typo to fix.The newly added translations in the
groupActivity
section provide excellent coverage of various aspects of group activities, including activity status, feedback, and group formation. These translations will greatly enhance the user experience for participants engaging in group activities.There's a minor typo in one of the translations that should be corrected:
- groupActivityPassed: 'Congratulations! Your group has passed the group activity.', - groupActivityFailed: 'Oh no! Your group has unfortunately not passed the group activity.', + groupActivityPassed: 'Congratulations! Your group has passed the group activity.', + groupActivityFailed: 'Oh no! Your group has unfortunately not passed the group activity.', groupActivityFeedback: 'Feedback: {feedback}', answerCORRECT: 'Your answer is correct.', - answerPARTIAL: 'Your answer is partially correct.', + answerPARTIAL: 'Your answer is partially correct.', answerINCORRECT: 'Your answer is incorrect.',The word "partially" is misspelled as "partialy" in the
answerPARTIAL
translation. Fixing this typo will ensure consistent and correct messaging for users.
Line range hint
1-1180
: Overall, excellent additions to the English translations with a suggestion for future updates.The new translations added to this file significantly enhance the KlickerUZH application's support for course, group, and group activity features. They are well-written, clear, and maintain a consistent style with the existing translations. This will greatly improve the user experience for both lecturers and students using these features.
For future updates, consider implementing a more structured approach to managing translations:
Split the translations into separate files based on features or application sections (e.g.,
courses.ts
,groups.ts
,groupActivities.ts
). This would make the file more manageable and easier to update.Use a translation management system or tool to help maintain consistency across translations and make it easier to spot missing or outdated translations.
Implement a automated testing system for translations to catch potential issues like missing placeholders or inconsistent capitalization.
These suggestions could help streamline the translation process and maintain high-quality translations as the application continues to grow and evolve.
packages/i18n/messages/de.ts (2)
Line range hint
1-1000
: Minor suggestions for improved consistency and clarity.While the translations are generally of high quality, consider the following suggestions for improvement:
In the
courses
section, the keycreateJoinRandomGroup
(line 466) uses "Hier klicken" (click here), which is not used in other similar messages. Consider rephrasing to match the style of other action-related messages.In the
groupActivity
section, the keygroupActivityName
(line 731) is in English. Translate this to German for consistency.Consider adding more context or examples to complex messages, such as those in the
groupActivity
section, to help users better understand the functionality.These minor adjustments would further enhance the overall consistency and clarity of the translations.
Line range hint
1-1000
: Reminder: Keep translations updated with new features.The translation file appears to be comprehensive, covering a wide range of functionality across different parts of the application. As new features are developed and added to the application, please remember to:
- Add corresponding translations for any new user-facing text.
- Update existing translations if the functionality or user interface changes.
- Remove obsolete translations if features are deprecated or removed.
Regularly reviewing and updating this file will ensure that the German localization remains complete and accurate as the application evolves.
packages/graphql/src/services/groups.ts (1)
61-61
: Refactor to trim the group name once and reuse the trimmed valueCurrently,
name.trim()
is called multiple times. Trimming the name once and storing it in a variable can improve readability and performance.Apply this diff:
+ const trimmedName = name.trim(); if (!course || !course.isGroupCreationEnabled || trimmedName === '') { throw new GraphQLError('Group name cannot be empty') } // ... data: { - name: name.trim(), + name: trimmedName, },Also applies to: 832-832, 841-841
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- apps/frontend-pwa/src/components/course/EditableGroupName.tsx (1 hunks)
- apps/frontend-pwa/src/components/participant/groups/GroupAction.tsx (4 hunks)
- apps/frontend-pwa/src/components/participant/groups/GroupCreationBlock.tsx (2 hunks)
- apps/frontend-pwa/src/components/participant/groups/GroupJoinBlock.tsx (2 hunks)
- packages/graphql/src/services/groups.ts (2 hunks)
- packages/i18n/messages/de.ts (2 hunks)
- packages/i18n/messages/en.ts (2 hunks)
🧰 Additional context used
🪛 Biome
apps/frontend-pwa/src/components/participant/groups/GroupJoinBlock.tsx
[error] 37-37: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (15)
apps/frontend-pwa/src/components/participant/groups/GroupCreationBlock.tsx (2)
9-9
: LGTM: Yup import added correctly.The import of Yup is correctly placed and aligns with the PR objective of implementing input validation for group names.
Line range hint
1-64
: Summary: PR objective successfully implemented.The changes effectively implement the PR objective of ensuring that participant group names cannot be empty. The addition of Yup for validation enhances the user experience without altering the existing functionality of group creation and data refetching.
Key points:
- Yup library is correctly imported and utilized.
- Validation schema is properly integrated into the
GroupAction
component.- Existing mutation and refetch queries are preserved, maintaining functionality.
These changes improve the robustness of the group creation process while maintaining the component's overall structure and behavior.
apps/frontend-pwa/src/components/course/EditableGroupName.tsx (2)
43-43
: LGTM: Prevent saving empty group namesThis change effectively prevents saving empty or whitespace-only group names, which aligns with the PR objective. Good use of the
trim()
method to handle whitespace-only inputs.
53-53
: LGTM: Trim group name before mutationExcellent change. Trimming the group name before sending it to the mutation ensures data cleanliness and consistency. This prevents any unintended leading or trailing whitespace in group names.
apps/frontend-pwa/src/components/participant/groups/GroupJoinBlock.tsx (2)
10-10
: LGTM: Yup import added correctly.The Yup library is imported correctly for input validation. Consider using named imports in the future if only specific Yup functions are needed to optimize bundle size.
Line range hint
1-93
: Potential misalignment with PR objectives.The PR objective states "ensure that participant group names cannot be empty", but the implemented changes validate a 6-digit numeric join code, not a group name.
Could you clarify if:
- The PR title is incorrect and should refer to join codes instead of group names?
- There are missing changes related to group name validation?
- The join code is considered as the group name in this context?
This clarification will help ensure that the implementation fully meets the intended objectives.
🧰 Tools
🪛 Biome
[error] 37-37: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
apps/frontend-pwa/src/components/participant/groups/GroupAction.tsx (3)
47-47
: LGTM:validationSchema
prop added toGroupAction
componentThe addition of
validationSchema
to the component's props is consistent with the interface updates and allows for flexible form validation. This change enhances the component's functionality and customization options.
73-91
: LGTM: Improved form submission handlingThe changes to the Form component enhance the user experience and prevent submission of invalid data:
- Utilizing Formik's
isValid
prop to manage the submit button's state is a best practice.- Disabling the submit button when the form is invalid provides immediate feedback to the user.
These improvements align well with the PR objective and follow React and Formik best practices.
Line range hint
1-114
: Summary: Solid implementation with minor suggestions for improvementOverall, the changes in this file effectively address the PR objective of ensuring that participant group names cannot be empty. The implementation improves form handling, validation, and user experience. Here's a summary of the key points:
- The addition of
validationSchema
enables custom validation rules.- Trimming the input value prevents submission of whitespace-only strings.
- The submit button is disabled when the form is invalid, providing immediate feedback to the user.
Minor suggestions for improvement include:
- Removing the unnecessary
validationSchema
prop fromGroupActionButtonProps
.- Using a more specific type for
validationSchema
inGroupActionFormProps
.- Adding an additional check to prevent submission of an empty string after trimming.
These changes significantly enhance the component's functionality and align well with React and Formik best practices.
packages/i18n/messages/en.ts (1)
691-693
: Group-related error messages are clear and consistent.The newly added translations in the
groups
section provide clear and concise error messages for input validation. These messages will help users understand and correct issues when creating or joining groups.The translations are consistent with the existing style of error messages in the application, which is excellent for maintaining a cohesive user experience.
packages/i18n/messages/de.ts (5)
Line range hint
460-465
: New translations added for course-related functionality.The following new keys have been added to the
courses
section:
createJoinGroup
createGroup
joinGroup
groupName
randomGroup
These additions seem to be related to group creation and joining functionality within courses. The translations are concise and clear.
Line range hint
466-474
: Additional group-related translations added.New keys added:
createJoinRandomGroup
joinGroupError
joinGroupFull
inRandomGroupPool
leaveRandomGroupPool
These translations provide feedback for various group-related actions and states. The messages are informative and user-friendly.
687-689
: New translations added for group-related input validation.The following keys have been added to the
groups
section:
nameRequired
pinRequired
pinNumeric
These translations provide error messages for group creation form validation. They are concise and clear.
Line range hint
690-734
: Group activity translations are comprehensive and consistent.The
groupActivity
section provides a wide range of translations covering various aspects of group activities, including:
- Activity states (available, started, submitted, past, passed, failed)
- Feedback messages
- Instructions for participants
- Error messages and warnings
The translations maintain a consistent style and use appropriate terminology throughout the section. They provide clear and informative messages for users interacting with group activities.
Line range hint
1-1000
: Overall, excellent quality and comprehensive German localization.This localization file demonstrates a thorough and professional approach to providing German translations for the application. Key strengths include:
- Comprehensive coverage of various application components and features.
- Consistent style and terminology usage throughout the file.
- Clear and concise translations that effectively convey the intended meanings.
- Newly added group-related translations that integrate seamlessly with existing content.
The file structure is well-organized, making it easy to locate and maintain translations for specific parts of the application. With minor suggestions for improvement noted earlier, this localization file serves as a solid foundation for delivering a high-quality German language experience to users.
klicker-uzh Run #3071
Run Properties:
|
Project |
klicker-uzh
|
Branch Review |
require-non-empty-group-name
|
Run status |
Passed #3071
|
Run duration | 08m 56s |
Commit |
4055911f57 ℹ️: Merge d852b048b6e7c89af16c2f34c8ee9afcdcbf8582 into 5e96551007c22e3da5169d1170f6...
|
Committer | Julius Schlapbach |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
1
|
Pending |
0
|
Skipped |
0
|
Passing |
44
|
View all changes introduced in this branch ↗︎ |
klicker-uzh Run #3072
Run Properties:
|
Project |
klicker-uzh
|
Branch Review |
v3
|
Run status |
Passed #3072
|
Run duration | 09m 50s |
Commit |
ba3fdda3ec: fix: ensure that participant group names cannot be empty (#4280)
|
Committer | Julius Schlapbach |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
2
|
Pending |
0
|
Skipped |
0
|
Passing |
44
|
View all changes introduced in this branch ↗︎ |
No description provided.