enhance: add functionality to remove or delete answer collections, withdraw access requests, improve modification logic #4448
Conversation
…nd collection deletion
|
Warning Rate limit exceeded@sjschlapbach has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 38 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis pull request introduces comprehensive enhancements to the management of answer collections across the frontend and backend of the application. The changes focus on adding new functionalities for deleting, removing, and canceling answer collections, with robust state management and user feedback mechanisms. New GraphQL mutations and schema updates support these actions, while frontend components are updated to handle success and error states through toast notifications and modal interactions. 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 (
|
… to display them after item removal
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
apps/frontend-manage/src/components/resources/answerCollections/AnswerCollectionItem.tsx (1)
Line range hint
73-87: Extract click handler logic to a separate functionThe click handler contains complex conditional logic that should be extracted for better maintainability.
const handleClick = () => { if (editable) { setActiveModal('edit') } else if (accessGranted) { setActiveModal('viewing') } else { setActiveModal('cancellation') } }
🧹 Nitpick comments (30)
packages/graphql/src/services/questions.ts (1)
Line range hint
439-443: Consider implementing the TODO comment.The TODO comment suggests reworking the approach to perform actual deletion instead of soft deletion. Would you like me to help create a GitHub issue to track this task?
packages/i18n/messages/en.ts (4)
1856-1857: Improve message clarity by connecting the cause and effect.The message first explains that the option is used as a correct solution but then states it cannot be deleted without connecting these facts. This could be confusing for users.
Consider this revision for better clarity:
- 'Answer options marked with the warning symbol are already used as correct solutions in a question by you or other users (in case of a shared collection). Please keep this in mind when editing the answer. The option cannot be deleted.', + 'Answer options marked with the warning symbol cannot be deleted as they are already used as correct solutions in a question by you or other users (in case of a shared collection). Please keep this in mind when editing the answer.',
1929-1941: Improve consistency in deletion-related messages.The deletion confirmation messages could be more specific about the consequences and maintain consistent punctuation.
Consider these revisions:
- deleteCollection: 'Delete collection', + deleteCollection: 'Delete Collection', - deletionDisabledInUse: - 'This collection is used by at least one of your questions. Therefore, you cannot delete the collection. To delete the collection, please first remove it from all questions that use it.', + deletionDisabledInUse: + 'This collection cannot be deleted as it is used by at least one of your questions. To delete the collection, please first remove it from all questions that use it.', deleteAnswerCollection: 'Delete Answer Collection', - confirmCollectionDeletion: - 'Are you sure you want to delete the answer collection "{name}" from your profile? For shared answer collections, access for other users remains as long as they use the collection.', + confirmCollectionDeletion: + 'Are you sure you want to delete the answer collection "{name}" from your profile? Note: For shared answer collections, other users will retain access as long as they use the collection.',
1942-1948: Improve consistency in removal-related messages.The removal confirmation messages could be more specific about the consequences and maintain consistent punctuation.
Consider these revisions:
- removeAnswerCollection: 'Remove Answer Collection', - confirmCollectionRemoval: - 'Are you sure you want to remove the answer collection "{name}" from your profile?', + removeAnswerCollection: 'Remove Collection', + confirmCollectionRemoval: + 'Are you sure you want to remove the answer collection "{name}" from your profile? Note: This will only remove your access to the collection.', confirmRemoval: 'Confirm removal', - removalSuccessful: 'The answer collection was successfully removed.', + removalSuccessful: 'The answer collection has been successfully removed.', - removalFailed: - 'An error occurred while removing the answer collection. Please try again or contact the support.', + removalFailed: + 'An error occurred while removing the answer collection. Please try again or contact support.',
1949-1955: Improve consistency in cancellation-related messages.The cancellation messages could maintain consistent punctuation and terminology.
Consider these revisions:
cancelSharingRequest: 'Cancel Sharing Request', confirmCancelRequest: - 'Please confirm that you want to cancel the sharing request for the answer collection "{name}". You can request access to the answer collection again later.', + 'Are you sure you want to cancel the sharing request for the answer collection "{name}"? You can request access to this collection again later.', confirmCancellation: 'Confirm cancellation', - cancellationSuccessful: 'The access request was successfully withdrawn.', + cancellationSuccessful: 'The access request has been successfully cancelled.', cancellationFailed: - 'An error occurred while withdrawing the access request. Please try again or contact the support.', + 'An error occurred while cancelling the access request. Please try again or contact support.',packages/graphql/src/services/resources.ts (2)
85-92: Consider refactoring duplicated_countselections to improve maintainabilityThe
_countselection forlinkedElementswith the conditionownerId: ctx.user.subis duplicated across multiple queries withingetAnswerCollections. Extracting this logic into a reusable helper or shared include object could enhance maintainability and reduce potential errors from code duplication.Also applies to: 109-117, 130-138
255-271: Remove unnecessary inclusion ofaccessRequestedto optimize query performanceIn the
deleteAnswerCollectionfunction, theincludeoption of the Prisma query includesaccessRequested: true, but this data is not used within the function. Removing this inclusion could improve query performance.apps/frontend-manage/src/components/resources/answerCollections/CollectionRemovalErrorToast.tsx (1)
4-24: Consider creating a genericErrorToastcomponent to reduce code duplicationThe
CollectionRemovalErrorToastandCollectionDeletionErrorToastcomponents have very similar implementations. To improve code reusability and maintainability, consider creating a genericErrorToastcomponent that accepts the error message or translation key as a prop.apps/frontend-manage/src/components/resources/answerCollections/CollectionDeletionErrorToast.tsx (1)
4-24: Consider creating a genericErrorToastcomponent to reduce code duplicationThe
CollectionDeletionErrorToastandCollectionRemovalErrorToastcomponents have very similar implementations. To improve code reusability and maintainability, consider creating a genericErrorToastcomponent that accepts the error message or translation key as a prop.apps/frontend-manage/src/components/resources/answerCollections/RequestCancellationSuccessToast.tsx (2)
1-26: Consider accessibility in toast notifications.While the component structure is consistent with others, consider these accessibility improvements for all toast components:
- Allow customizable duration based on message length (WCAG 2.2.1 Timing Adjustable)
- Ensure toast messages are announced by screen readers
- Consider adding
role="alert"for error toastsExample implementation in BaseToast:
interface BaseToastProps { // ... existing props ... /** Duration in ms. If not provided, calculated based on content length */ duration?: number /** Minimum duration in ms. Default: 3000 */ minDuration?: number } function BaseToast({ // ... existing props ... duration, minDuration = 3000, }: BaseToastProps) { const t = useTranslations() const message = t(translationKey) // Calculate duration based on content length if not provided const calculatedDuration = duration ?? Math.max( minDuration, message.length * 100 // 100ms per character ) return ( <Toast dismissible type={type} openExternal={open} onCloseExternal={onClose} duration={calculatedDuration} role={type === 'error' ? 'alert' : 'status'} aria-live={type === 'error' ? 'assertive' : 'polite'} > {message} </Toast> ) }
1-26: Consider architectural improvements for toast notification system.The current implementation provides good separation of concerns but could benefit from these architectural improvements:
- Create a toast context/provider to manage multiple toasts
- Implement a toast queue to handle multiple notifications
- Add analytics tracking for user interactions with toasts
Example implementation:
// ToastContext.tsx interface ToastState { queue: Array<{ type: 'success' | 'error' translationKey: string duration?: number }> } const ToastContext = createContext<{ state: ToastState addToast: (toast: Omit<ToastState['queue'][0], 'id'>) => void removeToast: (id: string) => void }>({} as any) function ToastProvider({ children }: { children: React.ReactNode }) { // Implementation details... } // Usage in components: function SomeComponent() { const { addToast } = useToast() const handleSuccess = () => { addToast({ type: 'success', translationKey: 'manage.resources.cancellationSuccessful' }) } }apps/frontend-manage/src/components/resources/answerCollections/AnswerCollectionList.tsx (1)
12-17: Consider consolidating the state management.While the implementation is functional, having six separate boolean states for success/failure scenarios could be simplified. Consider using an enum or a combined state object for better maintainability.
Example approach:
type ActionStatus = { action: 'deletion' | 'removal' | 'cancellation' status: 'success' | 'failure' | null } // Single state setter instead of six setActionStatus: Dispatch<SetStateAction<ActionStatus>>Also applies to: 21-26
apps/frontend-manage/src/components/resources/answerCollections/AnswerCollectionEditModal.tsx (1)
13-13: Consider adding loading state during deletion.While the implementation is correct, the UI might benefit from showing a loading state during the deletion process to prevent multiple clicks and provide better user feedback.
function AnswerCollectionEditModal({ collection, open, onClose, onDelete, }) { const t = useTranslations() const [successToast, setSuccessToast] = useState(false) + const [isDeleting, setIsDeleting] = useState(false) // ... <AnswerCollectionMetaForm collection={collection} setSuccessToast={setSuccessToast} + isDeleting={isDeleting} onDelete={async () => { + setIsDeleting(true) await onDelete() + setIsDeleting(false) onClose() }} />Also applies to: 18-18, 38-41
apps/frontend-manage/src/components/resources/answerCollections/CollectionDeletionModal.tsx (2)
63-87: Enhance delete button UX and add loading state.The delete button could benefit from:
- Loading state during deletion
- More explicit danger styling
- Disabled state during loading
+ const [isDeleting, setIsDeleting] = useState(false) <Button onClick={async () => { + setIsDeleting(true) const { data, errors } = await deleteAnswerCollection() + setIsDeleting(false) if ( typeof data?.deleteAnswerCollection !== 'undefined' && data?.deleteAnswerCollection !== null && !errors ) { setDeletionSuccess(true) setDeletionModal(false) } else { setDeletionFailure(true) } }} + disabled={isDeleting} className={{ - root: 'float-right mt-4 flex flex-row gap-1.5 border border-red-600', + root: 'float-right mt-4 flex flex-row gap-1.5 border border-red-600 bg-red-50 hover:bg-red-100', }} data={{ cy: 'confirm-delete-answer-collection' }} > - <FontAwesomeIcon icon={faTrashCan} /> + {isDeleting ? ( + <FontAwesomeIcon icon={faSpinner} spin /> + ) : ( + <FontAwesomeIcon icon={faTrashCan} /> + )} <div> {t('manage.resources.confirmDeletion', { name: collection.name })} </div> </Button>
67-71: Simplify the success condition check.The current success condition check is verbose and could be simplified.
- if ( - typeof data?.deleteAnswerCollection !== 'undefined' && - data?.deleteAnswerCollection !== null && - !errors - ) { + if (data?.deleteAnswerCollection && !errors) {apps/frontend-manage/src/components/resources/answerCollections/CollectionRemovalModal.tsx (3)
29-52: Enhance cache update logic with defensive checks.The cache update logic could be more defensive:
- Check if
collections.sharedCollectionsexists before filtering- Consider handling edge cases where cache might be empty
update: (cache, { data }) => { const res = data?.removeAnswerCollection if (res === null || typeof res === 'undefined') return const prevQuery = cache.readQuery({ query: GetAnswerCollectionsDocument, }) const collections = prevQuery?.getAnswerCollections if (!collections) return + const sharedCollections = collections.sharedCollections + if (!sharedCollections) return cache.writeQuery({ query: GetAnswerCollectionsDocument, data: { getAnswerCollections: { ...collections, answerCollections: collections.answerCollections ?? [], requestedCollections: collections.requestedCollections ?? [], - sharedCollections: collections.sharedCollections?.filter( + sharedCollections: sharedCollections.filter( (c) => c.id !== res ), }, }, }) }
70-70: Remove console.log statement.Remove debugging console.log statement before merging.
-console.log(data, errors)
68-82: Improve error handling with specific error messages.The error handling could be improved by:
- Logging errors properly
- Providing specific error messages based on error types
onClick={async () => { const { data, errors } = await removeAnswerCollection() - console.log(data, errors) + if (errors) { + console.error('Failed to remove collection:', errors) + setRemovalFailure(true) + return + } if ( typeof data?.removeAnswerCollection !== 'undefined' && data?.removeAnswerCollection !== null && - !errors ) { setRemovalSuccess(true) setRemovalModal(false) } else { + console.error('Unexpected response:', data) setRemovalFailure(true) } }}apps/frontend-manage/src/components/resources/answerCollections/RequestCancellationModal.tsx (1)
30-54: Consider optimistic updates for better UXThe cache update logic is correct but could be enhanced with optimistic updates to provide immediate feedback to users.
Consider implementing optimistic updates:
const [cancelAnswerCollectionRequest] = useMutation( CancelAnswerCollectionRequestDocument, { + optimisticResponse: { + cancelAnswerCollectionRequest: collection.id, + }, variables: { collectionId: collection.id }, update: (cache, { data }) => {apps/frontend-manage/src/components/resources/AnswerCollections.tsx (2)
21-27: Consider using a reducer for complex state managementManaging multiple boolean states for different actions could be simplified using a reducer pattern.
type ActionState = { type: 'deletion' | 'removal' | 'cancellation' status: 'success' | 'failure' | null } const actionReducer = (state: ActionState, action: { type: string, payload?: any }) => { switch (action.type) { case 'SET_SUCCESS': return { ...state, status: 'success' } case 'SET_FAILURE': return { ...state, status: 'failure' } case 'RESET': return { ...state, status: null } default: return state } }
58-81: Consider consolidating toast notificationsThe multiple toast components could be consolidated into a single reusable component.
type ToastProps = { type: 'success' | 'error' action: 'deletion' | 'removal' | 'cancellation' open: boolean onClose: () => void } const ActionToast = ({ type, action, open, onClose }: ToastProps) => { // Implementation }apps/frontend-manage/src/components/resources/answerCollections/AnswerCollectionMetaForm.tsx (1)
Line range hint
42-57: Optimize form submission with debounceThe form submission could benefit from debouncing to prevent multiple rapid submissions.
import { useMemo } from 'react' import debounce from 'lodash/debounce' const debouncedSubmit = useMemo( () => debounce(async (values) => { const { data } = await modifyAnswerCollection({ variables: { id: collection.id, name: values.name !== collection.name ? values.name : undefined, // ... rest of the variables }, }) if (data?.modifyAnswerCollection?.id) { setSuccessToast(true) } }, 300), [collection.id, modifyAnswerCollection] )apps/frontend-manage/src/components/resources/answerCollections/AnswerCollectionItem.tsx (2)
42-47: Consolidate modal states using a single state variableMultiple boolean states for modals could be simplified using a single state variable.
type ModalType = 'edit' | 'deletion' | 'viewing' | 'removal' | 'cancellation' | null const [activeModal, setActiveModal] = useState<ModalType>(null)
134-142: Extract access level UI componentsThe conditional rendering for different access levels could be extracted into separate components for better maintainability.
const RequestedAccessUI = () => ( <div className="flex flex-col items-end gap-0.5 py-0.5"> <div className="text-primary-100 flex flex-row items-center gap-2"> <FontAwesomeIcon icon={faClock} /> <div>{t('manage.resources.requestedAccess')}</div> </div> <div className="flex flex-row items-center gap-1.5 text-sm"> <FontAwesomeIcon icon={faHandPointer} /> <div>{t('manage.resources.clickToCancelRequest')}</div> </div> </div> )cypress/cypress/e2e/K-resources-workflow.cy.ts (2)
Line range hint
1249-1255: TODO comment needs implementation.The test case for verifying that answer collections cannot be deleted is currently a TODO stub and needs to be implemented.
Would you like me to help implement this test case? I can generate a solution that verifies:
- The delete button is disabled when a collection is in use
- A corresponding message is shown to the user
Line range hint
1441-1443: TODO comment needs implementation.The cleanup test for shared answer collections is currently a TODO stub and needs to be implemented.
Would you like me to help implement this cleanup test? I can generate a solution that:
- Deletes the shared answer collection
- Verifies it's removed from the UI
- Verifies any related data is cleaned up
packages/graphql/src/ops.ts (1)
1118-1118: Consider documenting the meaning of return values.The mutations
cancelAnswerCollectionRequest,deleteAnswerCollection, andremoveAnswerCollectionreturnMaybe<Int>. Please document what these integer values represent (e.g., collection ID, success status, affected rows).Also applies to: 1139-1139, 1202-1202
packages/graphql/src/public/schema.graphql (1)
1025-1025: Document the meaning of return values for collection mutations.The mutations
cancelAnswerCollectionRequest,deleteAnswerCollection, andremoveAnswerCollectionall returnInt. Please clarify:
- What values can be returned?
- What does each value indicate (success/error codes)?
- Are these values consistent across all collection mutations?
Consider adding descriptions to document the behavior:
+"# Cancels a pending request for access to an answer collection." +"# Returns 1 if successful, 0 if request not found." mutation cancelAnswerCollectionRequest(collectionId: Int!): Int +"# Permanently deletes an answer collection owned by the current user." +"# Returns 1 if successful, 0 if collection not found or not owned by user." mutation deleteAnswerCollection(collectionId: Int!): Int +"# Removes a shared answer collection from the current user's collections." +"# Returns 1 if successful, 0 if collection not found or not shared with user." mutation removeAnswerCollection(collectionId: Int!): IntAlso applies to: 1046-1046, 1109-1109
packages/graphql/src/ops.schema.json (2)
897-908: Add description for theisRemovablefield.The
isRemovablefield lacks a description explaining its purpose and usage. Consider adding a description to improve schema documentation.{ "name": "isRemovable", - "description": null, + "description": "Indicates whether the answer collection can be removed by the current user", "args": [],
11720-11735: Add descriptions for mutation arguments.The
collectionIdargument in all mutations lacks a description explaining its purpose.{ "name": "collectionId", - "description": null, + "description": "The unique identifier of the answer collection", "type": { "kind": "NON_NULL",Also applies to: 13360-13376, 16524-16540
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
apps/frontend-manage/src/components/resources/AnswerCollections.tsx(2 hunks)apps/frontend-manage/src/components/resources/SharedAnswerCollectionList.tsx(3 hunks)apps/frontend-manage/src/components/resources/answerCollections/AnswerCollectionCollapsible.tsx(1 hunks)apps/frontend-manage/src/components/resources/answerCollections/AnswerCollectionEditModal.tsx(2 hunks)apps/frontend-manage/src/components/resources/answerCollections/AnswerCollectionItem.tsx(3 hunks)apps/frontend-manage/src/components/resources/answerCollections/AnswerCollectionList.tsx(1 hunks)apps/frontend-manage/src/components/resources/answerCollections/AnswerCollectionMetaForm.tsx(3 hunks)apps/frontend-manage/src/components/resources/answerCollections/AnswerCollectionViewingModal.tsx(2 hunks)apps/frontend-manage/src/components/resources/answerCollections/CollectionDeletionErrorToast.tsx(1 hunks)apps/frontend-manage/src/components/resources/answerCollections/CollectionDeletionModal.tsx(1 hunks)apps/frontend-manage/src/components/resources/answerCollections/CollectionDeletionSuccessToast.tsx(1 hunks)apps/frontend-manage/src/components/resources/answerCollections/CollectionRemovalErrorToast.tsx(1 hunks)apps/frontend-manage/src/components/resources/answerCollections/CollectionRemovalModal.tsx(1 hunks)apps/frontend-manage/src/components/resources/answerCollections/CollectionRemovalSuccessToast.tsx(1 hunks)apps/frontend-manage/src/components/resources/answerCollections/CollectionSharingRequests.tsx(1 hunks)apps/frontend-manage/src/components/resources/answerCollections/RequestCancellationErrroToast.tsx(1 hunks)apps/frontend-manage/src/components/resources/answerCollections/RequestCancellationModal.tsx(1 hunks)apps/frontend-manage/src/components/resources/answerCollections/RequestCancellationSuccessToast.tsx(1 hunks)cypress/cypress/e2e/D-questions-workflow.cy.ts(6 hunks)cypress/cypress/e2e/K-resources-workflow.cy.ts(17 hunks)packages/graphql/src/graphql/ops/MCancelAnswerCollectionRequest.graphql(1 hunks)packages/graphql/src/graphql/ops/MDeleteAnswerCollection.graphql(1 hunks)packages/graphql/src/graphql/ops/MRemoveAnswerCollection.graphql(1 hunks)packages/graphql/src/graphql/ops/QGetAnswerCollections.graphql(3 hunks)packages/graphql/src/ops.schema.json(4 hunks)packages/graphql/src/ops.ts(15 hunks)packages/graphql/src/public/client.json(4 hunks)packages/graphql/src/public/schema.graphql(4 hunks)packages/graphql/src/public/server.json(4 hunks)packages/graphql/src/schema/mutation.ts(1 hunks)packages/graphql/src/schema/resource.ts(2 hunks)packages/graphql/src/services/questions.ts(3 hunks)packages/graphql/src/services/resources.ts(8 hunks)packages/i18n/messages/de.ts(3 hunks)packages/i18n/messages/en.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/frontend-manage/src/components/resources/answerCollections/CollectionSharingRequests.tsx
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: SonarCloud
- GitHub Check: check
- GitHub Check: cypress-run
- GitHub Check: build
🔇 Additional comments (34)
packages/graphql/src/services/questions.ts (1)
398-406: LGTM! Cache invalidation logic is well-placed.The new event emission ensures proper cache invalidation for answer collections when selection questions are modified.
apps/frontend-manage/src/components/resources/answerCollections/CollectionRemovalErrorToast.tsx (1)
17-18: Verify prop namesopenExternalandonCloseExternalEnsure that the
Toastcomponent usesopenExternalandonCloseExternalas props for controlling the open state externally. If the correct prop names areopenandonClose, please update them accordingly.apps/frontend-manage/src/components/resources/answerCollections/CollectionDeletionErrorToast.tsx (1)
17-18: Verify prop namesopenExternalandonCloseExternalEnsure that the
Toastcomponent usesopenExternalandonCloseExternalas props for controlling the open state externally. If the correct prop names areopenandonClose, please update them accordingly.apps/frontend-manage/src/components/resources/answerCollections/CollectionRemovalSuccessToast.tsx (1)
1-26: Component structure duplicates previous toast implementation.This component could be simplified using the suggested BaseToast component.
apps/frontend-manage/src/components/resources/answerCollections/CollectionDeletionSuccessToast.tsx (1)
1-26: Component structure duplicates previous toast implementations.This component could be simplified using the suggested BaseToast component.
apps/frontend-manage/src/components/resources/answerCollections/AnswerCollectionCollapsible.tsx (1)
8-8: LGTM! Clean implementation of styling customization.The addition of the
classNameprop with optionalrootproperty enhances the component's flexibility while maintaining good TypeScript practices.Also applies to: 12-12, 18-18
apps/frontend-manage/src/components/resources/answerCollections/AnswerCollectionList.tsx (1)
47-52: LGTM! Props are correctly passed down.The state setters are properly passed down to child components, maintaining the unidirectional data flow.
apps/frontend-manage/src/components/resources/SharedAnswerCollectionList.tsx (2)
13-18: LGTM! Props interface follows React patterns.The new props for managing deletion, removal, and cancellation states follow React's best practices using
Dispatch<SetStateAction<boolean>>.Also applies to: 23-28
65-70: Verify error handling in child components.The success/failure state setters are consistently passed to both shared and requested collections. Ensure that the child components (
AnswerCollectionItem) handle these states appropriately and provide meaningful error messages to users.Also applies to: 78-83
✅ Verification successful
Error handling implementation verified and found appropriate
The component properly handles errors through dedicated modal components for each operation (deletion, removal, cancellation), with corresponding error toast components for user feedback. The success/failure callbacks are correctly passed down and utilized in their respective contexts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check error handling in AnswerCollectionItem ast-grep --pattern 'setDeletionFailure($_) { $$$ }'Length of output: 55
Script:
#!/bin/bash # Find AnswerCollectionItem component file fd AnswerCollectionItem # Look for the component definition and its props ast-grep --pattern 'const AnswerCollectionItem = $_' # Backup search using ripgrep rg "AnswerCollectionItem" -A 10Length of output: 8635
Script:
#!/bin/bash # Check the implementation of AnswerCollectionItem cat apps/frontend-manage/src/components/resources/answerCollections/AnswerCollectionItem.tsx # Look for any error message components or strings rg -l "error" apps/frontend-manage/src/components/resources/answerCollections/Length of output: 7888
packages/graphql/src/schema/resource.ts (1)
29-29: LGTM! Schema changes are well-structured.The
isRemovablefield is appropriately:
- Added to the interface as optional
- Exposed in the GraphQL schema as nullable
- Used to control collection removal functionality
Also applies to: 46-46
apps/frontend-manage/src/components/resources/answerCollections/AnswerCollectionViewingModal.tsx (1)
64-96: LGTM! UI implementation is user-friendly and accessible.The removal button implementation:
- Correctly uses the
isRemovableflag for conditional disabling- Provides clear visual feedback with hover states
- Includes an informative tooltip for disabled state
- Uses appropriate icons and colors for destructive actions
cypress/cypress/e2e/K-resources-workflow.cy.ts (2)
Line range hint
1228-1248: LGTM! Test verifies that solution options cannot be deleted.The test correctly verifies that answer options used as solutions are protected from deletion while still being editable.
Line range hint
1421-1439: LGTM! Test verifies cleanup behavior.The test correctly verifies that after deleting a linked question, the previously locked solution options can be deleted again.
packages/graphql/src/schema/mutation.ts (3)
1263-1272: LGTM! Mutation properly secured with user authentication.The
cancelAnswerCollectionRequestmutation is correctly secured withasUserFullAccessauthorization and returns the appropriate type.
1274-1283: LGTM! Mutation properly secured with user authentication.The
removeAnswerCollectionmutation is correctly secured withasUserFullAccessauthorization and returns the appropriate type.
1285-1294: LGTM! Mutation properly secured with user authentication.The
deleteAnswerCollectionmutation is correctly secured withasUserFullAccessauthorization and returns the appropriate type.packages/i18n/messages/de.ts (2)
1870-1870: LGTM! Warning message updated for clarity.The warning message for answer options has been updated to clarify that options may be in use by other users in shared collections.
1943-1970: LGTM! Comprehensive translations added for collection management.New translations have been added to support:
- Collection deletion and removal workflows
- Error messages and confirmations
- Request cancellation functionality
cypress/cypress/e2e/D-questions-workflow.cy.ts (1)
Line range hint
1-1443: No changes to review.This file is provided for context only.
packages/graphql/src/ops.ts (6)
118-118: LGTM! Well-defined type for collection removability.The
isRemovablefield is correctly typed as an optional boolean, maintaining backward compatibility.
1275-1277: LGTM! Consistent argument type definitions.The mutation argument types are well-structured and follow a consistent pattern, each requiring a
collectionIdparameter.Also applies to: 1446-1448, 1825-1827
3109-3114: LGTM! Well-structured mutation variable types.The mutation variable types and their corresponding response types are properly defined following GraphQL codegen conventions.
Also applies to: 3322-3327, 3848-3853
4100-4100: LGTM! Comprehensive query type update.The
GetAnswerCollectionsQuerytype has been consistently updated to include theisRemovablefield across all collection types.
4614-4614: LGTM! Well-defined GraphQL documents.The GraphQL document definitions for the new mutations are properly structured and correctly typed.
Also applies to: 4635-4635, 4698-4698
Line range hint
118-4731: Verify GraphQL schema compatibility.The implementation looks solid. Let's verify that all new types and operations match the GraphQL schema definition.
✅ Verification successful
GraphQL schema compatibility verified
The implementation in
packages/graphql/src/ops.tsis fully compatible with the GraphQL schema. All required types, mutations, and their signatures are properly defined inschema.graphql:
- AnswerCollection type with all necessary fields
- cancelAnswerCollectionRequest mutation
- deleteAnswerCollection mutation
- removeAnswerCollection mutation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the GraphQL operations match the schema. # Test: Search for schema definitions of the new mutations rg -A 5 "type Mutation {" || echo "Mutation type not found in schema" rg -A 5 "input MutationCancelAnswerCollectionRequestArgs" || echo "Cancel args not found in schema" rg -A 5 "input MutationDeleteAnswerCollectionArgs" || echo "Delete args not found in schema" rg -A 5 "input MutationRemoveAnswerCollectionArgs" || echo "Remove args not found in schema" # Test: Search for AnswerCollection type definition rg -A 10 "type AnswerCollection {" || echo "AnswerCollection type not found in schema"Length of output: 823
Script:
#!/bin/bash # Search for AnswerCollection type definition and fields rg -A 10 "type AnswerCollection \{" || echo "AnswerCollection type not found" rg -A 5 "cancelAnswerCollectionRequest.*collectionId:" || echo "Cancel mutation not found" rg -A 5 "deleteAnswerCollection.*collectionId:" || echo "Delete mutation not found" rg -A 5 "removeAnswerCollection.*collectionId:" || echo "Remove mutation not found" # Search for input types and fields rg -A 5 "input.*AnswerCollection" || echo "Input types not found" # Search for mutation definitions rg -A 10 "type Mutation \{" || echo "Mutation type not found"Length of output: 14135
packages/graphql/src/graphql/ops/MDeleteAnswerCollection.graphql (1)
1-3: LGTM!The mutation is well-structured with appropriate naming, argument type, and return type.
packages/graphql/src/graphql/ops/MRemoveAnswerCollection.graphql (1)
1-3: LGTM!The mutation is well-structured with appropriate naming, argument type, and return type.
packages/graphql/src/graphql/ops/MCancelAnswerCollectionRequest.graphql (1)
1-3: LGTM!The mutation is well-structured with appropriate naming, argument type, and return type.
packages/graphql/src/graphql/ops/QGetAnswerCollections.graphql (1)
9-9: LGTM!The
isRemovablefield is consistently added to all collection types, providing necessary information for the UI to manage collection removal actions.Also applies to: 22-22, 34-34
packages/graphql/src/public/client.json (1)
9-9: LGTM!The client configuration is properly updated with hashes for the new mutations and the modified query.
Also applies to: 30-30, 93-93, 126-126
packages/graphql/src/public/schema.graphql (1)
90-90: LGTM! Boolean flag for collection removability.The
isRemovablefield is appropriately typed as Boolean and follows GraphQL naming conventions.packages/graphql/src/public/server.json (2)
9-9: LGTM! Consistent implementation of collection mutations.The mutations are correctly implemented according to the schema definition:
CancelAnswerCollectionRequestDeleteAnswerCollectionRemoveAnswerCollectionEach mutation follows the same pattern and matches its schema definition.
Also applies to: 30-30, 93-93
126-126: Verify the query implementation includes isRemovable field.The
GetAnswerCollectionsquery has been updated to include theisRemovablefield consistently across all collection types.Run the following script to verify the field usage:
✅ Verification successful
The isRemovable field is properly implemented and integrated.
The field is consistently implemented across the schema, backend service, and frontend components, with proper type definitions and usage patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that isRemovable field is used consistently in the codebase. # Test: Search for isRemovable field usage in GraphQL operations rg -A 5 'isRemovable' # Test: Search for isRemovable field in TypeScript interfaces/types rg -t ts 'isRemovable'Length of output: 51505
packages/graphql/src/ops.schema.json (1)
897-908: Verify schema changes impact.Let's verify:
- That existing queries using fragments on
AnswerCollectionwon't break with the newisRemovablefield- That the new mutations are properly secured with authentication and authorization checks
Also applies to: 11716-11744, 13357-13385, 16521-16549
✅ Verification successful
Schema changes verified - no issues found
The addition of the
isRemovablefield is a non-breaking change and all related mutations are properly secured with authentication checks.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential breaking changes and security implications # Check for fragments that might break with the new field rg --type graphql "fragment.*on AnswerCollection" -A 5 # Check for auth checks in resolvers rg --type typescript "cancelAnswerCollectionRequest|deleteAnswerCollection|removeAnswerCollection" -A 5 "src/**/*resolver*.ts"Length of output: 252
Script:
#!/bin/bash # Find and check fragments in both .graphql and .ts files echo "=== Checking for fragments ===" fd -e graphql -e ts | xargs rg "fragment.*on AnswerCollection" -A 5 echo -e "\n=== Checking resolver implementations ===" # Look for resolver implementations fd "resolver" | xargs rg "(cancelAnswerCollectionRequest|deleteAnswerCollection|removeAnswerCollection)" -A 5 echo -e "\n=== Checking potential auth decorators ===" # Look for auth-related decorators near these mutations fd "resolver" | xargs rg "@(Authorized|Auth|RequireAuth).*\n.*(cancelAnswerCollectionRequest|deleteAnswerCollection|removeAnswerCollection)" -B 1 -A 5Length of output: 72596
...frontend-manage/src/components/resources/answerCollections/RequestCancellationErrroToast.tsx
Outdated
Show resolved
Hide resolved
apps/frontend-manage/src/components/resources/answerCollections/CollectionDeletionModal.tsx
Outdated
Show resolved
Hide resolved
apps/frontend-manage/src/components/resources/answerCollections/RequestCancellationModal.tsx
Show resolved
Hide resolved
apps/frontend-manage/src/components/resources/answerCollections/RequestCancellationModal.tsx
Show resolved
Hide resolved
apps/frontend-manage/src/components/resources/answerCollections/AnswerCollectionMetaForm.tsx
Show resolved
Hide resolved
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
answer-collection-deletion
|
| Run status |
|
| Run duration | 16m 18s |
| Commit |
|
| Committer | Julius Schlapbach |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
240
|
| View all changes introduced in this branch ↗︎ | |
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 1509427 | Triggered | Generic Password | ab3ad1d | packages/prisma/src/data/seedUsers.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
apps/frontend-manage/src/components/resources/answerCollections/CollectionDeletionModal.tsx (1)
27-50: 🛠️ Refactor suggestionEnhance cache update logic and error handling.
The mutation implementation could be improved for better maintainability and error handling.
const [deleteAnswerCollection] = useMutation(DeleteAnswerCollectionDocument, { variables: { collectionId: collection.id }, + optimisticResponse: { + deleteAnswerCollection: collection.id, + }, update: (cache, { data }) => { - const res = data?.deleteAnswerCollection - if (res === null || typeof res === 'undefined') return - - const prevQuery = cache.readQuery({ + cache.modify({ query: GetAnswerCollectionsDocument, + fields: { + getAnswerCollections: (existing, { readField }) => ({ + ...existing, + answerCollections: existing.answerCollections.filter( + (c) => readField('id', c) !== data?.deleteAnswerCollection + ), + }), + }, }) - const collections = prevQuery?.getAnswerCollections - if (!collections) return - - cache.writeQuery({ - query: GetAnswerCollectionsDocument, - data: { - getAnswerCollections: { - ...collections, - answerCollections: - collections.answerCollections.filter((c) => c.id !== res) ?? [], - }, - }, - }) }, + onError: (error) => { + console.error('Failed to delete collection:', error) + }, })apps/frontend-manage/src/components/resources/answerCollections/RequestCancellationModal.tsx (1)
70-83: 🛠️ Refactor suggestionEnhance error handling with specific error messages.
The error handling is basic and could be improved to provide more specific feedback to users.
onClick={async () => { const { data, errors } = await cancelAnswerCollectionRequest() + if (errors) { + const errorMessage = errors[0]?.message ?? 'Unknown error occurred' + setCancellationFailure(true) + console.error('Failed to cancel request:', errorMessage) + return + } if ( typeof data?.cancelAnswerCollectionRequest !== 'undefined' && data?.cancelAnswerCollectionRequest !== null && - !errors ) { setCancellationSuccess(true) setCancellationModal(false) } else { setCancellationFailure(true) } }}
🧹 Nitpick comments (6)
apps/frontend-manage/src/components/resources/answerCollections/RequestCancellationErrorToast.tsx (1)
14-22: Consider making toast duration configurable.The duration is hardcoded to 3000ms. Consider making it configurable through props with 3000ms as the default value.
function RequestCancellationErrorToast({ open, onClose, + duration = 3000, }: { open: boolean onClose: () => void + duration?: number }) { // ... <Toast dismissible type="error" openExternal={open} onCloseExternal={onClose} - duration={3000} + duration={duration} >apps/frontend-manage/src/components/resources/answerCollections/CollectionDeletionModal.tsx (1)
64-88: Add ARIA labels for better accessibility.The delete button needs proper ARIA labels for better accessibility.
<Button onClick={async () => { // ... }} className={{ root: 'float-right mt-4 flex flex-row gap-1.5 border border-red-600', }} data={{ cy: 'confirm-delete-answer-collection' }} + aria-label={t('manage.resources.confirmDeletion', { name: collection.name })} >apps/frontend-manage/src/components/resources/AnswerCollections.tsx (3)
21-27: Consider using a reducer for complex state management.The component manages multiple related boolean states for success/failure scenarios. This could be simplified using a reducer pattern to manage these states more efficiently.
Example implementation:
-const [deletionSuccess, setDeletionSuccess] = useState(false) -const [deletionFailure, setDeletionFailure] = useState(false) -const [removalSuccess, setRemovalSuccess] = useState(false) -const [removalFailure, setRemovalFailure] = useState(false) -const [cancellationSuccess, setCancellationSuccess] = useState(false) -const [cancellationFailure, setCancellationFailure] = useState(false) +type ToastState = { + deletion: { success: boolean; failure: boolean }; + removal: { success: boolean; failure: boolean }; + cancellation: { success: boolean; failure: boolean }; +}; + +type ToastAction = { + type: 'SET_STATE'; + category: keyof ToastState; + state: 'success' | 'failure'; + value: boolean; +}; + +const [toastState, dispatch] = useReducer( + (state: ToastState, action: ToastAction) => { + if (action.type === 'SET_STATE') { + return { + ...state, + [action.category]: { + ...state[action.category], + [action.state]: action.value, + }, + }; + } + return state; + }, + { + deletion: { success: false, failure: false }, + removal: { success: false, failure: false }, + cancellation: { success: false, failure: false }, + } +);
40-45: Consider consolidating state setter props using a callback object.Multiple state setter props are being passed down to child components. This could be simplified by passing a single callback object.
Example implementation:
-setDeletionSuccess={setDeletionSuccess} -setDeletionFailure={setDeletionFailure} -setRemovalSuccess={setRemovalSuccess} -setRemovalFailure={setRemovalFailure} -setCancellationSuccess={setCancellationSuccess} -setCancellationFailure={setCancellationFailure} +onStateChange={(category: string, state: string, value: boolean) => { + dispatch({ type: 'SET_STATE', category, state, value }); +}}Also applies to: 51-56
58-80: Consider creating a reusable toast component.Multiple toast components with similar props and behavior are being rendered. This could be simplified using a single reusable component.
Example implementation:
type ToastProps = { type: 'deletion' | 'removal' | 'cancellation'; state: 'success' | 'failure'; open: boolean; onClose: () => void; }; const ActionToast: React.FC<ToastProps> = ({ type, state, open, onClose }) => { const toastMap = { deletion: { success: CollectionDeletionSuccessToast, failure: CollectionDeletionErrorToast, }, removal: { success: CollectionRemovalSuccessToast, failure: CollectionRemovalErrorToast, }, cancellation: { success: RequestCancellationSuccessToast, failure: RequestCancellationErrorToast, }, }; const ToastComponent = toastMap[type][state]; return <ToastComponent open={open} onClose={onClose} />; };packages/graphql/src/services/resources.ts (1)
85-89: Consider extracting common count selection logic.The same count selection logic for linked elements is duplicated across multiple includes. This could be extracted into a reusable selector.
Example implementation:
const linkedElementsCountSelector = (userId: string) => ({ linkedElements: { where: { ownerId: userId, }, }, }); // Usage in includes: _count: { select: linkedElementsCountSelector(ctx.user.sub), }Also applies to: 109-117, 130-138
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/frontend-manage/src/components/resources/AnswerCollections.tsx(2 hunks)apps/frontend-manage/src/components/resources/answerCollections/CollectionDeletionModal.tsx(1 hunks)apps/frontend-manage/src/components/resources/answerCollections/CollectionRemovalModal.tsx(1 hunks)apps/frontend-manage/src/components/resources/answerCollections/RequestCancellationErrorToast.tsx(1 hunks)apps/frontend-manage/src/components/resources/answerCollections/RequestCancellationModal.tsx(1 hunks)cypress/cypress.config.ts(1 hunks)cypress/cypress/e2e/D-questions-workflow.cy.ts(7 hunks)cypress/cypress/support/commands.ts(2 hunks)packages/graphql/src/services/resources.ts(8 hunks)packages/prisma/src/data/constants.ts(1 hunks)packages/prisma/src/data/seedUsers.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: cypress-run
- GitHub Check: check
- GitHub Check: build
🔇 Additional comments (15)
packages/prisma/src/data/constants.ts (1)
5-5: LGTM! Consistent with existing patternsThe new constant follows the established naming convention and maintains UUID uniqueness.
packages/prisma/src/data/seedUsers.ts (3)
7-7: LGTM! Clean import additionThe import is properly placed and follows the existing pattern.
57-66: LGTM! Well-structured test user additionThe new institutional pro user is properly configured with:
- Consistent pattern with existing users
- Unique identifiers (email, shortname)
- Appropriate institutional catalyst flag
56-66: Consider adding test users for comprehensive coverageGiven that this PR adds functionality for managing answer collections and access requests, consider whether additional test users with different permission combinations would be beneficial for thorough testing scenarios.
Would you like me to suggest additional test user configurations that could help cover edge cases in the new functionality?
cypress/cypress.config.ts (1)
17-17: LGTM! The environment variable follows the established pattern.The addition of
LECTURER_INST2_SHORTNAMEwith value 'pro3' maintains consistency with existing lecturer shortname variables.cypress/cypress/support/commands.ts (2)
100-110: LGTM! The new login command follows best practices.The implementation of
loginInstitutionalCatalyst2maintains consistency with existing login commands and properly utilizes theloginFactory.
915-915: LGTM! Type definition is properly declared.The type definition for
loginInstitutionalCatalyst2is correctly added to the global Cypress namespace.cypress/cypress/e2e/D-questions-workflow.cy.ts (6)
Line range hint
1086-1127: LGTM! Comprehensive test coverage for selection question creation.The test steps thoroughly validate:
- Question data entry
- Answer collection selection
- Input number configuration with boundary testing
1128-1131: LGTM! Sample solution validation is properly tested.The test verifies that enabling sample solution without selecting options correctly disables the save button.
Line range hint
1227-1253: LGTM! Proper validation of deletion restrictions.The test cases verify that:
- Options used as solutions cannot be deleted
- Answer collections linked to questions cannot be deleted
1308-1317: LGTM! Verification of answer collection deletion states.The test properly validates that:
- Previous answer collection becomes deletable after unlinking
- Current answer collection remains protected
1429-1447: LGTM! Cleanup verification is thorough.The test ensures that after question deletion:
- All solution options become deletable again
- Edit functionality remains accessible
1449-1455: LGTM! Proper cleanup of test data.The test appropriately cleans up by deleting the shared answer collection after all tests are complete.
apps/frontend-manage/src/components/resources/answerCollections/RequestCancellationErrorToast.tsx (1)
4-10: LGTM! Props interface is well-defined and typed.The component has a clear and typed interface with boolean
openand callbackonCloseprops.apps/frontend-manage/src/components/resources/answerCollections/RequestCancellationModal.tsx (1)
84-93: Add ARIA labels for better accessibility.The button needs proper ARIA labels for better accessibility.
className={{ root: 'float-right mt-4 flex flex-row gap-1.5 border border-red-600', }} data={{ cy: 'confirm-cancel-sharing-request' }} +aria-label={t('manage.resources.confirmCancellation', { name: collection.name })}
apps/frontend-manage/src/components/resources/answerCollections/CollectionRemovalModal.tsx
Outdated
Show resolved
Hide resolved
apps/frontend-manage/src/components/resources/answerCollections/CollectionRemovalModal.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/frontend-manage/src/components/resources/answerCollections/CollectionRemovalModal.tsx (1)
27-53: 🛠️ Refactor suggestionEnhance mutation implementation with optimistic updates and error handling.
The current cache update implementation could be improved for better user experience and maintainability.
const [removeAnswerCollection] = useMutation(RemoveAnswerCollectionDocument, { variables: { collectionId: collection.id }, + optimisticResponse: { + removeAnswerCollection: collection.id, + }, update: (cache, { data }) => { - const res = data?.removeAnswerCollection - if (res === null || typeof res === 'undefined') return - - const prevQuery = cache.readQuery({ + cache.modify({ query: GetAnswerCollectionsDocument, + fields: { + getAnswerCollections: (existing, { readField }) => ({ + ...existing, + answerCollections: existing.answerCollections ?? [], + requestedCollections: existing.requestedCollections ?? [], + sharedCollections: existing.sharedCollections?.filter( + (c) => readField('id', c) !== data?.removeAnswerCollection + ), + }), + }, }) - const collections = prevQuery?.getAnswerCollections - if (!collections) return - - cache.writeQuery({ - query: GetAnswerCollectionsDocument, - data: { - getAnswerCollections: { - ...collections, - answerCollections: collections.answerCollections ?? [], - requestedCollections: collections.requestedCollections ?? [], - sharedCollections: collections.sharedCollections?.filter( - (c) => c.id !== res - ), - }, - }, - }) }, + onError: (error) => { + console.error('Failed to remove collection:', error) + setRemovalFailure(true) + }, })
🧹 Nitpick comments (9)
cypress/cypress/e2e/K-resources-workflow.cy.ts (5)
236-276: Consider adding negative test cases for access level switching.While the current tests verify that access levels can be switched when no other users have access, consider adding test cases to verify:
- Error handling when switching fails
- Edge cases like rapid switching between access levels
605-761: Enhance deletion workflow test coverage.The deletion workflow tests are comprehensive but could be improved by:
- Testing deletion of collections with a large number of items
- Testing concurrent deletion attempts
- Verifying cleanup of associated data
763-824: Address TODO comments and uncomment relevant tests.There are several TODO comments and commented-out tests related to access rule modifications. These should be implemented to ensure complete test coverage of the feature.
Would you like me to help implement these test cases or create GitHub issues to track them?
671-680: Add test cases for concurrent question creation and collection deletion.The current tests verify that collections with linked questions cannot be deleted, but consider adding test cases for:
- Concurrent attempts to create a question and delete its collection
- Race conditions between question creation and collection deletion
Also applies to: 682-688
701-716: Consider parameterizing cleanup tests.The cleanup tests could be made more maintainable by:
- Using a beforeEach/afterEach hook for cleanup
- Creating a reusable cleanup function
- Parameterizing the collections to be cleaned up
Example refactor:
function cleanupCollection(name: string) { cy.get(`[data-cy="answer-collection-${name}"]`).click() cy.get('[data-cy="remove-answer-collection"]').click() cy.get('[data-cy="confirm-remove-answer-collection"]').click() cy.get(`[data-cy="answer-collection-${name}"]`).should('not.exist') } it('Cleanup: Remove all remaining answer collections from user pro1', () => { cy.loginIndividualCatalyst() cy.get('[data-cy="resources"]').click() [publicName, restrictedName].forEach(cleanupCollection) })apps/frontend-manage/src/components/resources/answerCollections/CollectionRemovalModal.tsx (2)
13-25: Consider simplifying modal state management.The modal state management could be simplified by combining
removalModalandsetRemovalModalinto a singleonClosecallback prop, following the controlled component pattern.function CollectionRemovalModal({ collection, - removalModal, - setRemovalModal, + open, + onClose, setRemovalSuccess, setRemovalFailure, }: { collection: AnswerCollection - removalModal: boolean - setRemovalModal: Dispatch<SetStateAction<boolean>> + open: boolean + onClose: () => void setRemovalSuccess: Dispatch<SetStateAction<boolean>> setRemovalFailure: Dispatch<SetStateAction<boolean>> })
67-81: Enhance button interaction with loading state and prevent double submission.The button click handler could be improved to handle loading states and prevent multiple submissions while the mutation is in progress.
+ const [isSubmitting, setIsSubmitting] = useState(false) + <Button + disabled={isSubmitting} onClick={async () => { + setIsSubmitting(true) const { data, errors } = await removeAnswerCollection() if ( typeof data?.removeAnswerCollection !== 'undefined' && data?.removeAnswerCollection !== null && !errors ) { setRemovalSuccess(true) - setRemovalModal(false) + onClose() } else { setRemovalFailure(true) } + setIsSubmitting(false) }} + loading={isSubmitting}apps/frontend-manage/src/components/resources/answerCollections/AnswerCollectionItem.tsx (2)
23-28: Consider consolidating success/failure states.The component has multiple success/failure state setters that follow a similar pattern. Consider consolidating these into a single state object for better maintainability.
- setDeletionSuccess: Dispatch<SetStateAction<boolean>> - setDeletionFailure: Dispatch<SetStateAction<boolean>> - setRemovalSuccess: Dispatch<SetStateAction<boolean>> - setRemovalFailure: Dispatch<SetStateAction<boolean>> - setCancellationSuccess: Dispatch<SetStateAction<boolean>> - setCancellationFailure: Dispatch<SetStateAction<boolean>> + setActionState: Dispatch<SetStateAction<{ + deletion: { success: boolean; failure: boolean }; + removal: { success: boolean; failure: boolean }; + cancellation: { success: boolean; failure: boolean }; + }>>Also applies to: 33-38
148-190: Consider simplifying fragment usage.The code uses multiple fragments (
<>...</>) for conditional rendering. Consider simplifying this structure:- {editable ? ( - <> - <AnswerCollectionEditModal - collection={collection} - open={editModal} - onClose={() => setEditModal(false)} - onDelete={() => setDeletionModal(true)} - /> - <CollectionDeletionModal - collection={collection} - deletionModal={deletionModal} - setDeletionModal={setDeletionModal} - setDeletionSuccess={setDeletionSuccess} - setDeletionFailure={setDeletionFailure} - /> - </> - ) : null} - {accessGranted ? ( - <> - <AnswerCollectionViewingModal - /* ... props ... */ - /> - <CollectionRemovalModal - /* ... props ... */ - /> - </> - ) : ( - <> - <RequestCancellationModal - /* ... props ... */ - /> - </> - )} + {editable && ( + <div> + <AnswerCollectionEditModal /* ... props ... */ /> + <CollectionDeletionModal /* ... props ... */ /> + </div> + )} + {accessGranted ? ( + <div> + <AnswerCollectionViewingModal /* ... props ... */ /> + <CollectionRemovalModal /* ... props ... */ /> + </div> + ) : ( + <RequestCancellationModal /* ... props ... */ /> + )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/frontend-manage/src/components/resources/answerCollections/AnswerCollectionItem.tsx(4 hunks)apps/frontend-manage/src/components/resources/answerCollections/CollectionRemovalModal.tsx(1 hunks)cypress/cypress/e2e/K-resources-workflow.cy.ts(15 hunks)packages/i18n/messages/de.ts(4 hunks)packages/i18n/messages/en.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/i18n/messages/de.ts
- packages/i18n/messages/en.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Analyze (javascript)
- GitHub Check: cypress-run
- GitHub Check: SonarCloud
- GitHub Check: build
- GitHub Check: check
- GitHub Check: build
🔇 Additional comments (3)
cypress/cypress/e2e/K-resources-workflow.cy.ts (1)
3-11: LGTM! Constants are well-organized and descriptive.The constants follow a consistent naming pattern and provide clear descriptions for different types of answer collections (public, restricted, private).
apps/frontend-manage/src/components/resources/answerCollections/AnswerCollectionItem.tsx (2)
42-47: LGTM! Clean state management implementation.The modal states are well-organized and follow React best practices.
80-86: LGTM! Clear and comprehensive click handler logic.The click handler effectively manages all three cases (edit, view, cancel) with clear conditional logic.
…on switch to public
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
cypress/cypress/e2e/K-resources-workflow.cy.ts (4)
236-276: Consider adding more assertions for access level switching tests.While the tests verify the UI interactions for switching access levels, they could be enhanced with:
- Assertions to verify the actual state change after switching
- Verification of any success/error messages
- Checks for persistence after page reload
it('Verify that the public answer collection can be switched to private if no other users have access', () => { cy.loginLecturer() cy.get('[data-cy="resources"]').click() cy.get(`[data-cy="answer-collection-${publicName}"]`).click() cy.get('[data-cy="answer-collection-access"]').contains( messages.manage.resources.accessPUBLIC ) cy.get('[data-cy="answer-collection-access"]').click() cy.get('[data-cy="answer-collection-access-restricted"]').should( 'not.have.css', 'pointer-events', 'none' ) cy.get('[data-cy="answer-collection-access-private"]').should( 'not.have.css', 'pointer-events', 'none' ) cy.get('[data-cy="answer-collection-access-public"]').click() + // Verify the state change + cy.get('[data-cy="answer-collection-access"]').contains( + messages.manage.resources.accessPUBLIC + ) + // Verify persistence + cy.reload() + cy.get(`[data-cy="answer-collection-${publicName}"]`).click() + cy.get('[data-cy="answer-collection-access"]').contains( + messages.manage.resources.accessPUBLIC + ) })
605-761: Consider adding error case tests for deletion workflows.While the happy path is well covered, consider adding tests for:
- Attempting to delete a collection that's in use
- Network failure during deletion
- Concurrent deletion attempts
// #region +it('Should handle errors when deleting an answer collection', () => { + cy.loginLecturer() + cy.get('[data-cy="resources"]').click() + + // Test network failure + cy.intercept('POST', '**/graphql', { + statusCode: 500, + body: { errors: [{ message: 'Internal Server Error' }] } + }).as('deleteFailure') + + cy.get(`[data-cy="answer-collection-${publicName}"]`).click() + cy.get('[data-cy="delete-answer-collection"]').click() + cy.get('[data-cy="confirm-delete-answer-collection"]').click() + + // Verify error handling + cy.get('[data-cy="error-message"]').should('be.visible') +})
763-836: TODO comments indicate incomplete test coverage for access rule modifications.The TODO section outlines important test scenarios for access rule modifications that need to be implemented. These tests are crucial for ensuring the robustness of the access control system.
Would you like me to help implement these test cases? I can generate the test implementations following the same patterns used in the existing tests.
Line range hint
65-836: Well-organized test suite with clear sections.The test suite is well-structured with clear sections for:
- Creation and editing workflows
- Sharing workflows
- Deletion workflows
- Access rule modifications (planned)
Consider extracting common test setup into custom Cypress commands to reduce duplication.
For example:
// commands.ts Cypress.Commands.add('verifyCollectionAccess', (name: string, access: string) => { cy.get(`[data-cy="answer-collection-${name}"]`).click() cy.get('[data-cy="viewing-collection-title"]').contains(name) cy.get('[data-cy="viewing-collection-access"]').contains(access) })packages/graphql/src/services/resources.ts (1)
199-199: Consider wrapping access changes in a transaction.While the logic for handling the transition from restricted to public access is correct, the multiple database operations (connecting users and clearing requests) should be atomic.
- const updatedCollection = await ctx.prisma.answerCollection.update({ + const updatedCollection = await ctx.prisma.$transaction(async (tx) => { + return await tx.answerCollection.update({ where: { id, ownerId: ctx.user.sub, }, data: { name: name ?? undefined, access: access ?? undefined, description: description ?? undefined, version: { increment: 1, }, accessGranted: { connect: restrictedToPublic ? collection.accessRequested.map((request) => ({ id: request.id, })) : undefined, }, accessRequested: { set: restrictedToPublic ? [] : undefined, }, }, include: { entries: true, }, - }) + }) + })Also applies to: 222-247
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cypress/cypress/e2e/K-resources-workflow.cy.ts(15 hunks)packages/graphql/src/services/resources.ts(11 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: SonarCloud
- GitHub Check: build
- GitHub Check: check
- GitHub Check: build
- GitHub Check: cypress-run
- GitHub Check: Analyze (javascript)
🔇 Additional comments (8)
cypress/cypress/e2e/K-resources-workflow.cy.ts (2)
Line range hint
3-62: LGTM! Well-structured test data setup.The constants are well-organized with clear naming conventions and descriptive values, making the test data easy to understand and maintain.
Line range hint
279-603: LGTM! Comprehensive sharing workflow test coverage.The tests thoroughly cover all aspects of sharing workflows including:
- Access request flows
- Approval/denial scenarios
- Visibility verification
- Permission checks
packages/graphql/src/services/resources.ts (6)
85-89: Well-structured implementation of linked elements tracking!The consistent implementation of
linkedElementscounting across all collection types (owned, shared, and requested) with proper owner filtering provides a robust foundation for the collection removal functionality.Also applies to: 109-117, 130-138, 159-159, 164-164, 169-169
260-324: Enhance transaction safety in deleteAnswerCollection.The owner disconnection and request clearing operations should be atomic to maintain data consistency.
- updatedCollection = await ctx.prisma.answerCollection.update({ + updatedCollection = await ctx.prisma.$transaction(async (tx) => { + return await tx.answerCollection.update({ where: { id: collectionId, }, data: { owner: { disconnect: true, }, accessRequested: { set: [], }, }, - }) + }) + })Enhance error handling with specific error types.
326-388: Add validation for collection ownership.
524-525: Good data integrity check!The additional filter preventing access to collections without owners is a good safeguard against potential data inconsistencies.
592-594: Consistent data integrity check!The ownerId validation aligns well with the collection selection changes, maintaining consistent access control.
647-681: Add validation for request existence.
sjschlapbach
changed the title
|
Summary by CodeRabbit
New Features
isRemovablefield to answer collections to indicate removability.Bug Fixes
Documentation
Chores