Skip to content

Conversation

@alexjoham
Copy link
Member

@alexjoham alexjoham commented Apr 28, 2025

Checklist

General

Client

  • Important: I implemented the changes with a very good performance, prevented too many (unnecessary) REST calls and made sure the UI is responsive, even with large data (e.g. using paging).
  • I strictly followed the principle of data economy for all client-server REST calls.
  • I strictly followed the client coding and design guidelines.
  • I added multiple integration tests (Jest) related to the features (with a high test coverage), while following the test guidelines.
  • I documented the TypeScript code using JSDoc style.

Motivation and Context

As mentioned in PR#10748 the redirect-to-iris-component fetches the same iris-settings multiple times. This should not happen as it fetches the exact same data from the same REST endpoint. Only one call should be made.

Description

This PR adds two changes.
The first change reduces the calls in the iris-settings.service.ts by caching all the calls. This is done by first caching pending requests and the caching the fetched settings. The cache save them for 5 minutes to prevent updated settings to not take affect.
The second change is in the redirect-to-iris-button.component.ts by fetching the iris-settings only if the conversation id changes. This fixes multiple requests if metadata changes for the conversations which triggers a new request.
With this changes the component now only fetches the settings once.
Disclaimer: If you first test the exercise and then the lecture channels a second request is triggered, because those are different settings that need to be fetched separately.

Steps for Testing

Prerequisites:

  • 1 Student in a course with comms enabled and a lecture with communication channel
  • iris activated for the course and exercises
  1. Log in to Artemis
  2. Navigate to Communication
  3. Open a channel of a lecture or exercise
  4. Verify that only one iris-settings call is made

Testserver 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

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Test Coverage

Client

Class/File Line Coverage Confirmation (assert/expect)
redirect-to-iris-button.component.ts 93.5% ✅ ❌
iris-settings.service.ts 98.21% ✅ ❌

Summary by CodeRabbit

  • New Features
    • Improved performance and reliability of Iris settings retrieval by introducing caching and request deduplication for course and exercise settings.
  • Bug Fixes
    • Enhanced handling of active conversations to ensure settings are only checked when necessary, reducing unnecessary operations.
  • Tests
    • Added comprehensive tests for caching, request reuse, cache expiration, and error recovery in Iris settings services.
    • Extended tests for conversation handling in the redirect button component.

@alexjoham alexjoham requested review from a team as code owners April 28, 2025 14:34
@github-project-automation github-project-automation bot moved this to Work In Progress in Artemis Development Apr 28, 2025
@github-actions github-actions bot added client Pull requests that update TypeScript code. (Added Automatically!) communication Pull requests that affect the corresponding module iris Pull requests that affect the corresponding module labels Apr 28, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Apr 28, 2025

Walkthrough

This pull request enhances both the RedirectToIrisButtonComponent and the IrisSettingsService to improve efficiency and reliability. The component now manages subscriptions more robustly and optimizes calls to fetch Iris settings based on conversation changes, using RxJS operators to avoid redundant requests. The service introduces in-memory caching and request deduplication for fetching combined course and exercise settings, preventing unnecessary network calls and ensuring consistent data delivery. The test suites for both the component and the service are expanded to cover the new behaviors, including cache expiration, error handling, and request reuse scenarios.

Changes

File(s) Change Summary
src/main/webapp/app/communication/shared/redirect-to-iris-button/redirect-to-iris-button.component.ts Added subscription management for settings fetches; optimized observable pipeline using filter and distinctUntilKeyChanged; unsubscribes from settings subscription in ngOnDestroy; updates to use a private subscription for fetching settings.
src/main/webapp/app/iris/manage/settings/shared/iris-settings.service.ts Added in-memory caching and request deduplication for getCombinedCourseSettings and getCombinedExerciseSettings; uses cache with 5-minute expiry and shares pending HTTP requests to avoid duplicates; updates cache on success and clears on error or completion.
src/main/webapp/app/communication/shared/redirect-to-iris-button/redirect-to-iris-button.component.spec.ts Added tests to verify component behavior for active conversation changes, including ensuring settings are fetched only on ID changes and not for repeated or undefined conversations.
src/main/webapp/app/iris/manage/settings/shared/iris-settings.service.spec.ts Added tests for caching, request reuse, cache expiration, and error handling for combined course and exercise settings in the Iris settings service.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant RedirectToIrisButtonComponent
    participant IrisSettingsService
    participant Server

    User->>RedirectToIrisButtonComponent: Selects/changes conversation
    RedirectToIrisButtonComponent->>IrisSettingsService: getCombinedCourseSettings/courseSettings
    alt Cache is valid
        IrisSettingsService-->>RedirectToIrisButtonComponent: Return cached settings
    else Pending request exists
        IrisSettingsService-->>RedirectToIrisButtonComponent: Share pending observable
    else No cache or pending request
        IrisSettingsService->>Server: HTTP GET settings
        Server-->>IrisSettingsService: Responds with settings
        IrisSettingsService-->>RedirectToIrisButtonComponent: Return settings, update cache
    end
Loading

Possibly related PRs

  • ls1intum/Artemis#10748: Also modifies the RedirectToIrisButtonComponent test suite and logic for fetching Iris settings in relation to active conversations.
  • ls1intum/Artemis#10394: Introduces the initial implementation of RedirectToIrisButtonComponent and Iris redirect button features, which this PR refines and builds upon.

Suggested labels

tests, ready for review, client, iris, priority:high, bugfix

Suggested reviewers

  • florian-glombik
  • krusche
  • HawKhiem

📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b25fe81 and c446b31.

📒 Files selected for processing (4)
  • src/main/webapp/app/communication/shared/redirect-to-iris-button/redirect-to-iris-button.component.spec.ts (1 hunks)
  • src/main/webapp/app/communication/shared/redirect-to-iris-button/redirect-to-iris-button.component.ts (4 hunks)
  • src/main/webapp/app/iris/manage/settings/shared/iris-settings.service.spec.ts (3 hunks)
  • src/main/webapp/app/iris/manage/settings/shared/iris-settings.service.ts (4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalC...

src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

  • src/main/webapp/app/communication/shared/redirect-to-iris-button/redirect-to-iris-button.component.spec.ts
  • src/main/webapp/app/communication/shared/redirect-to-iris-button/redirect-to-iris-button.component.ts
  • src/main/webapp/app/iris/manage/settings/shared/iris-settings.service.spec.ts
  • src/main/webapp/app/iris/manage/settings/shared/iris-settings.service.ts
🧬 Code Graph Analysis (1)
src/main/webapp/app/iris/manage/settings/shared/iris-settings.service.ts (1)
src/main/webapp/app/iris/shared/entities/settings/iris-settings.model.ts (2)
  • IrisCourseSettings (42-53)
  • IrisExerciseSettings (55-61)
🔇 Additional comments (20)
src/main/webapp/app/communication/shared/redirect-to-iris-button/redirect-to-iris-button.component.ts (5)

5-5: Good use of RxJS operators for optimization.

Adding distinctUntilKeyChanged and filter operators is a well-thought-out approach for reducing unnecessary API calls.


36-36: Good subscription management implementation.

Adding dedicated settingsSubscription variable improves memory management by ensuring all observables are properly tracked and unsubscribed.


52-59: Excellent optimization using RxJS operators.

The use of filter to prevent processing undefined conversations and distinctUntilKeyChanged('id') to only respond when the conversation ID changes effectively reduces redundant API calls, addressing the core issue mentioned in the PR objectives.


64-64: Proper resource cleanup in ngOnDestroy.

Unsubscribing from settingsSubscription in ngOnDestroy prevents memory leaks by ensuring all subscriptions are properly cleaned up when the component is destroyed.


86-86: Effective subscription management.

Assigning the result of fetchSettings() to this.settingsSubscription ensures that the subscription is properly tracked and can be unsubscribed when the component is destroyed.

src/main/webapp/app/communication/shared/redirect-to-iris-button/redirect-to-iris-button.component.spec.ts (3)

274-283: Good test for handling undefined conversations.

This test verifies that the component doesn't call checkIrisSettings when the conversation is undefined, which is crucial for preventing unnecessary API calls.


285-297: Good test for handling conversation ID changes.

This test confirms that the component correctly calls checkIrisSettings for each new conversation with a different ID, validating the core functionality of the component.


299-310: Excellent test for verifying optimization.

This test verifies the key optimization introduced - ensuring that checkIrisSettings is only called once for consecutive emissions with the same conversation ID. This directly validates the functionality of the distinctUntilKeyChanged operator.

src/main/webapp/app/iris/manage/settings/shared/iris-settings.service.spec.ts (6)

64-77: Good test for request deduplication.

This test verifies that multiple simultaneous calls for the same course settings reuse the same pending HTTP request, which is crucial for preventing multiple redundant API calls.


79-91: Good test for caching functionality.

This test confirms that cached results are returned on subsequent calls without triggering new HTTP requests, validating the caching mechanism works as expected.


126-139: Good test for exercise settings request deduplication.

This test verifies the request deduplication for exercise settings, confirming consistent behavior across different types of settings.


141-153: Good test for exercise settings caching.

This test validates the caching mechanism for exercise settings, ensuring consistent behavior with course settings.


155-185: Good tests for cache expiration.

These tests verify that after the cache duration (5 minutes), new HTTP requests are issued. This ensures that stale data isn't used indefinitely.


202-244: Good tests for error handling.

These tests verify that on HTTP request failure, the pending request is cleared to allow retries. This ensures the system can recover from failures appropriately.

src/main/webapp/app/iris/manage/settings/shared/iris-settings.service.ts (6)

3-4: Good RxJS operator imports.

Adding the necessary RxJS operators (of, finalize, shareReplay, tap) to support the caching and request deduplication functionality.


18-24: Well-structured cache implementation.

The implementation uses separate Maps for caching settings, tracking pending requests, and storing timestamps, which provides a clean and maintainable structure. The 5-minute cache duration is a reasonable balance between data freshness and reducing API calls.


46-49: Good method documentation.

The updated documentation clearly explains the caching and request deduplication features, which helps future developers understand the behavior of the method.


52-80: Excellent implementation of caching and request deduplication for course settings.

The implementation:

  1. Checks for valid cached data and returns it if available
  2. Checks for pending requests and returns the existing observable if found
  3. Creates a new request with proper caching and lifecycle management
  4. Uses shareReplay(1) to multicast the response to all subscribers
  5. Updates the cache on successful responses
  6. Cleans up pending requests with finalize

This approach effectively reduces unnecessary network calls while ensuring data is kept reasonably up-to-date.


93-96: Good method documentation for exercise settings.

The updated documentation for getCombinedExerciseSettings clearly explains the caching and request deduplication features, matching the documentation for course settings.


99-127: Consistent implementation for exercise settings.

The implementation for getCombinedExerciseSettings follows the same pattern as getCombinedCourseSettings, ensuring consistent behavior across different types of settings.

✨ Finishing Touches
  • 📝 Generate Docstrings

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions
Copy link

End-to-End (E2E) Test Results Summary

TestsPassed ☑️Skipped ⚠️Failed ❌️Time ⏱
End-to-End (E2E) Test Report201 ran197 passed3 skipped1 failed48m 26s 948ms
TestResultTime ⏱
End-to-End (E2E) Test Report
e2e/course/CourseMessages.spec.ts
ts.Course messages › Channel messages › Write/edit/delete message in channel › Student should be able to edit message in channel❌ failure2m 3s 152ms

@github-actions
Copy link

End-to-End (E2E) Test Results Summary

TestsPassed ☑️Skipped ⚠️Failed ❌️Time ⏱
End-to-End (E2E) Test Report201 ran196 passed3 skipped2 failed49m 2s 790ms
TestResultTime ⏱
End-to-End (E2E) Test Report
e2e/course/CourseMessages.spec.ts
ts.Course messages › Channel messages › Write/edit/delete message in channel › Student should be able to edit message in channel❌ failure2m 4s 384ms
e2e/exercise/programming/ProgrammingExerciseStaticCodeAnalysis.spec.ts
ts.Static code analysis tests › Configures SCA grading and makes a successful submission with SCA errors❌ failure1m 43s 381ms

@helios-aet helios-aet bot temporarily deployed to artemis-test3.artemis.cit.tum.de April 28, 2025 19:49 Inactive
Copy link
Contributor

@HawKhiem HawKhiem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on TS3. Works as expected

image

Copy link
Contributor

@Anishyou Anishyou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Locally tested
earlier-
image
after-
image

Copy link
Contributor

@ahmetsenturk ahmetsenturk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code lgmt

@helios-aet helios-aet bot temporarily deployed to artemis-test1.artemis.cit.tum.de April 30, 2025 12:08 Inactive
Copy link

@chuuuun chuuuun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested on ts1, user 5. Works as described
Screenshot 2025-04-30 at 14 15 48

@krusche krusche added this to the 8.0.5 milestone Apr 30, 2025
@krusche krusche merged commit 3291382 into develop May 1, 2025
42 of 45 checks passed
@krusche krusche deleted the bugfix/communication/reduce-iris-settings-calls branch May 1, 2025 22:16
@github-project-automation github-project-automation bot moved this from Work In Progress to Merged in Artemis Development May 1, 2025
@github-project-automation github-project-automation bot moved this from Todo to Done in Communication Webclient May 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

client Pull requests that update TypeScript code. (Added Automatically!) communication Pull requests that affect the corresponding module iris Pull requests that affect the corresponding module ready to merge

Projects

Archived in project
Status: Done

Development

Successfully merging this pull request may close these issues.

7 participants