Assessment: Fix an issue where grading criteria are not applied correctly on drag and drop
#10127
Conversation
chrisknedl
added
enhancement
tests
client
github-actions
bot
removed
the
assessment
chrisknedl
added
the
assessment
github-actions
bot
added
ready for review
and removed
assessment
chrisknedl
added
the
assessment
WalkthroughThe pull request introduces changes to enhance the drag-and-drop functionality for structured assessment criteria within the feedback management system. Key modifications include the addition of Changes
Assessment against linked issues
Possibly related PRs
Suggested Labels
Suggested Reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
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
Documentation and Community
|
![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 comments (1)
src/main/webapp/app/exercises/shared/unreferenced-feedback/unreferenced-feedback.component.ts (1)
Line range hint
134-142: Add event propagation handling to prevent duplicate events.According to the PR objectives, the issue involves event bubbling causing duplicate grading criteria. Add
event.stopPropagation()to prevent the event from bubbling up and triggering multiple handlers.createAssessmentOnDrop(event: Event) { + event.stopPropagation(); this.addUnreferencedFeedback(); const newFeedback: Feedback | undefined = this.unreferencedFeedback.last(); if (newFeedback) { this.structuredGradingCriterionService.updateFeedbackWithStructuredGradingInstructionEvent(newFeedback, event); this.updateFeedback(newFeedback); } }
🧹 Nitpick comments (5)
src/main/webapp/app/exercises/shared/unreferenced-feedback/unreferenced-feedback.component.ts (4)
Line range hint
44-58: LGTM with minor suggestions for improvement.The validation logic is well-structured with clear documentation. Consider these enhancements:
- Extract
undefinedcheck into a type guard- Use
number.isFinite()for more precise numeric validationvalidateFeedback() { if (!this.unreferencedFeedback || this.unreferencedFeedback.length === 0) { this.assessmentsAreValid = false; return; } for (const feedback of this.unreferencedFeedback) { - if (feedback.credits == undefined || isNaN(feedback.credits)) { + if (!this.isValidCredits(feedback.credits)) { this.assessmentsAreValid = false; return; } } this.assessmentsAreValid = true; } +private isValidCredits(credits: number | undefined): credits is number { + return credits !== undefined && Number.isFinite(credits); +}
Line range hint
35-44: Consider emitting changes after validation.The event emission should occur after validation to ensure subscribers receive a valid state.
public deleteFeedback(feedbackToDelete: Feedback): void { const indexToDelete = this.unreferencedFeedback.indexOf(feedbackToDelete); this.unreferencedFeedback.splice(indexToDelete, 1); - this.feedbacksChange.emit(this.unreferencedFeedback); this.validateFeedback(); + this.feedbacksChange.emit(this.unreferencedFeedback); }
Line range hint
63-74: Consider emitting changes after validation.Similar to
deleteFeedback, move the event emission after validation to ensure subscribers receive a valid state.updateFeedback(feedback: Feedback) { const indexToUpdate = this.unreferencedFeedback.indexOf(feedback); if (indexToUpdate < 0) { this.unreferencedFeedback.push(feedback); } else { this.unreferencedFeedback[indexToUpdate] = feedback; } this.validateFeedback(); - this.feedbacksChange.emit(this.unreferencedFeedback); + if (this.assessmentsAreValid) { + this.feedbacksChange.emit(this.unreferencedFeedback); + } }
Line range hint
1-142: Consider implementing OnDestroy for proper cleanup.To prevent potential memory leaks, implement the
OnDestroyinterface to clean up event listeners and subscriptions.-export class UnreferencedFeedbackComponent { +export class UnreferencedFeedbackComponent implements OnDestroy { + ngOnDestroy(): void { + // Clean up any subscriptions or event listeners + this.feedbacksChange.complete(); + this.onAcceptSuggestion.complete(); + this.onDiscardSuggestion.complete(); + }src/test/javascript/spec/helpers/stubs/unreferenced-feedback-detail-stub.component.ts (1)
9-19: Consider adding type safety for boolean inputs.While the implementation is correct, consider initializing boolean inputs with default values for better type safety.
- @Input() isSuggestion: boolean; - @Input() public readOnly: boolean; - @Input() highlightDifferences: boolean; - @Input() useDefaultFeedbackSuggestionBadgeText: boolean; + @Input() isSuggestion = false; + @Input() public readOnly = false; + @Input() highlightDifferences = false; + @Input() useDefaultFeedbackSuggestionBadgeText = false;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/webapp/app/assessment/unreferenced-feedback-detail/unreferenced-feedback-detail.component.ts(1 hunks)src/main/webapp/app/exercises/shared/unreferenced-feedback/unreferenced-feedback.component.ts(1 hunks)src/test/javascript/spec/component/shared/unreferenced-feedback.component.spec.ts(4 hunks)src/test/javascript/spec/helpers/stubs/loading-indicator-container-stub.component.ts(1 hunks)src/test/javascript/spec/helpers/stubs/unreferenced-feedback-detail-stub.component.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/test/javascript/spec/helpers/stubs/loading-indicator-container-stub.component.ts
🧰 Additional context used
📓 Path-based instructions (4)
src/main/webapp/app/assessment/unreferenced-feedback-detail/unreferenced-feedback-detail.component.ts (1)
src/main/webapp/app/exercises/shared/unreferenced-feedback/unreferenced-feedback.component.ts (1)
src/test/javascript/spec/component/shared/unreferenced-feedback.component.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/helpers/stubs/unreferenced-feedback-detail-stub.component.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: client-tests-selected
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Build and Push Docker Image
- GitHub Check: Analyse
- GitHub Check: Mend Security Check
🔇 Additional comments (6)
src/test/javascript/spec/helpers/stubs/unreferenced-feedback-detail-stub.component.ts (2)
4-8: LGTM! Well-structured stub component.The component follows Angular style guidelines with proper selector naming and minimal template implementation.
21-24: LGTM! Correct event handling implementation.The
updateFeedbackOnDropmethod correctly stops event propagation, matching the behavior of the actual component.src/main/webapp/app/assessment/unreferenced-feedback-detail/unreferenced-feedback-detail.component.ts (1)
79-82: LGTM! Correct fix for event bubbling.The addition of
event.stopPropagation()properly addresses the issue of duplicate grading criteria creation by preventing the drop event from bubbling up to parent components.Let's verify there are no other drop handlers in parent components that might be affected:
✅ Verification successful
Verified: Event propagation stop is safe and correct
The
event.stopPropagation()addition is safe because:
- The drop handler exists only at the root level of the component
- No other components in the assessment directory rely on this drop event
- No nested handlers within the component are affected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other drop event handlers in parent components rg -l '\(drop\)=' src/main/webapp/app/assessment/Length of output: 155
Script:
#!/bin/bash # Check the template file content cat src/main/webapp/app/assessment/unreferenced-feedback-detail/unreferenced-feedback-detail.component.htmlLength of output: 5416
src/test/javascript/spec/component/shared/unreferenced-feedback.component.spec.ts (3)
19-19: LGTM! Proper test setup with stub component.The test module correctly uses the new stub component instead of MockComponent, providing better control over the test scenarios.
91-95: LGTM! Clear and focused mock implementation.The mock implementation directly updates the feedback object, making the test behavior clear and maintainable.
119-136: LGTM! Comprehensive test for event propagation.The test case thoroughly verifies that:
- The drop event is handled by the detail component
- Event propagation is stopped
- No duplicate feedback is created
Third time's the charm. If github-actions decides one more time that the |
chrisknedl
changed the title
Assessment: Fix an issue where grading criteria were not created correctly on drag and dropAssessment: Fix an issue where grading criteria are not created correctly on drag and drop
vinceclifford
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
github-actions
bot
removed
the
assessment
sawys777
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
chrisknedl
dismissed stale reviews from b-fein, ole-ve, coderabbitai[bot], and sawys777
via
ffaeb5c
Checklist
General
Client
Motivation and Context
Closes #9979.
Description
The current issue actually stems from another bugfix that solved a problem where a drag and drop of the assessment criteria caused duplicates (#8908). The problem was that, due to event bubbling, the drop event was triggered twice when a new grading criteria was dropped on an existing unreferenced feedback item, namely in the
unreferenced-feedback.component.htmlandunreferenced-feedback-detail.component.htmlfiles. The solution is to callevent.stopPropagation()on the sub-component to make sure that only the intended event handler is executed.Steps for Testing
Prerequisites:
File upload exercises: Drag and Drop feedback duplicates when applied to additional feedback #8908 andAssessment: Cannot drag and drop structured assessment criteria under certain circumstances #9979 can't be replicated anymoreTestserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Code Review
Manual Tests
Test Coverage
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Refactor
Tests