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: Answer questions about lecture slides #8636

Merged
merged 131 commits into from
Jun 22, 2024

Conversation

yassinsws
Copy link
Contributor

@yassinsws yassinsws commented May 20, 2024

Checklist

General

Server

  • Important: I implemented the changes with a very good performance and prevented too many (unnecessary) database calls.
  • I strictly followed the server coding and design guidelines.
  • I added multiple integration tests (Spring) related to the features (with a high test coverage).
  • I added pre-authorization annotations according to the guidelines and checked the course groups for all new REST Calls (security).
  • I documented the Java code using JavaDoc style.

Motivation and Context

We want Iris to be able to respond to the students questions based on the lecture content. For that we need to send the lectures to Pyris, everytime new lecture slides or attachments get updated or deleted.

Description

Every lecture that gets updated or created is sent to the Pyris through a rest call. If a course, lecture or a lectureUnit gets erased the webhook is also executed and the files are erased from the Pyris side.

Steps for Testing

Prerequisites:

Test as an admin(only people with admin rights can test on ts3):

  1. Log in to Artemis as an Admin
  2. Go to the course page
  3. Upload a lecture unit attachment with a pdf file in it-> Nothing should happen here on Pyris side
  4. Now Enable in Iris global settings and course, lecture ingestion and enable Auto Lecture ingestion.

Test as an INSTRUCTOR/Admin:
5. Download a lecture unit attachment then Reupload it -> Check if the file is sent correctly to Pyris by checking Artemis logs on Grafana. After some time you should find this log: Slides ingestion:Lecture Ingestion Finished.
6. Delete a lecture unit, delete a Lecture -> Check if the file is sent correctly to Pyris by checking the Artemis logs on Grafana after some time you should find this log: Old slides removal:Old slides removed.
7. On the lectures page there is a button called send lectures to Pyris near create lectures, when you click on it, the upload should work as expected. After some time you should find this log: Slides ingestion:Lecture Ingestion Finished.

Testserver States

Note

These badges show the state of the test servers.
Green = Currently available, Red = Currently locked

Review Progress

Performance Review

  • I (as a reviewer) confirm that the client changes (in particular related to REST calls and UI responsiveness) are implemented with a very good performance
  • I (as a reviewer) confirm that the server changes (in particular related to database calls) are implemented with a very good performance

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Test Coverage

Client

Class/File Line Coverage Confirmation (assert/expect)
iris-common-sub-settings-update.component.ts 93.18% ✅ ❌
iris-enabled.component.ts 94.28% ✅ ❌

Server

Class/File Line Coverage Confirmation (assert/expect)
IrisCourseSettings.java 97%
IrisGlobalSettings.java 97%
AttachmentUnitService.java 95%
LectureService.java 95%
PyrisJobService.java 95%
PyrisStatusUpdateService.java 91%
PyrisWebhookService.java 94%
IrisCombinedLectureIngestionSubSettingsDTO.java 100%
IrisCombinedSettingsDTO.java 100%
IrisSubSettingsService.java 89%
LectureResource.java 93%

Summary by CodeRabbit

  • New Features

    • Added functionality to handle lecture ingestion in Pyris, including automatic updates and deletions of attachments.
    • Integrated new settings to manage lecture ingestion in Iris interface.
  • Enhancements

    • Improved lecture ingestion processes with Pyris integration.
    • Updated language files for multilingual support, including new labels and settings.
  • Bug Fixes

    • Corrected handling of lecture ingestion settings in various components.
  • Tests

    • Added comprehensive test cases for new lecture ingestion functionalities and settings management.
  • Documentation

    • Updated language files to include new key-value pairs for lecture ingestion settings.

@yassinsws yassinsws requested a review from a team as a code owner May 20, 2024 09:45
@github-actions github-actions bot added tests server Pull requests that update Java code. (Added Automatically!) client Pull requests that update TypeScript code. (Added Automatically!) database Pull requests that update the database. (Added Automatically!). Require a CRITICAL deployment. labels May 20, 2024
coderabbitai[bot]

This comment was marked as outdated.

coderabbitai[bot]
coderabbitai bot previously approved these changes May 20, 2024
@yassinsws yassinsws changed the title Feature/iris/final ingestion branch Feature/iris/Lecture Ingestion Pipeline May 20, 2024
coderabbitai[bot]
coderabbitai bot previously approved these changes May 20, 2024
Hialus

This comment was marked as outdated.

@bassner bassner changed the title Feature/iris/Lecture Ingestion Pipeline Iris: Add lecture ingestion pipeline May 26, 2024
coderabbitai[bot]

This comment was marked as outdated.

yassinsws and others added 5 commits May 27, 2024 14:05
…odel.ts

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…GlobalSettings.java

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Copy link
Contributor

@Jan-Thurner Jan-Thurner left a comment

Choose a reason for hiding this comment

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

reapprove

@krusche krusche modified the milestones: 7.2.2, 7.2.3 Jun 18, 2024
Copy link
Contributor

@egekurt123 egekurt123 left a comment

Choose a reason for hiding this comment

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

Reapprove

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

Comment on lines +159 to +161
* @return returns the job token if the operation is successful else it returns null
*/
public boolean ingestLecturesInPyris(Set<Lecture> lectures) {
Copy link
Member

Choose a reason for hiding this comment

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

a boolean is not a job token

}
}
catch (Exception e) {
log.error(e.getMessage());
Copy link
Member

Choose a reason for hiding this comment

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

We should probably do something here when we add a status update to the UI

@krusche krusche changed the title Iris: Add lecture ingestion pipeline Iris: Allow students to answer questions on lecture slides Jun 21, 2024
@bassner bassner changed the title Iris: Allow students to answer questions on lecture slides Iris: Allow Iris to answer questions about lecture slides Jun 21, 2024
@krusche krusche changed the title Iris: Allow Iris to answer questions about lecture slides Iris: Answer questions about lecture slides Jun 22, 2024
@krusche krusche merged commit 67df5f2 into develop Jun 22, 2024
25 of 33 checks passed
@krusche krusche deleted the feature/iris/FinalIngestionBranch branch June 22, 2024 09:46
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!) database Pull requests that update the database. (Added Automatically!). Require a CRITICAL deployment. ready to merge server Pull requests that update Java code. (Added Automatically!) tests
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.