-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
[#11402] Submission status stays as pending for team submissions #11518
[#11402] Submission status stays as pending for team submissions #11518
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, just some minor comments
if (allQuestions.isEmpty()) { | ||
return true; | ||
} | ||
Boolean isAllTeamQuestions = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can use the primitive type boolean
here
student = dataBundle.students.get("student5InCourse1"); | ||
assertFalse(fsLogic.isFeedbackSessionCompletedByStudent(fs, student.getEmail(), student.getTeam())); | ||
|
||
______TS("success: second feedback session (both team and individual questions"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing a close bracket questions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments.
You can also help to deprecate fqLogic.sessionHasQuestions
if you have removed the only place it is used! The old way of checking this seems irrelevant.
@@ -237,15 +238,33 @@ public boolean isFeedbackSessionHasQuestionForStudents( | |||
* | |||
* <p> If there is no question for students, the feedback session is completed</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to update the logic in the javadoc writeup:)
return frLogic.hasGiverRespondedForSession(userTeam, feedbackSessionName, courseId); | ||
} else { | ||
// if there are individual questions, session is complete only if | ||
// the student has responded to the individual questions | ||
return frLogic.hasGiverRespondedForSession(userEmail, feedbackSessionName, courseId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The userEmail
param in hasGiverRespondedForSession
seems overloaded with userTeam
now. Do you have a better way of phrasing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a more accurate name for userEmail
param in hasGiverRespondedForSession
is giverIdentifier
since from what I understand, email is used for individual responses but team name is used for team responses. It will be good to probably change the giverEmail
column in FeedbackResponse
entity to giverIdentifier
also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me. Let's see how it looks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be good to probably change the
giverEmail
column inFeedbackResponse
entity togiverIdentifier
also.
I think this might not be backward compatible... An alternative way would be to get all students in the team and return true
if at least one of the students in the team has responded for the session.
@moziliar what do you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry, I missed out the part about changing the entity field... @jianhandev you are right. Let's keep the entity untouched. We should only touch the code logic.
Changing the userEmail
param to giverIdentifier
or something similar will do. The problem is minor in that a userTeam
is given as userEmail
param.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the userEmail param to giverIdentifier or something similar will do.
Ok sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am good with the change in code. However, the addition of entity reads makes raise my eyebrows a little. @damithc is it ok?
@Deprecated | ||
public boolean sessionHasQuestions(String feedbackSessionName, String courseId) { | ||
return fqDb.hasFeedbackQuestionsForGiverType(feedbackSessionName, courseId, FeedbackParticipantType.STUDENTS) | ||
|| fqDb.hasFeedbackQuestionsForGiverType(feedbackSessionName, courseId, FeedbackParticipantType.TEAMS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can just remove this altogether since it is not used anywhere after this change.
String courseId = fsa.getCourseId(); | ||
|
||
List<FeedbackQuestionAttributes> allQuestions = | ||
fqLogic.getFeedbackQuestionsForStudents(feedbackSessionName, courseId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might actually incur many data read. @damithc can we evaluate the volume of such call?
Before this change, this execution flow only performs key only queries (in frLogic.hasGiverRespondedForSession
and fqLogic.sessionHasQuestions
). This solution is clean if adding back entity reads will not incur any concern on the cost.
From the logs, I don't see it though. Perhaps it is about the period.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have refactored to only perform key-only queries.
List<StudentAttributes> studentsToRemindList = new ArrayList<>(); | ||
for (StudentAttributes student : studentList) { | ||
if (!logic.isFeedbackSessionCompletedByStudent(session, student.getEmail())) { | ||
if (!logic.isFeedbackSessionCompletedByStudent(session, student.getEmail(), student.getTeam())) { | ||
studentsToRemindList.add(student); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can try to use stream for this part of code:)
List<StudentAttributes> studentsToRemindList = studentList.stream().filter(student ->
!logic.isFeedbackSessionCompletedByStudent(session, student.getEmail())
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use of stream is encouraged, as it presents a cleaner, possibly immutable semantics. The use of filter
means that we are getting the same set of data, possibly less, instead of potentially doing something to the data.
if (isAllTeamQuestions) { | ||
return frLogic.hasGiverRespondedForSession(userTeam, feedbackSessionName, courseId); | ||
} else { | ||
return frLogic.hasGiverRespondedForSession(userEmail, feedbackSessionName, courseId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we trace hasGiverRespondedForSession
until the DB layer, in FeedbackResponseDb.java
, the hasResponsesFromGiverInSession
method filters the responses by giverEmail
. Passing the team name to hasResponsesFromGiverInSession
does not seem to make sense as giverEmail
is different from team name.
teammates/src/main/java/teammates/storage/api/FeedbackResponsesDb.java
Lines 175 to 189 in d4dbaa2
public boolean hasResponsesFromGiverInSession( | |
String giverEmail, String feedbackSessionName, String courseId) { | |
assert giverEmail != null; | |
assert feedbackSessionName != null; | |
assert courseId != null; | |
return !load() | |
.filter("giverEmail =", giverEmail) | |
.filter("feedbackSessionName =", feedbackSessionName) | |
.filter("courseId =", courseId) | |
.limit(1) | |
.keys() // key query is free query | |
.list() | |
.isEmpty(); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea but from what I understand for team questions, the team name is what is actually stored in the db under giverEmail
, which is why I initially suggest renaming the entity field. But I do agree there will prob be backward compatibility issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The entity field is unfortunately giverEmail
, possibly due to legacy issue. However, FeedbackResponseAttribute
class has a more neutral field called giver
with brief explanation about the nature of the field. We can also put some remark in the form of comment in the FeedbackResponse
class to indicate this the same way the Attribute class does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The gitblame shows that the entity class is created back in 2013, and the renaming of giverEmail
to giver
in the Attribute class was performed in 2016 by PR #5547.
I am ok with renaming in code logic and adding comments in entity class to reflect this ambiguity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Alternatively) we could try fetching all students in the team and for each student check if the student has responded using hasResponsesFromGiverInSession
, return true if at least one of the students in the team has responded for the session, but this will incur more DB reads I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea that does seem a little expensive and will require storing the student email instead of the team name for team question responses.
We can also put some remark in the form of comment in the FeedbackResponse class to indicate this the same way the Attribute class does.
Personally I think this should be sufficient for now unless we need to track which team member answered the team questions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slowly I feel like the cost might not justify the improvement of the UX as we moved away from key only query to reading many entities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat. LGTM.
@jianhandev take a final look? Am good with the kind of additional read (1 entity max) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM too, although you have written tests to verify that it works, which should be sufficient in this case, but it'll be even better if you can provide a few screenshots of the changes in the PR description as well.
/** | ||
* {@code giverEmail} does not necessarily contain an email. Depending on the question giver type, | ||
* it may contain the giver's email, the team name, "anonymous", etc. | ||
*/ | ||
private String giverEmail; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to change giverEmail
to giver
as well in the future, but maybe not now
…ions (TEAMMATES#11518) * Fixed team questions submission stauts bug. * Updated tests. * Fixed linting. * Minor fixes. * Add javadoc and minor refactoring. * Refactor code to use key-only db query. * Fix linting. Co-authored-by: Ahmed Bahajjaj <42177597+madanalogy@users.noreply.github.com>
Fixes #11402
Outline of Solution
As discussed with @damithc under the issue: