Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Iris: Add a temporary ChatGPT interface for specific user groups and exercises #10167

Merged
merged 11 commits into from
Jan 21, 2025

Conversation

wasnertobias
Copy link
Contributor

@wasnertobias wasnertobias commented Jan 17, 2025

Split users in programming exercise in three groups for the conduction of an experiment:

  • ChatGPT variant (without context):
  • Iris as usual
  • No AI

Checklist

General

Steps for Testing

  • 1 Instructor
  • 1 Programming Exercise with Iris enabled and "ICER 2025 Paper a5157934-9092-4a72-addc-3aaf489debdc" in the problem statement
  1. Log in to Artemis
  2. Participate in the programming exercises with multiple users. Randomly (based on the user id), one should see Iris, one should see ChatGPT and one should see no AI

Testserver 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

Performance Review

Code Review

  • Code Review 1

Manual Tests

  • Test 1

Summary by CodeRabbit

  • New Features

    • Added a temporary ChatGPT wrapper feature for a specific paper
    • Introduced conditional rendering for chatbot interface based on exercise and user criteria
  • Documentation

    • Added disclaimer messages for ChatGPT interactions in English and German
  • Chores

    • Updated test configurations to support new feature
    • Slight adjustment to code coverage threshold for iris module

@wasnertobias wasnertobias requested a review from a team as a code owner January 17, 2025 16:21
@github-actions github-actions bot added server Pull requests that update Java code. (Added Automatically!) exercise Pull requests that affect the corresponding module iris Pull requests that affect the corresponding module labels Jan 17, 2025
@wasnertobias wasnertobias requested a review from bassner January 17, 2025 16:21
Copy link

coderabbitai bot commented Jan 17, 2025

Warning

Rate limit exceeded

@bassner has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 50 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between 76b6896 and ca64629.

📒 Files selected for processing (1)
  • gradle/jacoco.gradle (1 hunks)

Walkthrough

The pull request introduces conditional modifications across several components in the application. In the ExerciseResource and IrisExerciseChatSessionService classes, new logic checks if an exercise's problem statement contains the constant ICER_PAPER_FLAG and adjusts the behavior based on the user's ID. Additionally, the IrisExerciseChatbotButtonComponent and related HTML files have been updated to conditionally render elements based on a new input property, isChatGptWrapper. These changes are marked as temporary features for a specific use case.

Changes

File Change Summary
src/main/java/de/tum/cit/aet/artemis/.../ExerciseResource.java Added conditional block in getExerciseDetails method to set irisSettings to null if problem statement contains ICER_PAPER_FLAG and user ID modulo 3 equals 2.
src/main/java/de/tum/cit/aet/artemis/.../IrisExerciseChatSessionService.java Added conditional logic in requestAndHandleResponse method to set variant to "chat-gpt-wrapper" if problem statement contains ICER_PAPER_FLAG and user ID is divisible by 3.
src/main/webapp/app/iris/exercise-chatbot/.../exercise-chatbot-button.component.html Introduced conditional rendering for chatbot button based on isChatGptWrapper variable, displaying a temporary ChatGPT logo if true.
src/main/webapp/app/iris/exercise-chatbot/.../exercise-chatbot-button.component.ts Added new input property isChatGptWrapper to the component, initialized to false.
src/main/webapp/app/overview/exercise-details/.../course-exercise-details.component.html Updated jhi-exercise-chatbot-button instantiation to include new input property isChatGptWrapper.
src/main/webapp/app/overview/exercise-details/.../course-exercise-details.component.ts Added AccountService import and private property to manage user identity, along with a new boolean property isChatGptWrapper.
src/main/java/de/tum/cit/aet/artemis/core/config/.../Constants.java Added new constant ICER_PAPER_FLAG with the value "ICER 2025 Paper a5157934-9092-4a72-addc-3aaf489debdc".
src/main/webapp/app/.../app.constants.ts Added new constant ICER_PAPER_FLAG for the same value as above.
src/main/webapp/app/iris/base-chatbot/.../iris-base-chatbot.component.html Implemented conditional rendering for logos and text based on isChatGptWrapper.
src/main/webapp/app/iris/base-chatbot/.../iris-base-chatbot.component.ts Added new input property isChatGptWrapper to the component.
src/main/webapp/app/iris/exercise-chatbot/widget/.../chatbot-widget.component.html Added [isChatGptWrapper] property to <jhi-iris-base-chatbot> component.
src/main/webapp/app/iris/exercise-chatbot/widget/.../chatbot-widget.component.ts Introduced dialogData property to access isChatGptWrapper.
src/main/webapp/i18n/de/exerciseChatbot.json Added new key disclaimerGPT with a disclaimer message in German.
src/main/webapp/i18n/en/exerciseChatbot.json Added new key disclaimerGPT with a disclaimer message in English.
src/test/javascript/spec/component/iris/ui/chatbot-widget.component.spec.ts Added provider for MAT_DIALOG_DATA with default value { isChatGptWrapper: false } in test setup for IrisChatbotWidgetComponent.
gradle/jacoco.gradle Modified the INSTRUCTION coverage threshold for the iris module from 0.795 to 0.792.

Possibly related PRs

Suggested labels

small, bugfix, ready to merge

Suggested reviewers

  • kaancayli
  • Hialus
  • florian-glombik
  • JohannesStoehr

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?

❤️ 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. (Beta)
  • @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.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java (1)

174-181: Consider extracting the experiment logic into a dedicated service.

The experimental group assignment logic is duplicated across files and mixed with regular business logic. This violates the Single Responsibility Principle and makes it harder to maintain and eventually remove the temporary code.

Consider creating an ExperimentService to encapsulate all experiment-related logic:

@Service
public class ExperimentService {
    private static final String ICER_PAPER_FEATURE_FLAG = "ICER 2025 Paper";
    private static final int TOTAL_GROUPS = 3;
    
    public enum ExperimentGroup {
        CHATGPT(0),
        IRIS(1),
        CONTROL(2);
        
        private final int modValue;
        ExperimentGroup(int modValue) {
            this.modValue = modValue;
        }
    }
    
    public boolean isIcerPaperExperiment(Exercise exercise) {
        return exercise.getProblemStatement() != null && 
               exercise.getProblemStatement().contains(ICER_PAPER_FEATURE_FLAG);
    }
    
    public ExperimentGroup getExperimentGroup(User user) {
        int group = user.getId() % TOTAL_GROUPS;
        return Arrays.stream(ExperimentGroup.values())
                    .filter(g -> g.modValue == group)
                    .findFirst()
                    .orElse(ExperimentGroup.IRIS);
    }
}
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 5941797 and d176ad4.

⛔ Files ignored due to path filters (1)
  • src/main/resources/public/images/chatgpt-temp/ChatGPT_logo.svg is excluded by !**/*.svg, !**/*.svg
📒 Files selected for processing (2)
  • src/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseResource.java (1 hunks)
  • src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

src/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseResource.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

⏰ Context from checks skipped due to timeout of 90000ms (6)
  • GitHub Check: client-style
  • GitHub Check: client-tests
  • GitHub Check: server-tests
  • GitHub Check: Build and Push Docker Image
  • GitHub Check: Analyse
  • GitHub Check: Build .war artifact

@github-actions github-actions bot added the client Pull requests that update TypeScript code. (Added Automatically!) label Jan 20, 2025
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (3)
src/main/webapp/app/iris/exercise-chatbot/exercise-chatbot-button.component.ts (1)

55-56: Consider documenting the temporary feature's expiration.

The TODO comment indicates this is a temporary feature. Consider adding:

  1. An expiration date or milestone
  2. A tracking issue for removal
  3. Documentation about the feature's purpose and scope
src/main/webapp/app/iris/exercise-chatbot/exercise-chatbot-button.component.html (1)

20-25: Improve the ChatGPT logo implementation.

Consider the following improvements:

  1. Move the hardcoded image path to a constant or configuration
  2. Move the inline style to the component's stylesheet
  3. Use a more descriptive alt text for accessibility

Apply this diff:

-            <img src="public/images/chatgpt-temp/ChatGPT_logo.svg" alt="Iris Logo" style="height: 70px" class="iris-logo" (click)="handleButtonClick()" />
+            <img [src]="chatGptLogoPath" alt="ChatGPT Assistant Logo" class="iris-logo chatgpt-logo" (click)="handleButtonClick()" />

Add to the component's TypeScript file:

readonly chatGptLogoPath = 'public/images/chatgpt-temp/ChatGPT_logo.svg';

Add to the component's SCSS file:

.chatgpt-logo {
    height: 70px;
}
src/main/webapp/app/overview/exercise-details/course-exercise-details.component.html (1)

193-194: Ensure consistent TODO comments across files.

The TODO comment should be consistent with the one in the component definition. Consider moving all temporary feature documentation to a central location.

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between d176ad4 and e204965.

📒 Files selected for processing (4)
  • src/main/webapp/app/iris/exercise-chatbot/exercise-chatbot-button.component.html (1 hunks)
  • src/main/webapp/app/iris/exercise-chatbot/exercise-chatbot-button.component.ts (1 hunks)
  • src/main/webapp/app/overview/exercise-details/course-exercise-details.component.html (1 hunks)
  • src/main/webapp/app/overview/exercise-details/course-exercise-details.component.ts (4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/main/webapp/app/iris/exercise-chatbot/exercise-chatbot-button.component.html (1)

Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.

src/main/webapp/app/iris/exercise-chatbot/exercise-chatbot-button.component.ts (1)

Pattern 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/overview/exercise-details/course-exercise-details.component.ts (1)

Pattern 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/overview/exercise-details/course-exercise-details.component.html (1)

Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.

🪛 Biome (1.9.4)
src/main/webapp/app/overview/exercise-details/course-exercise-details.component.ts

[error] 231-231: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

⏰ Context from checks skipped due to timeout of 90000ms (8)
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: client-tests-selected
  • GitHub Check: client-tests
  • GitHub Check: server-tests
  • GitHub Check: Build and Push Docker Image
  • GitHub Check: server-style
  • GitHub Check: Build .war artifact
  • GitHub Check: Analyse

@ls1intum ls1intum deleted a comment from coderabbitai bot Jan 21, 2025
@github-actions github-actions bot added the core Pull requests that affect the corresponding module label Jan 21, 2025
@bassner bassner changed the title General Special hacks for ICER 2025 Paper General: Special hacks for ICER 2025 Paper Jan 21, 2025
@bassner bassner changed the title General: Special hacks for ICER 2025 Paper Iris: Special hacks for ICER 2025 Paper Jan 21, 2025
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Nitpick comments (4)
src/main/webapp/app/iris/base-chatbot/iris-base-chatbot.component.ts (1)

148-148: Add JSDoc comment for the isChatGptWrapper input property.

While the implementation is correct, adding documentation would help other developers understand the purpose and usage of this property.

+    /**
+     * Indicates whether the chatbot should use ChatGPT variant UI.
+     * This is a temporary feature for ICER 2025 paper.
+     * @default false
+     */
     @Input() isChatGptWrapper = false;
src/main/webapp/app/iris/exercise-chatbot/widget/chatbot-widget.component.html (1)

Line range hint 38-39: Consider implementing a feature flag service.

Since this is a temporary feature for research purposes, consider implementing a proper feature flag service (like LaunchDarkly or a custom solution) instead of using string constants. This would make it easier to:

  1. Control the feature rollout
  2. Collect metrics
  3. Remove the feature after the paper
  4. Manage future experimental features

This would also help prevent temporary code from becoming permanent technical debt.

Also applies to: 148-148

src/main/webapp/app/iris/base-chatbot/iris-base-chatbot.component.html (2)

140-145: Reduce code duplication in logo rendering.

The logo rendering logic is duplicated from the header section. Consider extracting this into a reusable component.

// Create a new component: chat-logo.component.ts
@Component({
  selector: 'jhi-chat-logo',
  template: `
    @if (!isChatGptWrapper) {
      <jhi-iris-logo [size]="size" />
    } @else {
      <img [src]="chatGptLogoPath" [alt]="'ChatGPT Logo'" [class]="logoClass" />
    }
  `
})
export class ChatLogoComponent {
  @Input() isChatGptWrapper: boolean;
  @Input() size: IrisLogoSize;
  @Input() logoClass: string;
}

Line range hint 4-206: Consider implementing a robust feature flag system for research experiments.

The current implementation uses a simple boolean flag for the ICER paper experiment. For better maintainability and future research needs, consider:

  1. Implementing a proper feature flag system that supports multiple experiments
  2. Moving all experiment-related configurations to a dedicated service
  3. Adding clear documentation about the lifecycle of research features

This will make it easier to:

  • Manage multiple concurrent experiments
  • Clean up temporary features
  • Track experiment-specific metrics
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 5afdb38 and 5fc4731.

📒 Files selected for processing (13)
  • src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1 hunks)
  • src/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseResource.java (2 hunks)
  • src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java (2 hunks)
  • src/main/webapp/app/app.constants.ts (1 hunks)
  • src/main/webapp/app/iris/base-chatbot/iris-base-chatbot.component.html (3 hunks)
  • src/main/webapp/app/iris/base-chatbot/iris-base-chatbot.component.scss (1 hunks)
  • src/main/webapp/app/iris/base-chatbot/iris-base-chatbot.component.ts (1 hunks)
  • src/main/webapp/app/iris/exercise-chatbot/exercise-chatbot-button.component.ts (2 hunks)
  • src/main/webapp/app/iris/exercise-chatbot/widget/chatbot-widget.component.html (1 hunks)
  • src/main/webapp/app/iris/exercise-chatbot/widget/chatbot-widget.component.ts (2 hunks)
  • src/main/webapp/app/overview/exercise-details/course-exercise-details.component.ts (5 hunks)
  • src/main/webapp/i18n/de/exerciseChatbot.json (1 hunks)
  • src/main/webapp/i18n/en/exerciseChatbot.json (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java
  • src/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseResource.java
  • src/main/webapp/app/iris/exercise-chatbot/exercise-chatbot-button.component.ts
🧰 Additional context used
📓 Path-based instructions (7)
src/main/webapp/i18n/de/exerciseChatbot.json (1)

Pattern src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".

src/main/webapp/app/iris/exercise-chatbot/widget/chatbot-widget.component.ts (1)

Pattern 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/app.constants.ts (1)

Pattern 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/iris/base-chatbot/iris-base-chatbot.component.ts (1)

Pattern 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/iris/exercise-chatbot/widget/chatbot-widget.component.html (1)

Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.

src/main/webapp/app/iris/base-chatbot/iris-base-chatbot.component.html (1)

Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.

src/main/webapp/app/overview/exercise-details/course-exercise-details.component.ts (1)

Pattern 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

🪛 Biome (1.9.4)
src/main/webapp/app/overview/exercise-details/course-exercise-details.component.ts

[error] 231-231: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: Codacy Static Code Analysis
  • 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: Build .war artifact
🔇 Additional comments (6)
src/main/webapp/app/iris/exercise-chatbot/widget/chatbot-widget.component.ts (1)

9-9: LGTM! Clean implementation of dialog data injection.

The implementation follows Angular's dependency injection best practices for dialog data handling.

Also applies to: 22-22

src/main/webapp/app/iris/exercise-chatbot/widget/chatbot-widget.component.html (1)

4-10: LGTM! Clean template binding implementation.

The template follows Angular best practices with proper property binding and safe null handling.

src/main/webapp/app/iris/base-chatbot/iris-base-chatbot.component.scss (1)

165-191: LGTM! The changes improve style encapsulation and margin control.

The modifications enhance the component's style isolation by moving bubble styles inside the :host selector and provide better control over nested paragraph margins.

src/main/webapp/i18n/en/exerciseChatbot.json (1)

56-56: LGTM! The translation key follows the existing pattern.

The new disclaimer message for ChatGPT maintains consistency with the existing Iris disclaimer.

src/main/webapp/i18n/de/exerciseChatbot.json (1)

56-56: LGTM! The German translation follows the informal style guideline.

The new disclaimer message for ChatGPT maintains consistency and uses the informal "du" form as required.

src/main/webapp/app/iris/base-chatbot/iris-base-chatbot.component.html (1)

202-206: LGTM! Clean implementation of conditional disclaimer messages.

The implementation correctly uses translation keys and follows the new Angular syntax guidelines.

@github-actions github-actions bot added the tests label Jan 21, 2025
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (3)
src/main/webapp/app/overview/exercise-details/course-exercise-details.component.ts (2)

116-116: Consider removing the TODO comment from the injection declaration.

The TODO comment in the injection declaration adds noise to the code. Since this is a temporary feature, consider moving the comment to a more appropriate location, such as the feature's implementation block.

-    private accountService = inject(AccountService); // TODO TW: This "feature" is only temporary for a paper.
+    private accountService = inject(AccountService);

134-134: Move the TODO comment to a documentation block.

Instead of inline TODO comments, consider using a documentation block at the property declaration to better explain the temporary nature of this feature.

-    isChatGptWrapper: boolean = false; // TODO TW: This "feature" is only temporary for a paper.
+    /** 
+     * @temporary This feature is only for ICER 2025 Paper
+     * Controls whether to show the ChatGPT variant of the interface
+     */
+    isChatGptWrapper = false;
src/test/javascript/spec/component/iris/ui/chatbot-widget.component.spec.ts (1)

19-24: Enhance mock implementations and provider configuration.

While the providers are correctly defined, consider these improvements:

  1. Use jest.spyOn() instead of jest.fn() for better test maintainability
  2. Follow the NgMocks pattern consistently across all providers
 providers: [
     MockProvider(IrisChatService),
-    { provide: MatDialog, useValue: { closeAll: jest.fn() } },
+    MockProvider(MatDialog, {
+        closeAll: jest.spyOn(jest.fn(), 'closeAll')
+    }),
     { provide: Router, useValue: { events: of() } },
     { provide: MAT_DIALOG_DATA, useValue: { isChatGptWrapper: false } },
 ],
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 5fc4731 and 3c9473c.

📒 Files selected for processing (3)
  • src/main/webapp/app/iris/base-chatbot/iris-base-chatbot.component.scss (1 hunks)
  • src/main/webapp/app/overview/exercise-details/course-exercise-details.component.ts (5 hunks)
  • src/test/javascript/spec/component/iris/ui/chatbot-widget.component.spec.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/main/webapp/app/iris/base-chatbot/iris-base-chatbot.component.scss
🧰 Additional context used
📓 Path-based instructions (2)
src/test/javascript/spec/component/iris/ui/chatbot-widget.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/main/webapp/app/overview/exercise-details/course-exercise-details.component.ts (1)

Pattern 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

🪛 Biome (1.9.4)
src/main/webapp/app/overview/exercise-details/course-exercise-details.component.ts

[error] 231-231: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

⏰ Context from checks skipped due to timeout of 90000ms (8)
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: server-tests
  • GitHub Check: server-style
  • GitHub Check: client-tests-selected
  • GitHub Check: client-tests
  • GitHub Check: Build and Push Docker Image
  • GitHub Check: Build .war artifact
  • GitHub Check: Analyse
🔇 Additional comments (5)
src/main/webapp/app/overview/exercise-details/course-exercise-details.component.ts (2)

39-39: LGTM: Import statement follows Angular conventions.

The import statement for ICER_PAPER_FLAG is correctly grouped with related constants.


228-234: 🛠️ Refactor suggestion

Improve the user group assignment logic.

The current implementation has several issues:

  1. The condition uses optional chaining incorrectly (as flagged by static analysis)
  2. The logic for user group assignment is not clearly documented
  3. The implementation might be better served by a dedicated service

Consider applying these improvements:

-        // TODO TW: This "feature" is only temporary for a paper.
-        if (this.exercise.problemStatement?.includes(ICER_PAPER_FLAG)) {
-            this.accountService.identity().then((user) => {
-                this.isChatGptWrapper = user && user.id ? user.id % 3 == 0 : false;
-            });
-        }
+        /**
+         * ICER 2025 Paper Feature
+         * Assigns users to one of three groups based on their ID:
+         * - Group 0 (id % 3 = 0): ChatGPT variant
+         * - Group 1 (id % 3 = 1): Standard Iris variant
+         * - Group 2 (id % 3 = 2): Control group (no AI)
+         */
+        if (this.exercise.problemStatement?.includes(ICER_PAPER_FLAG)) {
+            this.accountService.identity().then((user) => {
+                this.isChatGptWrapper = user?.id !== undefined && user.id % 3 === 0;
+            });
+        }

Additionally, consider:

  1. Moving this logic to a dedicated service (e.g., UserGroupAssignmentService)
  2. Adding error handling for the promise
  3. Using an enum to represent the user groups

Would you like me to help create a dedicated service for managing the user group assignments?

Likely invalid or redundant comment.

🧰 Tools
🪛 Biome (1.9.4)

[error] 231-231: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

src/test/javascript/spec/component/iris/ui/chatbot-widget.component.spec.ts (3)

5-5: LGTM!

The import for MAT_DIALOG_DATA is correctly added and follows Angular Material import best practices.


Line range hint 33-83: Add test coverage for isChatGptWrapper functionality.

The test suite lacks coverage for the new isChatGptWrapper feature. Please add the following test cases:

  1. Component behavior when isChatGptWrapper is true
  2. Component's reaction to dialog data changes
  3. Integration with IrisBaseChatbotComponent

Example test case structure:

it('should handle isChatGptWrapper flag correctly', () => {
    TestBed.resetTestingModule();
    TestBed.configureTestingModule({
        declarations: [IrisChatbotWidgetComponent, MockComponent(IrisBaseChatbotComponent)],
        providers: [
            MockProvider(IrisChatService),
            MockProvider(MatDialog),
            { provide: Router, useValue: { events: of() } },
            { provide: MAT_DIALOG_DATA, useValue: { isChatGptWrapper: true } }
        ]
    }).compileComponents();

    const fixture = TestBed.createComponent(IrisChatbotWidgetComponent);
    const component = fixture.componentInstance;
    fixture.detectChanges();

    // Add expectations based on the actual component behavior
    expect(component.dialogData.isChatGptWrapper).toBeTrue();
});

Line range hint 1-83: Verify the consistency of isChatGptWrapper usage across components.

Let's ensure that the isChatGptWrapper flag is consistently used across all related components.

✅ Verification successful

The isChatGptWrapper implementation is consistent but temporary

The flag is properly implemented across the component hierarchy with consistent data flow and adequate test coverage. However, note that this feature is explicitly marked as temporary for a paper, as indicated by TODO comments in both the component and template.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for consistent usage of isChatGptWrapper across components

# Check for isChatGptWrapper usage in component files
echo "Checking isChatGptWrapper usage in components:"
rg -l "isChatGptWrapper" "src/main/webapp/app/iris"

# Check for consistent dialog data structure
echo "\nChecking MAT_DIALOG_DATA usage:"
ast-grep --pattern 'MAT_DIALOG_DATA' -l ts

# Check for potential missing test coverage
echo "\nChecking test coverage for components using isChatGptWrapper:"
fd -e spec.ts$ . src/test/javascript/spec/component/iris

Length of output: 1848


Script:

#!/bin/bash
# Check implementation details and usage patterns

# Show how isChatGptWrapper is used in component implementations
echo "Implementation in chatbot-widget.component.ts:"
rg -A 5 "isChatGptWrapper" "src/main/webapp/app/iris/exercise-chatbot/widget/chatbot-widget.component.ts"

echo "\nImplementation in exercise-chatbot-button.component.ts:"
rg -A 5 "isChatGptWrapper" "src/main/webapp/app/iris/exercise-chatbot/exercise-chatbot-button.component.ts"

echo "\nImplementation in iris-base-chatbot.component.ts:"
rg -A 5 "isChatGptWrapper" "src/main/webapp/app/iris/base-chatbot/iris-base-chatbot.component.ts"

# Check HTML template usage
echo "\nHTML usage patterns:"
rg -A 2 "isChatGptWrapper" "src/main/webapp/app/iris/**/*.html"

Length of output: 1694


Script:

#!/bin/bash
# Check HTML template bindings

echo "HTML template usage in base component:"
rg -A 2 "isChatGptWrapper" "src/main/webapp/app/iris/base-chatbot/iris-base-chatbot.component.html"

echo "\nHTML template usage in widget component:"
rg -A 2 "isChatGptWrapper" "src/main/webapp/app/iris/exercise-chatbot/widget/chatbot-widget.component.html"

echo "\nHTML template usage in button component:"
rg -A 2 "isChatGptWrapper" "src/main/webapp/app/iris/exercise-chatbot/exercise-chatbot-button.component.html"

Length of output: 1647

@krusche krusche changed the title Iris: Special hacks for ICER 2025 Paper Iris: Add a temporary ChatGPT interface for specific user groups and exercises Jan 21, 2025
@krusche krusche added this to the 7.9.0 milestone Jan 21, 2025
krusche
krusche previously approved these changes Jan 21, 2025
Copy link
Member

@krusche krusche left a comment

Choose a reason for hiding this comment

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

While this is not the most beautiful code ;-) it should do the trick in the experiment 👍

coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 21, 2025
@bassner bassner dismissed stale reviews from coderabbitai[bot] and krusche via 2f2a3f9 January 21, 2025 21:28
coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 21, 2025
@bassner bassner merged commit 67bd6e7 into develop Jan 21, 2025
23 of 27 checks passed
@bassner bassner deleted the feature/icer-2025-paper branch January 21, 2025 22:06
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!) core Pull requests that affect the corresponding module deployment-error Added by deployment workflows if an error occured exercise Pull requests that affect the corresponding module iris Pull requests that affect the corresponding module server Pull requests that update Java code. (Added Automatically!) tests
Projects
Status: Merged
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants