Closed damithc closed 10 years ago
From dam...@gmail.com on April 14, 2014 03:08:07
Winson, do this one instead. It is a bit more urgent. Remember to add a test case.
Owner: winsonta...@gmail.com
Cc: -shrianu...@gmail.com
Labels: Priority-Medium Reviewer-James
From winsonta...@gmail.com on April 15, 2014 02:10:00
[Summary] I hid the mailto using display:none if the email is the default value for when recipient is nobody specific.
Also, I did not do the assertion .contains("@") because when the recipient is a team, the email does not contain "@" (the email is the team name).
For the test, I added a response for InstructorFeedbackResultsPageUiTest for nobody specific. https://codereview.appspot.com/87910044/ [Issue reporting] On a side note, I also tried adding a response for when giver and recipient is a team, for the InstructorFeedbackResultsPageUiTest, in accordance with the template in typicalDataBundle.json. However, it doesn't appear in the InstructorFeedbackResultsPage. Im not sure if this is a mistake on my part as I'm not familiar with the requirements of adding a response in the json. I've removed this change in the submitted to-be-reviewed code.
Status: ReadyForReview
From dam...@gmail.com on April 15, 2014 02:14:18
When the recipient is a team, we should not show the mailto link right?
From winsonta...@gmail.com on April 15, 2014 06:40:44
Oh, you're right, for the sort by recipient, currently we show an invalid mailto link, but I think we should hide them.
For the sort by giver, we show something like:
From: Team 1 [student2inTeam1@gmail.com's Team],
so I thought it was fine, but I have just realized the email address contains even the "...'s Team" part. So for this case, do you want me to
Another thing is if I'm to implement this, I have a question regarding the test case involved. As I mentioned, I cannot seem to add a response for when giver and recipient is a team, even though I add this at the end of InstructorFeedbackResultsPageUiTest.json: "response14":{ "feedbackSessionName":"First Session", "courseId":"CFResultsUiT.CS2104", "feedbackQuestionId":"4", "feedbackQuestionType":"TEXT", "giverEmail":"CFResultsUiT.alice.b@gmail.com", "recipientEmail":"Team 2", "responseMetaData":{ "value":"Response from team 1 (by alice) to team 2." } },
From dam...@gmail.com on April 15, 2014 07:01:18
For the first question (i.e. when sorted by 'giver'), I prefer to keep the fix the email rather than hide it. For the second question, let's see if Arnold has any tips.
From winsonta...@gmail.com on April 16, 2014 10:07:57
Just to update, I've implemented it, with minor refactoring, and manually tested it. All tests pass, but I haven't written any test cases yet for the reason I mentioned earlier.
And can I change the status to ChangesRequested? because it's not ready to review.
Status: ChangesRequested
From dam...@gmail.com on April 16, 2014 21:24:23
Try to complete the tests. See if other json files have data that fits your scenario so that you can figure out what data to put in the json file.
From winsonta...@gmail.com on April 17, 2014 03:43:33
Dr Damith, thanks, that helps. I found out that the reason the response does not show up is because the question is not visible to instructors. I should come up with the patch tonight.
From winsonta...@gmail.com on April 17, 2014 18:33:18
Sorry for the delay, here it is https://codereview.appspot.com/89180044/
Status: ReadyForReview
From dam...@gmail.com on April 18, 2014 00:30:16
This issue was updated by revision a59619c86162 .
Status: Delivered
From dam...@gmail.com on April 20, 2014 21:23:20
Status: Deployed
Labels: Milestone-V4.97
From dam...@gmail.com on April 12, 2014 13:55:52
May be also do a .contains("@") before displaying the email? Related to Issue 1725
Original issue: http://code.google.com/p/teammatespes/issues/detail?id=1779