[#11571] Cascade user updates/deletes to deadline maps

[jayasting98]

Part of #11571

PR Checklist

Outline of Solution

Any user email address update or any user deletion is cascaded down to the deadline maps. This is implemented by finding all the feedback sessions in the course the user belongs to. For each feedback session, the email is updated or deleted accordingly in the respective deadline map if that map contains a selective deadline for that user.

[samuelfangjw]

For the FeedbackSessionLogic

1. Let's group the delete methods and update methods together.
2. I don't think we need so many methods for a simple update. The current implementation seems unnecessarily complex for this purpose.

I think something like this will be more suitable. I've tried to write it in your style.

    public void deleteFeedbackSessionsDeadlinesForUser(String courseId, String email, boolean isInstructor) {
        Consumer<Map<String, Instant>> deadlineUpdater = deadlines -> deadlines.remove(email);

        updateFeedbackSessionDeadlinesForUser(courseId, email, isInstructor, deadlineUpdater);
    }

    public void updateFeedbackSessionDeadlinesWithNewEmail(String courseId, String oldEmail, String newEmail, boolean isInstructor) {
        if (oldEmail.equals(newEmail)) {
            return;
        }

        Consumer<Map<String, Instant>> deadlineUpdater = deadlines -> deadlines.put(newEmail, deadlines.remove(oldEmail));

        updateFeedbackSessionDeadlinesForUser(courseId, oldEmail, isInstructor, deadlineUpdater);
    }

    private void updateFeedbackSessionDeadlinesForUser(String courseId, String email,
            boolean isInstructor, Consumer<Map<String, Instant>> deadlineUpdater) {
        List<FeedbackSessionAttributes> feedbackSessions = fsDb.getFeedbackSessionsForCourse(courseId);
        for (var session : feedbackSessions) {
            Map<String, Instant> studentDeadlines = session.getStudentDeadlines();
            Map<String, Instant> instructorDeadlines = session.getInstructorDeadlines();
            if (isInstructor && instructorDeadlines.containsKey(email)) {
                deadlineUpdater.accept(instructorDeadlines);
            } else if (!isInstructor && studentDeadlines.containsKey(email)) {
                deadlineUpdater.accept(studentDeadlines);
            } else {
                // No deadline found in session
                continue;
            }
            try {
                fsDb.updateFeedbackSession(FeedbackSessionAttributes
                        .updateOptionsBuilder(session.getFeedbackSessionName(), courseId)
                        .withStudentDeadlines(studentDeadlines)
                        .withInstructorDeadlines(instructorDeadlines)
                        .build());
            } catch (InvalidParametersException | EntityDoesNotExistException e) {
                // Both Exceptions should not be thrown
                log.severe("Unexpected error", e);
            }
        }
    }

[samuelfangjw]

For the test json files, let's add the corresponding DeadlineExtensions as well.

[jayasting98]

@samuelfangjw I initially did it similarly to how you suggested, also to mirror your implementation for cascading for the deadline extension entity since it seemed pretty good. However, I felt that for your case, the reason why cascading to the entity can be implemented that way is because isInstructor is actually a property of the entity. On the other hand, in my case, all it was doing was changing the behaviour of the method, so I thought that meant isInstructor would be a flag argument. I decided to split it into different methods instead to avoid that. What are your thoughts on this?

Regardless, I think for right now, I will change deadlinesUpdater into a Consumer like you suggested because that seems much better.

Edit:

Also the log.severe

[samuelfangjw]

@samuelfangjw I initially did it similarly to how you suggested, also to mirror your implementation for cascading for the deadline extension entity since it seemed pretty good. However, I felt that for your case, the reason why cascading to the entity can be implemented that way is because isInstructor is actually a property of the entity. On the other hand, in my case, all it was doing was changing the behaviour of the method, so I thought that meant isInstructor would be a flag argument. I decided to split it into different methods instead to avoid that. What are your thoughts on this?

If you can do it in a way that doesn't sacrifice readability I don't see any issues.

My personal thoughts are that we should have a pragmatic approach to refactoring and make sure any "rules" we apply actually makes sense in the context.

[jayasting98]

Ok, I think if I cannot make it readable enough, I might consider putting your stuff as the underlying private methods instead.

[moziliar]

Perhaps you are looking for Collectors.GroupingBy?

[jayasting98]

Oh yes that would be much better; I did not know it existed. Thanks.

[moziliar]

Maybe the test case name can describe the input and the output, say updateInstructorEmail_transferDeadlinesToNewEmail?

[jayasting98]

I think the test method name gets a bit too long if I add it to the end:

public void testUpdateFeedbackSessionsInstructorDeadlinesWithNewEmail_updateInstructorEmail_transferDeadlinesToNewEmail()

Or do you mean replacing it completely? Wouldn't this be unconventional?

public void testUpdateInstructorEmail_transferDeadlinesToNewEmail()

Or how about using something like ______TS("Update instructor email; transfers deadlines to new email.")?

[moziliar]

I think TS will do. Indeed it isn't too conventional to just replace as such.

[moziliar]

ditto

[moziliar]

ditto?

[moziliar]

Can consider move to after line 819?

[moziliar]

A few more places like this are unclear. Can apply the same fix here

[jayasting98]

Ah sorry, I forgot I did it in other files too.

[moziliar]

LGTM! I'm ok with the rest of the changes!

[halfwhole]

Looks good!