Generate game report after the game ends. #176

zpChris · 2021-06-30T03:35:50Z

List of Changes:

Add createGameReport method within the GameManagementService.
Trigger the creation of the game report through CreateGameReportTask one minute after the game ends (to ensure all submissions are recorded).
Update the deleteProblem method to remove all room_problems and problem_containers references.
Add tests for the CreateGameReportTask.
Add two service tests (one large test) for the createGameReport method.

Notes:

Should I add a test within GameTests.java to check that the game report is created, or are the service and task tests enough @alankbi?
The game report save functionality is set to run a minute after the game ends - this is because a submission could run immediately after the game ends and take up to forty-five seconds to see a response. I rounded that time up to a minute, and created a timer task to wait that amount of time before calling createGameReport.
- Note: The Tester repository does not allow a method to run longer than ten seconds, whereas the main repository only times out the method after forty-five seconds. I think it'd make sense to change one of those values, either allow the method to run longer or have the method time-out sooner - I could potentially then create the game report faster, which could be nice as it'd allow the teacher to go immediately from the results page to the game report after it loads. (There could be a loading bar at the top that would approximate how long that process takes.) Thoughts?
The deleteProblem method can now take a very long time, depending on the references in the room_problems table (the selected problems in rooms) and the references with the problem_containers (the problem with statistics in the game report). Let me know if you think we should give some sort of ongoing progress update on the frontend, or just a note that the delete process will take a while, or if we should just redirect them immediately.

Screencast and Images:

Screencast of Create Game Report in Database

New Database Model with Game Report

…eReport

alankbi · 2021-07-07T03:43:59Z

Looks like there are some commit issues similar to what was on #174 so I'll hold off on reviewing for now - however, after #172 was merged those commit issues went away for #174 so I'd imagine this will get resolved pretty quickly (as soon as #174 is merged)

zpChris · 2021-07-08T08:01:07Z

Looks like there are some commit issues similar to what was on #174 so I'll hold off on reviewing for now - however, after #172 was merged those commit issues went away for #174 so I'd imagine this will get resolved pretty quickly (as soon as #174 is merged)

I'll resolve all the merge conflicts at once after #174 is merged, since I just approved that and #178.

alankbi · 2021-07-09T03:37:59Z

Looks like the commits are fixed, but maybe not the line numbers 😮 hopefully a merge will fix that tho, in which case will try to review this quickly (sorry for the delay)

alankbi

From a quick look, this looks great! Just leaving some preliminary comments since the bulk of changes seem to be in the GameManagementService file, and that way once you fix the merge conflicts/lines changed hopefully you won't be completely blocked waiting on a review

alankbi · 2021-07-10T02:23:43Z

src/main/java/com/codejoust/main/service/GameManagementService.java

+        createGameReportTimer.schedule(createGameReportTask, GameTimer.DURATION_1 * 1000);
+    }
+
+    public GameReport createGameReport(Game game) {


nit: move this + other report logic into a new ReportService

Sounds good - done! ✅

When I created the new ReportService class, I also created the new ReportServiceTests class; in the original tests, I also used a Spy for GameManagementService not just for the createGameReport method, but also to add submissions and add the game from the room. Is it ok that I have Spy objects for both GameManagementService and ReportService within that file, or should I find some workaround?

(I ask because this change makes the ReportServiceTests dependent not only on ReportService, but also on the GameManagementService - so it's not entirely isolated, but the work to get there may be large.) Thoughts @alankbi?

Does this cause any issues? Never tried that before, but I guess if it works then it hopefully shouldn't be an issue

It doesn't cause any issues, just makes the code styling arguably a bit worse. In the interest of keeping progress moving given how long this PR has been out, I'll merge this change in though, if that's alright. 👍

src/main/java/com/codejoust/main/service/GameManagementService.java

alankbi · 2021-07-10T02:36:07Z

src/main/java/com/codejoust/main/service/GameManagementService.java

+            numTestCases += problem.getTestCases().size();
+
+            ProblemContainer problemContainer = new ProblemContainer();
+            problemContainer.setProblem(problem);


What will happen if a problem is deleted in the middle of a game? I believe the problem will still exist in the game object since it's in the backend, but what will happen here?

This is a good point! You're right - I believe the problem will still exist in the game object, but it will not exist in the database. The following error thus exists.

Problem Deleted During Game

I therefore want to make the change to check if the problem does not exist in the database, then I want to set the problem within the ProblemContainer as null. I'm adding this, and I'm also going to do this right before I save the game report.

There is still a chance, I believe, that while looping through the problems, one of the problems is deleted after it is already visited. I'm not sure what to do about conflicts like this - I think that relates to the issue we've had elsewhere of simultaneous SQL updates.

I will note that after I implemented this change, it worked, but still produced the below error in the Terminal; I'm not sure whether this is something that I should be concerned about or not.

Terminal Problem Not Found Error

Does this error still happen? If so, I wonder if it has to do with one of the timer tasks not canceling properly? If it doesn't affect anything then should hopefully be fine, assuming that it only shows up in the edge case of deleting a problem

…ing tests.

…problems during game.

…ing after the game ends.

alankbi

Looks good! Mainly just left a few questions here and there. Also, another potential suggestion would be to add a new README under model/report diagraming/explaining how all the report model classes fit together (since it can be confusing), but not a huge deal

src/main/java/com/codejoust/main/model/Account.java

alankbi · 2021-08-04T01:32:23Z

src/main/java/com/codejoust/main/model/User.java

    private List<SubmissionGroupReport> submissionGroupReports = new ArrayList<>();
+
+    public void addSubmissionGroupReport(SubmissionGroupReport submissionGroupReport) {
+        submissionGroupReports.add(submissionGroupReport);


Just to double-check, this isn't included in the UserDto right? Since it's only used for the report related stuff?

That's right - this is not included in the standard UserDto class, nor will I add it there. I don't yet have a GameReportDto class as I've been working in the backend thus far, but I will add that at some point - I'll make another DTO class for the User object specifically for the report purposes, so it is only passed to the frontend when necessary.

alankbi · 2021-08-04T01:41:35Z

src/main/java/com/codejoust/main/service/ProblemService.java

+        for (ProblemContainer problemContainer : problemContainers) {
+            problemContainer.setProblem(null);
+            problemContainerRepository.save(problemContainer);
+        }


How long does this take to run? I can imagine at some point in the future we might need ways to optimize this process it if takes too long

You're right, this can take a very long time - I agree, some method to optimize this would be very useful. This is especially a problem because after clicking the "Delete Problem" button, the page does not redirect until the REST call returns. I think we could address this issue in a future PR, though I'll leave a few ideas here (not mutually exclusive):

Add a boolean deletionInProgress field that allows us to show a notice that the problem is currently being deleted on the frontend.

Redirect immediately after the call is made rather than waiting for the return.

Have the function return success if it parses through everything, even if it has not finished deleting the problem - this may require having the for loop above continue in a separate thread or something, which I'm fairly certain is possible though I don't know how that would really work.

Somehow speed up the actual deletion process.

Remove the problem relation and replace it with a problemId, then simply replace those with null whenever a reference is attempted and not found.

alankbi · 2021-08-04T01:53:38Z

src/main/java/com/codejoust/main/service/SubmitService.java

+            }
        }

+        // TODO: Will this be an issue if the submission is not actually counted? Should I return null in that case?


not sure - if I remember correctly this value isn't sent via socket to the frontend right? If so (if it's just the game to being sent), then I'm guessing this should be fine, but idk

alankbi · 2021-08-04T01:58:03Z

src/test/java/com/codejoust/main/service/ReportServiceTests.java

+        room.addUser(user4);
+        room.setHost(user1);
+        Problem problem1 = TestFields.problem1();
+        Problem problem2 = TestFields.problem2();


nit: add these to TestFields as user1, user2, etc. and also potentially a room1 method (if this room is good enough to be reused in other tests)

zpChris added 30 commits June 17, 2021 17:38

Update the timer task setup.

f296930

Add createGameReport method and update corresponding data models.

a2164a3

Resolve merge conflicts with ResultSetup and main.

bbc04bd

Add TODO and helper methods.

5282463

Merge branch 'main' of https://github.com/rocketden/main into Generat…

e25a919

…eReport

Merge with multi-problem UI.

8c0ab51

Add logic to set data in submission report.

ddc34c1

Add problem container variable and remaining game report statistics.

54d81fe

Delay before creating the game report.

7f5d22a

Add game report repository and save the object in the database.

5530121

Update the saving functionality for users and accounts.

913cf38

Update column definitions to text.

15aa1ed

Move utility tasks to the task folder.

cc3a0d4

Add the timer task for creating a game report.

56a73ec

Fix average test cases passed and total number of test cases.

0fbd919

Remove old logs.

ed5c3da

Merge branch 'main' of https://github.com/rocketden/main into Generat…

18e1f1c

…eReport

Merge branch 'main' of https://github.com/rocketden/main into Generat…

0311bec

…eReport

Add file for creating game report.

22eff98

Add mockito annotations calls to fix methods, remove unnecessary checks.

56c580a

Remove unnecessary tests and add check for create game report.

0ae5741

Update the game management service tests.

ee95904

Remove unused variables.

54795e5

Add verification of the relevant game report objects.

3433138

Update the submission helper to have only one test case.

9dcabee

Add the overall problems solved and test cases passed statistics.

4325c79

Add a second incorrect attempt that factors only into attempt count.

71f715c

Add final test statistics and add explanatory top-level comment.

f43d8ce

Add the create two game reports test.

b22927e

Add final test for game end type.

832341a

Remove unnecessary user repository save.

ed98efb

zpChris added 2 commits July 6, 2021 23:41

Add problem service delete method.

6cf7dd8

Remove extraneous comment.

bd62778

alankbi mentioned this pull request Jul 9, 2021

Integrate multiple problems into existing flow #174

Merged

Base automatically changed from MultiProblemUI to main July 9, 2021 03:25

alankbi reviewed Jul 10, 2021

View reviewed changes

zpChris added 10 commits July 20, 2021 20:04

Resolve merge conflicts with main.

c972883

Create the new ReportService as well as modified the other correspond…

da88286

…ing tests.

Add problem repository to the report service to take care of deleted …

dabe142

…problems during game.

Update submit service functionality to prevent submissions from occur…

8ad55e7

…ing after the game ends.

Remove CreateGameReportTask class and call createGameReport directly.

7f52b25

Fix reportService tests.

056dd07

Update the removeProblem test API.

f3dc868

Attempt initial implementation of createGameReportStarted.

fe0bd95

Test problem deletion in the ReportServiceTests class.

72dd0a6

Split problem container and account additions to separate methods.

87d39b1

zpChris requested a review from alankbi August 2, 2021 01:13

zpChris added 2 commits August 1, 2021 18:16

Further split up the createGameReport method into different methods.

c743cfa

Remove blank lines in createGameReport method.

2705ac5

alankbi approved these changes Aug 4, 2021

View reviewed changes

Generate game report after the game ends. #176

Are you sure you want to change the base?

Generate game report after the game ends. #176

Uh oh!

Conversation

zpChris commented Jun 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

List of Changes:

Notes:

Screencast and Images:

Uh oh!

alankbi commented Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zpChris commented Jul 8, 2021

Uh oh!

alankbi commented Jul 9, 2021

Uh oh!

alankbi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alankbi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zpChris Aug 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zpChris commented Jun 30, 2021 •

edited

Loading

alankbi commented Jul 7, 2021 •

edited

Loading

zpChris Aug 9, 2021 •

edited

Loading