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

Development: Fix team students eager loading for several occasions #7993

Merged
merged 36 commits into from
Mar 31, 2024

Conversation

julian-christl
Copy link
Member

@julian-christl julian-christl commented Feb 2, 2024

Checklist

General

Server

  • Important: I implemented the changes with a very good performance and prevented too many (unnecessary) database calls.
  • I followed the coding and design guidelines.
  • I documented the Java code using JavaDoc style.

Changes affecting Programming Exercises

  • High priority: I tested all changes and their related features with all corresponding user types on a test server configured with the integrated lifecycle setup (LocalVC and LocalCI).
  • I tested all changes and their related features with all corresponding user types on a test server configured with Gitlab and Jenkins.
  • Removed Atlassian Stack as it's deprecated

Motivation and Context

After issues with #7829 and some hot fixes, I went through the whole code base to find any occurrence where loading team students might be necessary. It took some time, but I was able to go through most branches. There were a few that I had to skip because they were just too complex, but I added an initialisation on these instances, thus they are still covered.

Description

Eagerly loaded team students wherever they are called. In some areas I maintained the relations after saving by overriding them afterwards. Some queries I had to extract as participation.team.students won't work on the entity graph because team is on the student participation, not the abstract one. In some sections I load the students if not initialised. Usually because it would be too much effort to go through all call branches and make sure that every one of them loads the students for team exercises. Finally I also modified some sections to reduce database calls and improved some queries.

Steps for Testing

Prerequisites:

  • 1 Instructor
  • 2 Students in a team
  • 1 Exercise of each type as team exercise
  1. Ensure that the team administration (create, update, etc) is fully working. You can check this in the test preparation already.
  2. Do all steps of the participation (submit, assess, complain, answer complain) and make sure that for every step you check all team views (i.e. the view where team members are listed).
  3. Make sure that the scores are displayed correctly at all times for students and instructor alike
  4. Make sure that when updating the due date of programming exercises, the repositories get locked/unlocked correctly
  5. Make sure that the websocket events work especially for the students and hence the values get updated. If you're not sure what to expect, a good suggestion would be to monitor sentry during your tests and make sure that there are no errors from your server related to this PR (especially initialization errors) https://sentry.ase.in.tum.de/organizations/artemis/issues/?environment=test&project=2&statsPeriod=1h

Exam Mode Testing

Not required, exams don't support team exercises.

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 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

No functional changes really affecting the coverage. If you really need the coverage, let me know and I'll add it.

Summary by CodeRabbit

  • Refactor
    • Improved efficiency in fetching related entities for exercises.
    • Enhanced submission query methods for better data handling and added a new method for fetching specific submission associations.

@krusche krusche modified the milestones: 6.8.0, 6.8.1 Feb 4, 2024
@krusche
Copy link
Member

krusche commented Feb 4, 2024

I am not sure if the PR follows the right approach. It seems we load team and students way too often now. I guess it would be better to only load the team with students, when we actually have a team exercise

@krusche krusche removed this from the 6.8.1 milestone Feb 4, 2024
@krusche
Copy link
Member

krusche commented Feb 4, 2024

I made sure that the hot fix in the release branch release/6.7.7 was merged into develop to avoid a regression in 6.8.0

@julian-christl
Copy link
Member Author

julian-christl commented Feb 20, 2024

I am not sure if the PR follows the right approach. It seems we load team and students way too often now. I guess it would be better to only load the team with students, when we actually have a team exercise

@krusche
So do I get you right that you would prefer having two different repository methods for most of these methods and call the corresponding one depending on whether exercise.isTeamMode() resolves to false or true?
I personally don't see the benefit. If it's not a team exercise, it won't get loaded anyway. On the opposite, it would create the overload of creating many more methods and force us adapting many method signatures to pass down the information.

Or did I misinterpret your intention here?

mateusmm01

This comment was marked as resolved.

Copy link
Contributor

@mateusmm01 mateusmm01 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 ts5 and works as expected 🚀

Copy link
Contributor

@rstief rstief 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
Contributor

@basak-akan basak-akan left a comment

Choose a reason for hiding this comment

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

Re approve

Copy link
Contributor

@undernagruzez undernagruzez left a comment

Choose a reason for hiding this comment

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

Re approve

@julian-christl julian-christl added this to the 7.0.0 milestone Mar 31, 2024
@krusche krusche merged commit 7a401e8 into develop Mar 31, 2024
30 of 38 checks passed
@krusche krusche deleted the bugfix/fix-team-lazy-loading branch March 31, 2024 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:Team ready to merge server Pull requests that update Java code. (Added Automatically!) tests too-long-open !!! This is an antipattern, we should aim for small PRs that are only open for a short time !!!
Projects
Archived in project
Artemis Development
  
In progress
Development

Successfully merging this pull request may close these issues.

None yet

9 participants