Iris: Allow users to resolve memory conflicts
#12039
Conversation
github-actions
bot
added
tests
server
|
@Hialus Test coverage could not be fully measured because some tests failed. Please check the workflow logs for details. |
…olution' into feature/iris/memory-conflict-resolution
|
@Hialus Test coverage has been automatically updated in the PR description. |
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
|
@Hialus Test coverage has been automatically updated in the PR description. |
|
@Hialus Test coverage has been automatically updated in the PR description. |
WalkthroughThis change introduces a new aggregated Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Frontend as Learner Profile<br/>(UI)
participant ListComp as MemirisMemoriesListComponent<br/>(Signal-based)
participant HttpService as IrisMemoriesHttpService
participant Backend as IrisMemoryResource<br/>(REST)
participant PyrisService as PyrisConnectorService
participant Modal as ResolveMemoriesConflictsModal
User->>Frontend: Load learner profile
Frontend->>ListComp: loadMemories()
ListComp->>HttpService: getUserMemoryData()
HttpService->>Backend: GET /api/iris/user/memory-data
Backend->>PyrisService: listMemirisMemoryData()
PyrisService-->>Backend: MemirisMemoryDataDTO
Backend-->>HttpService: MemirisMemoryDataDTO
HttpService-->>ListComp: Observable<MemirisMemoryDataDTO>
ListComp->>ListComp: Store in memoryData signal<br/>Calculate conflictGroups
alt Conflicts Detected
ListComp->>Modal: openResolveConflictsModal()
Modal->>Modal: Initialize groups & currentIndex
User->>Modal: Browse conflicts
Modal->>Modal: prev()/next() navigation
User->>Modal: keep(groupIndex, selectedId)
Modal->>HttpService: deleteUserMemory(id) for non-selected
HttpService->>Backend: DELETE /api/iris/user/memory/{id}
Backend-->>HttpService: OK
Modal->>ListComp: resolve with deletedIds[]
ListComp->>ListComp: applyDeletions() to memoryData
else No Conflicts
Frontend->>Frontend: Render memories directly
end
User->>Frontend: Click memory for details
ListComp->>ListComp: toggleOpen(memory)
ListComp->>HttpService: getUserMemory(memoryId)
HttpService->>Backend: GET /api/iris/user/memory/{memoryId}
Backend-->>HttpService: MemirisMemoryWithRelationsDTO
ListComp->>ListComp: buildDetails() & cache result
Frontend-->>Frontend: Render via<br/>MemirisMemoryDetailsComponent
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisConnectorService.java (1)
83-99: Normalize potentially null lists in the aggregated response.If Pyris returns any null lists, the API contract (arrays) can break client assumptions. Consider normalizing to empty lists before returning the DTO. Based on learnings, please sanitize upstream inputs.
🛠️ Suggested defensive normalization
- return response.getBody(); + var body = response.getBody(); + return new MemirisMemoryDataDTO( + Optional.ofNullable(body.memories()).orElseGet(Collections::emptyList), + Optional.ofNullable(body.learnings()).orElseGet(Collections::emptyList), + Optional.ofNullable(body.connections()).orElseGet(Collections::emptyList) + );src/main/webapp/app/core/user/settings/learner-profile/insights-learner-profile/iris-learner-profile/memiris-memories-list.component.spec.ts (1)
49-98: Add tests for conflict resolution functionality in the main component spec.The test file covers memory loading, details expansion, and deletion, but lacks coverage for key conflict-resolution features introduced in this PR:
conflictGroupscomputed signal (conflict detection and grouping)hasConflictscomputed signalopenResolveConflictsModal()method and its integration with modal result handlingapplyDeletions()behavior beyond the deleteMemory contextWhile a separate
resolve-memories-conflicts-modal.component.spec.tsexists for the modal component, the main component's integration with conflict detection and modal-driven deletions is not tested.
🧹 Nitpick comments (6)
src/main/webapp/app/core/user/settings/learner-profile/insights-learner-profile/iris-learner-profile/resolve-memories-conflicts-modal.component.spec.ts (2)
40-44: Consider usingmockClear()for consistent mock resets.Reassigning
jest.fn()to reset mocks works but is unconventional. UsingmockClear()ormockReset()is more idiomatic and preserves mock implementation if needed.♻️ Suggested improvement
afterEach(() => { jest.restoreAllMocks(); - activeModalMock.close = jest.fn(); - activeModalMock.dismiss = jest.fn(); + (activeModalMock.close as jest.Mock).mockClear(); + (activeModalMock.dismiss as jest.Mock).mockClear(); });
53-63: Consider adding boundary condition tests for navigation.The test verifies forward/backward navigation but doesn't verify behavior at boundaries (e.g., calling
prev()at index 0 ornext()at the last index). Adding these assertions would ensure the component handles edge cases correctly.♻️ Suggested additions
it('navigates with next and prev within bounds', () => { component.conflictGroups = [['a'], ['b'], ['c']]; component.ngOnInit(); expect(component.currentIndex()).toBe(0); + // Verify prev() at start doesn't go negative + component.prev(); + expect(component.currentIndex()).toBe(0); component.next(); expect(component.currentIndex()).toBe(1); component.next(); expect(component.currentIndex()).toBe(2); + // Verify next() at end doesn't exceed bounds + component.next(); + expect(component.currentIndex()).toBe(2); component.prev(); expect(component.currentIndex()).toBe(1); });src/main/webapp/app/core/user/settings/learner-profile/insights-learner-profile/iris-learner-profile/memiris-memory-details.component.ts (1)
13-13: Remove unnecessaryCommonModuleimport.The template uses only modern Angular control flow syntax (
@if,@for) and theTranslateDirective. No CommonModule-dependent features (pipes, legacy structural directives) are present, making the import unnecessary for this standalone component.♻️ Suggested change
`@Component`({ selector: 'jhi-memiris-memory-details', standalone: true, - imports: [CommonModule, TranslateDirective], + imports: [TranslateDirective], templateUrl: './memiris-memory-details.component.html', })Also remove the unused import:
-import { CommonModule } from '@angular/common';src/main/webapp/app/core/user/settings/learner-profile/insights-learner-profile/iris-learner-profile/resolve-memories-conflicts-modal.component.ts (1)
54-57: Remove unused_groupIndexparameter.The
_groupIndexparameter is documented as unused. Consider removing it entirely to avoid confusion, or if it's required for template binding compatibility, document that explicitly.♻️ Suggested fix
- async keep(_groupIndex: number, keepId: string): Promise<void> { + async keep(keepId: string): Promise<void> {src/main/webapp/app/core/user/settings/learner-profile/insights-learner-profile/iris-learner-profile/memiris-memories-list.component.ts (2)
176-198: Spread operator usage for objects.Per coding guidelines, prefer avoiding the spread operator for objects. The current implementation uses spread in multiple places (lines 183-184, 189, 194). While functional, consider using explicit object construction or
Object.assignfor consistency with project conventions.♻️ Example refactor for one instance
- this.open.update((o) => { - const copy = { ...o }; - for (const id of deletedIds) delete copy[id]; - return copy; - }); + this.open.update((o) => { + const copy = Object.assign({}, o); + for (const id of deletedIds) delete copy[id]; + return copy; + });
116-120: Remove orphaned comment.This comment block and the empty
// conflicts are derived...line appear to be leftover artifacts. The conflict computation logic is already documented at theconflictGroupscomputed signal definition.♻️ Suggested fix
- /** - * Computes conflict groups from aggregated connections and precomputes details for involved memories. - */ - // conflicts are derived via computed conflictGroups -
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
Summary
This PR updates the learner profile to use the new aggregated Iris memories API, eliminating per-memory requests and significantly improving performance and UX. Memory details and conflict detection are now derived locally from aggregated data, enabling instant expansion, silent updates after deletions, and removal of unnecessary loading states. The change also modernizes the client with Angular Signals, introduces a dedicated memory details component, and aligns client, server, tests, and shared models with the updated API.
Checklist
General
Server
Client
Motivation and Context
Memiris is not yet capable of automatically resolving memory conflicts, as this will be a significant effort outside of the scope of my Master's thesis.
Therefore for the meantime, we want to ensure that users are able to resolve memories manually in their settings.
Additionally, this PR reworks the system to load memories, as the old one was too inefficient to support this new system.
Description
Client
IrisMemoriesHttpService
getUserMemoryData()→GET api/iris/user/memoryData.getUserMemory(id)anddeleteUserMemory(id)→GET/DELETE api/iris/user/memory/{id}.MemirisMemoriesListComponent
loading,deleting,memoryData,open).memories,conflictGroups,hasConflicts,details).memiris-memory-details.component (standalone)
Templates
Tests (client)
Server
Aligned IrisMemoryResource paths:
GET api/iris/user/memoryDataGET/DELETE api/iris/user/memory/{id}Tests (server)
IrisRequestMockProviderto mock the v2 aggregated Memiris endpoint while retaining v1 single-memory routes.Shared models
MemirisMemoryDataDTO(memories, learnings, connections) to match the server response.i18n
Steps for Testing
Prerequisites
Iris: Add better ways to retrieve memories via the API edutelligence#392 deployed to Iris TestTestserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Performance Review
Code Review
Manual Tests
Test Coverage
Client
Server
Last updated: 2026-01-26 21:30:43 UTC
Screenshots
Summary by CodeRabbit
Release Notes
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.