feat: allow deletion of all activities independent of status and distinguish between hard and soft deletion in backend #4294
Conversation
…inguish between hard and soft deletion in backend
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.
|
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces a series of changes across various components and services related to course management. Key modifications include the implementation of deletion functionality through new Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🔇 Additional comments (4)
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)
packages/prisma/src/prisma/migrations/20241004214633_activity_soft_deletion_booleans/migration.sql (4)
1-2: LGTM! Consider adding an index for performance.The addition of the
isDeletedcolumn to the GroupActivity table is correct and aligns with the PR objectives for implementing soft deletion. The BOOLEAN type, NOT NULL constraint, and default value of false are appropriate for this use case.Consider adding an index on the
isDeletedcolumn to optimize queries that filter on this column:CREATE INDEX idx_group_activity_is_deleted ON "GroupActivity" ("isDeleted");This can improve performance when querying for non-deleted or deleted group activities.
4-5: LGTM! Consider adding an index for performance.The addition of the
isDeletedcolumn to the MicroLearning table is correct and consistent with the implementation for the GroupActivity table. It aligns with the PR objectives for implementing soft deletion.Consider adding an index on the
isDeletedcolumn to optimize queries that filter on this column:CREATE INDEX idx_micro_learning_is_deleted ON "MicroLearning" ("isDeleted");This can improve performance when querying for non-deleted or deleted microlearnings.
7-8: LGTM! Consider adding an index for performance.The addition of the
isDeletedcolumn to the PracticeQuiz table is correct and consistent with the implementations for the GroupActivity and MicroLearning tables. It aligns with the PR objectives for implementing soft deletion.Consider adding an index on the
isDeletedcolumn to optimize queries that filter on this column:CREATE INDEX idx_practice_quiz_is_deleted ON "PracticeQuiz" ("isDeleted");This can improve performance when querying for non-deleted or deleted practice quizzes.
1-8: Overall implementation looks good. Consider application logic implications.The migration script successfully adds the
isDeletedcolumn to all three tables (GroupActivity, MicroLearning, and PracticeQuiz) consistently. This implementation aligns well with the PR objectives for implementing soft deletion.Consider the following points for the application logic:
- Ensure that all queries in the application are updated to include a WHERE clause filtering out soft-deleted records (e.g.,
WHERE isDeleted = false) unless explicitly querying for deleted items.- Implement a mechanism to periodically clean up soft-deleted records if necessary.
- Update any unique constraints or indexes that should not consider soft-deleted records.
- Consider implementing an audit trail for deletion operations to track when and by whom items were soft-deleted.
These considerations will help ensure that the soft deletion functionality is fully integrated into the application's behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/graphql/src/services/courses.ts (6 hunks)
- packages/prisma/src/prisma/migrations/20241004214633_activity_soft_deletion_booleans/migration.sql (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/graphql/src/services/courses.ts
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
soft-deletion-activities
|
| Run status |
|
| Run duration | 10m 16s |
| Commit |
|
| Committer | Julius Schlapbach |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
1
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
47
|
| View all changes introduced in this branch ↗︎ | |
|
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (5)
cypress/cypress/e2e/K-group-activity-workflow.cy.ts (2)
309-319: New test case for deleting a group activity looks good, but consider adding assertions.The new test case successfully demonstrates the deletion of a group activity after it has been created and published. This aligns well with the PR objectives of enhancing deletion functionalities. However, there are a few suggestions to improve the test:
- Consider adding an assertion to verify the success message after deletion.
- It might be beneficial to check if the activity is truly gone by attempting to find it again, expecting a failure.
Here's a suggested improvement:
cy.get(`[data-cy="delete-groupActivity-${newName}"]`).click() cy.get(`[data-cy="confirm-delete-groupActivity"]`).click() + cy.get('[data-cy="success-message"]').should('contain', 'Group activity deleted successfully') cy.get(`[data-cy="groupActivity-actions-${newName}"]`).should('not.exist') + cy.get(`[data-cy="groupActivity-${newName}"]`).should('not.exist')These additions will make the test more robust by ensuring the deletion was successful and the activity is no longer visible in the UI.
Line range hint
1-819: Overall test structure is solid, but consider enhancing error handling and consistency.The test file provides comprehensive coverage for group activity workflows, including the new deletion functionality. However, there are a few general suggestions to improve the overall quality of the test suite:
Error Handling: Consider adding more robust error handling throughout the tests. For example, use
cy.on('uncaught:exception', ...)to handle any unexpected errors.Consistency: Ensure consistent naming conventions for data-cy attributes across all tests. This will make maintenance easier in the future.
Reusability: Some common operations (like logging in, navigating to specific pages) are repeated across tests. Consider extracting these into custom Cypress commands to reduce duplication.
Cleanup: After each test, especially the new deletion test, consider adding cleanup steps to ensure a consistent state for subsequent tests.
Here's an example of how you could implement a custom command for login:
// In cypress/support/commands.js Cypress.Commands.add('loginUser', (username, password) => { cy.visit(Cypress.env('URL_STUDENT')) cy.get('[data-cy="username-field"]').type(username) cy.get('[data-cy="password-field"]').type(password) cy.get('[data-cy="submit-login"]').click() cy.wait(1000) // Consider using a more robust wait strategy }) // In your test file it('can send a message to the group', function () { cy.loginUser(Cypress.env('STUDENT_USERNAME'), Cypress.env('STUDENT_PASSWORD')) // Rest of the test... })Implementing these suggestions will improve the maintainability and robustness of your test suite.
cypress/cypress/e2e/H-practice-quiz-workflow.cy.ts (3)
209-236: LGTM: Deletion and verification steps added.The new steps to delete the practice quiz and verify its inaccessibility are well-implemented and align with the PR objectives. The verification process is thorough, checking both the lecturer's view and the student's access.
Consider adding a comment before line 223 to clearly separate the lecturer actions from the student verification steps. This would improve readability:
cy.get(`[data-cy="practice-quiz-actions-${practiceQuizName}"]`).should( 'not.exist' ) +// Verify that the deleted quiz is no longer accessible to students // make sure that the practice quiz is no longer accessible to students cy.clearAllCookies() cy.visit(Cypress.env('URL_STUDENT'))
439-465: LGTM: Consistent implementation of deletion and verification steps.This segment correctly implements the deletion and verification steps for another test case, maintaining consistency with the previous implementation. This consistency is good for code maintainability and readability.
As suggested in the previous comment, consider adding a separating comment before the student verification steps (before line 452) to improve readability.
660-686: LGTM: Consistent implementation of deletion and verification steps.This segment correctly implements the deletion and verification steps for the third test case, maintaining consistency with the previous implementations.
Consider refactoring these repeated deletion and verification steps into a custom Cypress command to adhere to the DRY (Don't Repeat Yourself) principle. This would improve maintainability and reduce the chance of inconsistencies in future updates. For example:
// In commands.js Cypress.Commands.add('deletePracticeQuizAndVerify', (quizName, quizDisplayName, courseName) => { // Lecturer deletion steps cy.loginLecturer() cy.get('[data-cy="courses"]').click() cy.findByText(courseName).click() cy.get('[data-cy="tab-practiceQuizzes"]').click() cy.get(`[data-cy="practice-quiz-actions-${quizName}"]`).click() cy.get(`[data-cy="delete-practice-quiz-${quizName}"]`).click() cy.get(`[data-cy="confirm-delete-practice-quiz"]`).click() cy.get(`[data-cy="practice-quiz-actions-${quizName}"]`).should('not.exist') // Student verification steps cy.loginStudent() cy.get('[data-cy="quizzes"]').click() cy.get(`[data-cy="practice-quiz-${quizDisplayName}"]`).should('not.exist') }) // In the test file cy.deletePracticeQuizAndVerify(practiceQuizName2, practiceQuizDisplayName2, courseName)This refactoring would significantly reduce code duplication across your test cases.
🛑 Comments failed to post (6)
cypress/cypress/e2e/G-microlearning-workflow.cy.ts (6)
327-337: 🛠️ Refactor suggestion
Refactor microlearning deletion steps into a custom Cypress command
The steps to delete a microlearning as a lecturer are repeated across multiple tests. To improve maintainability and reduce code duplication, consider creating a custom Cypress command or helper function for this process.
For example, you can add the following custom command in your
cypress/support/commands.js:Cypress.Commands.add('deleteMicrolearning', (microlearningName) => { cy.clearAllCookies() cy.loginLecturer() cy.get('[data-cy="courses"]').click() cy.findByText(courseName).click() cy.get('[data-cy="tab-microLearnings"]').click() cy.get(`[data-cy="microlearning-actions-${microlearningName}"]`).click() cy.get(`[data-cy="delete-microlearning-${microlearningName}"]`).click() cy.get('[data-cy="confirm-delete-microlearning"]').click() cy.get(`[data-cy="microlearning-actions-${microlearningName}"]`).should('not.exist') })Then, in your test file, you can simply call:
cy.deleteMicrolearning(microLearningName)
340-350: 🛠️ Refactor suggestion
Refactor student login steps into a custom Cypress command
The steps to log in as a student are repeated across multiple tests. Consider creating a custom Cypress command to encapsulate this logic, enhancing maintainability and reducing code duplication.
For example, add the following to your
cypress/support/commands.js:Cypress.Commands.add('loginStudent', () => { cy.clearAllCookies() cy.visit(Cypress.env('URL_STUDENT')) cy.get('[data-cy="username-field"]') .click() .type(Cypress.env('STUDENT_USERNAME')) cy.get('[data-cy="password-field"]') .click() .type(Cypress.env('STUDENT_PASSWORD')) cy.get('[data-cy="submit-login"]').click() })Then, replace the login steps in your test with:
cy.loginStudent()
464-470: 🛠️ Refactor suggestion
Refactor microlearning deletion steps into a custom Cypress command
The deletion steps here duplicate code used elsewhere. Consider using the
cy.deleteMicrolearning()command to streamline your tests.
571-582: 🛠️ Refactor suggestion
Refactor microlearning deletion steps into a custom Cypress command
Again, you're repeating the deletion logic. Using a custom command like
cy.deleteMicrolearning(microLearningName)can improve code readability.
854-880: 🛠️ Refactor suggestion
Refactor microlearning deletion and student login into custom commands
The code for deleting a microlearning and logging in as a student is repeated. To enhance maintainability, consider using the
cy.deleteMicrolearning()andcy.loginStudent()custom commands.
1429-1454: 🛠️ Refactor suggestion
Refactor microlearning deletion and student login into custom commands
The deletion and student login steps are duplicated here. Utilizing custom commands will reduce redundancy and ease future maintenance.
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
v3
|
| Run status |
|
| Run duration | 09m 48s |
| Commit |
|
| Committer | Julius Schlapbach |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
Failures
|
0
|
|
Flaky
|
2
|
|
Pending
|
0
|
|
Skipped
|
0
|
|
Passing
|
47
|
| View all changes introduced in this branch ↗︎ | |
This pull requests adds the following functionalities - potentially more detailed deletion modals with specific required confirmations follow on separate PRs for better readability:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation