[#9564] Consolidate GET Student Profile RESTFul API #9580

Adoby7 · 2019-03-19T04:54:15Z

Part of #9564
Outline of Solution

The access control is following:

A student can view his own profile.
A student can view his teammate's profile in the same class.
An instructor can view profiles of students in his class.

Case 1 requires no parameter.
Case 2, 3 require studentEmail and courseId as parameters.

updated: the profile page e2e test is updated because /student/profile endpoint is changed

junming403

Overall good work!

src/main/java/teammates/ui/webapi/action/GetStudentProfileAction.java

src/test/java/teammates/test/cases/webapi/GetStudentProfileActionTest.java

ChooJeremy

@rrtheonlyone is probably more appropriate to review this, especially since he's also working on profile API. Here are my initial thoughts, I'll do a more thorough check when I have time to download this branch and test out the front end.

src/main/java/teammates/ui/webapi/action/GetStudentProfileAction.java

src/test/java/teammates/test/cases/webapi/GetStudentProfileActionTest.java

rrtheonlyone

Look good 👍 I just have some comments.

src/main/java/teammates/ui/webapi/action/GetStudentProfileAction.java

src/web/app/pages-student/student-profile-page/student-profile-page.component.ts

Adoby7 · 2019-03-21T16:58:43Z

close and reopen PR to solve "Failed to read manifest file" problem

rrtheonlyone

LGTM 👍

I just have one small suggestion.

src/main/java/teammates/logic/api/GateKeeper.java

src/main/java/teammates/ui/webapi/action/GetStudentProfileAction.java

src/test/java/teammates/test/cases/webapi/GetStudentProfileActionTest.java

xpdavid · 2019-03-30T15:51:24Z

src/test/java/teammates/test/cases/webapi/GetStudentProfileActionTest.java

+                .build();
+        typicalBundle.students.put("unregisteredStudentInCourse1", unregisteredStudentInCourse1);
+        removeAndRestoreTypicalDataBundle();
+        removeAndRestoreDataBundle(typicalBundle);


You can use logic.createStudent().

Actually the logic does not have this function, and I think it's unnecessary to create one just for testing

But the creation of student is really weird.

StudentsLogic.inst().createStudent?

src/web/services/student-profile.service.ts

src/test/java/teammates/test/cases/webapi/GetStudentProfileActionTest.java

ChooJeremy

Test consolidation still needs more work. Do take a look through the methods in BaseActionTest and make use of them if possible. Update them if necessary; as long as they don't break anything else. Don't rely on me to find them.

ChooJeremy · 2019-04-02T07:55:21Z

src/test/java/teammates/test/cases/webapi/GetStudentProfileActionTest.java

+    }
+
+    @Test
+    public void testAccessControl_instructorAccessOtherCourse_shouldFail() {


You can merge this method with testAccessControl_instructorAccessProfileFromHisCourse_shouldPass.
They're both testing for the same thing but one for validity and one for invalidity. Instead of doing these tests, you can just do:

Suggested change

public void testAccessControl_instructorAccessOtherCourse_shouldFail() {

public void testAccessControl_onlyInstructorsOfTheSameCourse_shouldPass() {

StudentAttributes student1InCourse1 = typicalBundle.students.get("student1InCourse1");

String[] submissionParams = new String[] {

Const.ParamsNames.COURSE_ID, student1InCourse1.getCourse(),

Const.ParamsNames.STUDENT_EMAIL, student1InCourse1.getEmail(),

};

verifyAccessibleForInstructorsOfTheSameCourse(submissionParams);

verifyInaccessibleForInstructorsOfOtherCourses(submissionParams);

I think it's better to split the test cases as a good practice

ChooJeremy · 2019-04-02T08:02:31Z

src/test/java/teammates/test/cases/webapi/GetStudentProfileActionTest.java

    }

+    @Test
+    public void testAccessControl_instructorWithoutViewStudentInSectionPrivilege_shouldFail() throws Exception {


You should use verifyInaccessibleWithoutViewStudentInSectionsPrivilege in BaseActionTest?

This is because I need to specify which privilege is needed to be tested, see comments below

Adoby7 · 2019-04-02T16:08:20Z

Hi @ChooJeremy, I think using the BaseActionTest methods is not necessary in the following reasons:

The method name of each test case has already self-explained the parameter and the result of this case. Therefore, even not using the BaseActionTest method, each case is described clearly and we can focus more on test case design.
The BaseActionTest method may also confuse people. For example, the verifyAccessibleForInstructorInSameCourse simply uses instructor1InCourse1. Why this instructor is considered in the same course? How about I need to test other courses?
Some testing will be designed improperly using the BaseActionTest method. For example, the testAccessControl_instructorWithoutViewStudentInSectionPrivilege_shouldFail() that you have commented. The verifyInaccessibleWithoutViewStudentInSectionsPrivilege simply uses a helper account to represent an instructor without privilege. The only invalid value that causes inaccessible is ViewStudentInSectionPrivilege, but the helper account has no privilege at all (i.e. too many invalid values), and this may disrupt the accuracy of testing. See here for test case design heuristic.

That's what my thought.

ChooJeremy

@Adoby7
Here's my reasoning:

While this is true, it is harder to see that everything is tested if everything is split up into multiple different test cases in methods. I think it's better to do some abstraction so that each methods tests related sections of the endpoint, such as a single method testing for instructors accessing from other courses, testing for both validity and invalidity. Without this abstraction it's harder to reason about the test cases.
I agree, and this is a fault with the BaseActionTest that @rrtheonlyone also brought up at some point. The student tests in BaseAction tests also only test specifically for student1 and not any other students. Unfortunately while it is a fault, it's harder to have it adapt - it would have to read the parameters send in and decide which instructor is appropriate as same course and which is appropriate as different course, or add in new parameters to the method. Unfortunately it's likely just a design choice that we'll just have to follow, it's harder to fix it either way.
This is kind of why I suggested editing the method if needed.

I see no other problems with your code, so I'll just approve this and pass it on the senior devs to let them give their look through. If they're OK with it, then sure.

junming403

LGTM!

xpdavid

LGTM!

Adoby7 requested review from junming403 and rrtheonlyone March 19, 2019 04:54

Adoby7 added s.ToReview The PR is waiting for review(s) s.Ongoing The PR is being worked on by the author(s) and removed s.ToReview The PR is waiting for review(s) labels Mar 19, 2019

Adoby7 removed request for rrtheonlyone and junming403 March 19, 2019 07:28

Adoby7 force-pushed the student_profile branch from f9dfb8d to d768390 Compare March 19, 2019 08:22

Adoby7 added s.ToReview The PR is waiting for review(s) and removed s.Ongoing The PR is being worked on by the author(s) labels Mar 19, 2019

Adoby7 requested review from junming403, rrtheonlyone and ChooJeremy March 19, 2019 08:34

junming403 requested changes Mar 20, 2019

View reviewed changes

src/main/java/teammates/ui/webapi/action/GetStudentProfileAction.java Outdated Show resolved Hide resolved

src/test/java/teammates/test/cases/webapi/GetStudentProfileActionTest.java Show resolved Hide resolved

Adoby7 requested a review from junming403 March 20, 2019 16:08

ChooJeremy requested changes Mar 20, 2019

View reviewed changes

rrtheonlyone requested changes Mar 21, 2019

View reviewed changes

src/main/java/teammates/ui/webapi/action/GetStudentProfileAction.java Outdated Show resolved Hide resolved

src/web/app/pages-student/student-profile-page/student-profile-page.component.ts Outdated Show resolved Hide resolved

Adoby7 force-pushed the student_profile branch from 191d084 to dac1c04 Compare March 21, 2019 14:14

Adoby7 removed the request for review from junming403 March 21, 2019 14:24

Adoby7 force-pushed the student_profile branch 3 times, most recently from 0879196 to fad5d7d Compare March 21, 2019 16:39

Adoby7 requested a review from rrtheonlyone March 21, 2019 16:56

Adoby7 closed this Mar 21, 2019

Adoby7 reopened this Mar 21, 2019

rrtheonlyone approved these changes Mar 22, 2019

View reviewed changes

src/main/java/teammates/logic/api/GateKeeper.java Show resolved Hide resolved

Adoby7 force-pushed the student_profile branch from fad5d7d to 27d896e Compare March 22, 2019 15:21

Adoby7 requested a review from xpdavid March 22, 2019 15:41

xpdavid requested a review from ChooJeremy March 25, 2019 13:12

Adoby7 requested review from xpdavid and removed request for junming403 and xpdavid March 29, 2019 05:57

xpdavid reviewed Mar 30, 2019

View reviewed changes

Adoby7 force-pushed the student_profile branch from 566ef9d to 1dbc28f Compare April 1, 2019 05:43

Adoby7 requested review from junming403 and ChooJeremy April 1, 2019 05:58

Adoby7 force-pushed the student_profile branch from 1dbc28f to 64002be Compare April 2, 2019 06:46

ChooJeremy requested changes Apr 2, 2019

View reviewed changes

Adoby7 force-pushed the student_profile branch from b6b8483 to 7bbf7d0 Compare April 2, 2019 08:10

ChooJeremy approved these changes Apr 3, 2019

View reviewed changes

junming403 approved these changes Apr 4, 2019

View reviewed changes

Adoby7 added 2 commits April 4, 2019 15:24

consolidate ’/student/profile‘ API

b62e647

add test cases

04e3010

Adoby7 force-pushed the student_profile branch from 7bbf7d0 to 04e3010 Compare April 4, 2019 07:24

Adoby7 requested a review from xpdavid April 4, 2019 14:44

xpdavid approved these changes Apr 7, 2019

View reviewed changes

xpdavid added this to Ongoing in Front-end and RESTful back-end Migration via automation Apr 7, 2019

xpdavid requested a review from damithc April 7, 2019 09:29

xpdavid added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Apr 7, 2019

damithc approved these changes Apr 7, 2019

View reviewed changes

Adoby7 added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.FinalReview The PR is ready for final review labels Apr 7, 2019

Merge branch 'master' into student_profile

9bff109

Adoby7 merged commit 6300b2f into TEAMMATES:master Apr 7, 2019

Front-end and RESTful back-end Migration automation moved this from Ongoing to Done Apr 7, 2019

Adoby7 deleted the student_profile branch April 7, 2019 12:13

wkurniawan07 added this to the V7.0.0-beta.0 milestone Apr 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#9564] Consolidate GET Student Profile RESTFul API #9580

[#9564] Consolidate GET Student Profile RESTFul API #9580

Adoby7 commented Mar 19, 2019 •

edited

junming403 left a comment

ChooJeremy left a comment

rrtheonlyone left a comment

Adoby7 commented Mar 21, 2019

rrtheonlyone left a comment •

edited

xpdavid Mar 30, 2019

Adoby7 Apr 1, 2019

xpdavid Apr 1, 2019

ChooJeremy left a comment

ChooJeremy Apr 2, 2019

Adoby7 Apr 2, 2019

ChooJeremy Apr 2, 2019

Adoby7 Apr 2, 2019

Adoby7 commented Apr 2, 2019

ChooJeremy left a comment

junming403 left a comment

xpdavid left a comment

-    public void testAccessControl_instructorAccessOtherCourse_shouldFail() {
+    public void testAccessControl_onlyInstructorsOfTheSameCourse_shouldPass() {
+        StudentAttributes student1InCourse1 = typicalBundle.students.get("student1InCourse1");
+        String[] submissionParams = new String[] {
+                Const.ParamsNames.COURSE_ID, student1InCourse1.getCourse(),
+                Const.ParamsNames.STUDENT_EMAIL, student1InCourse1.getEmail(),
+        };
+        verifyAccessibleForInstructorsOfTheSameCourse(submissionParams);
+        verifyInaccessibleForInstructorsOfOtherCourses(submissionParams);

[#9564] Consolidate GET Student Profile RESTFul API #9580

[#9564] Consolidate GET Student Profile RESTFul API #9580

Conversation

Adoby7 commented Mar 19, 2019 • edited

junming403 left a comment

Choose a reason for hiding this comment

ChooJeremy left a comment

Choose a reason for hiding this comment

rrtheonlyone left a comment

Choose a reason for hiding this comment

Adoby7 commented Mar 21, 2019

rrtheonlyone left a comment • edited

Choose a reason for hiding this comment

xpdavid Mar 30, 2019

Choose a reason for hiding this comment

Adoby7 Apr 1, 2019

Choose a reason for hiding this comment

xpdavid Apr 1, 2019

Choose a reason for hiding this comment

ChooJeremy left a comment

Choose a reason for hiding this comment

ChooJeremy Apr 2, 2019

Choose a reason for hiding this comment

Adoby7 Apr 2, 2019

Choose a reason for hiding this comment

ChooJeremy Apr 2, 2019

Choose a reason for hiding this comment

Adoby7 Apr 2, 2019

Choose a reason for hiding this comment

Adoby7 commented Apr 2, 2019

ChooJeremy left a comment

Choose a reason for hiding this comment

junming403 left a comment

Choose a reason for hiding this comment

xpdavid left a comment

Choose a reason for hiding this comment

Adoby7 commented Mar 19, 2019 •

edited

rrtheonlyone left a comment •

edited