[#9494] Consolidate GET course RESTFul API #9512

ChooJeremy · 2019-02-27T12:40:08Z

Part of #9494

PR Checklist

Outline of Solution

GET /course will only serve the course related portions of the other APIs, and no other parts. To eliminate extra data, I've taken a look at what the UI shows for student and instructor and came up with the following:
GET /course.
If instructor: input courseID, return CourseData for that course.

Ready for peer review.

Adoby7

Some nits

Adoby7 · 2019-02-28T04:55:28Z

.../app/pages-instructor/instructor-session-edit-page/instructor-session-edit-page.component.ts

-              }, (resp: ErrorMessageOutput) => {
-                this.statusMessageService.showErrorMessage(resp.error.message);
-              });
+    this.courseService.getCourse(this.courseId).subscribe((course: Course) => {


Use Response DTO here for Course?

Adoby7 · 2019-02-28T04:56:10Z

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

-        assertEquals(courseAttributes.getId(), response.getCourseId());
-        assertEquals(courseAttributes.getName(), response.getCourseName());
-        assertEquals(courseAttributes.getTimeZone().getId(), response.getTimeZone());
+        assertEquals(courseAttributes.getId(), courseData.getCourseId());


Suggest to change courseAttributes to be expectedCourse

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

rrtheonlyone

Just some minor stuff 👍

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

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

ChooJeremy · 2019-02-28T13:27:17Z

Ready for review

junming403 · 2019-03-02T08:12:37Z

If student: no input. output array of CourseData (only ID, name and timeZone provided) - this is based off functionality from GetStudentCourseAction

Where is this part reflected?

junming403

Looks good, some nits.

junming403 · 2019-03-02T08:14:49Z

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


        loginAsInstructor(instructor1OfCourse1.googleId);

        ______TS("Not enough parameters");

        verifyHttpParameterFailure();

-        ______TS("typical success case");
+        ______TS("typical success case for instructor");


Instead of using ____TS(). It's better to separate tests into different sub tests and annotate them with @Test. Since you already did it for one of the function, let's do it for all of them?

ChooJeremy · 2019-03-02T08:18:36Z

If student: no input. output array of CourseData (only ID, name and timeZone provided) - this is based off functionality from GetStudentCourseAction

Where is this part reflected?

That part's functionality has been moved to get /courses, as this API should only return a single course object (instead of multiple, like an array)

Adoby7

LGTM

junming403

LGTM!

ChooJeremy · 2019-03-15T16:30:15Z

Sorry, it turns out that student support is required in this endpoint after all, from STUDENT_COURSE (GET). It has been updated to support students. The latest 2 commits contains all the changes since the last LGTM, please take a look :)

Adoby7

LGTM, just a note to let you know.

Adoby7 · 2019-03-16T11:40:34Z

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


        submissionParams = new String[] {
                Const.ParamsNames.COURSE_ID, courseAttributes.getId(),
        };

-        verifyOnlyInstructorsOfTheSameCourseCanAccess(submissionParams);
+        verifyInaccessibleWithoutLogin(submissionParams);


You are advised to put this change in "Updated tests to support students" for a better practice of organizing commit messages

Should it? I kind of disagree... this change is relevant to the change in access control for GetCourseAction, so the test should be updated in the same commit that contains that change?

ChooJeremy · 2019-03-18T07:21:25Z

The build passes, but does not appear to be correctly being reported in the UI here.

xpdavid · 2019-03-19T08:38:40Z

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

+        }
+
+        if (userInfo.isStudent) {
+            if (!hasStudentJoinedCourse(courseId)) {


This if check is not necessary I think. gateKeeper.verifyAccessible(logic.getStudentForGoogleId(courseId, userInfo.id), course); will do null check.

xpdavid

LGTM

src/web/services/course.service.ts

Co-Authored-By: ChooJeremy <ChooJeremy@Ymail.com>

wkurniawan07 · 2019-03-21T07:16:22Z

.../app/pages-instructor/instructor-session-edit-page/instructor-session-edit-page.component.ts

+              statusMessageService: StatusMessageService,
+              navigationService: NavigationService,
+              feedbackSessionsService: FeedbackSessionsService,
+              feedbackQuestionsService: FeedbackQuestionsService,


Not your fault, but all of the above fields should be private. Let's fix it while we have the chance.

Hmm... making this change causes:

ERROR in src/web/app/pages-instructor/instructor-session-edit-page/instructor-session-edit-page.component.ts(60,14): error TS2415: Class 'InstructorSessionEditPageComponent' incorrectly extends base class 'InstructorSessionBasePageComponent'. Property 'feedbackQuestionsService' is private in type 'InstructorSessionEditPageComponent' but not in type 'InstructorSessionBasePageComponent'.

for each of the properties changing to private. If we change the access modifier of the base class, we'll need to modify the access modifier for all the other classes that extend from it as well (instructor sessions page component etc.)

Opps. Merged but not seen this comment :P

Then it's protected; still not the default which is public

ChooJeremy self-assigned this Feb 27, 2019

ChooJeremy force-pushed the course-get branch 4 times, most recently from cb767a1 to 1208a26 Compare February 27, 2019 17:19

wkurniawan07 unassigned ChooJeremy Feb 27, 2019

Adoby7 reviewed Feb 28, 2019

View reviewed changes

ChooJeremy force-pushed the course-get branch from 1208a26 to dd1c178 Compare February 28, 2019 11:01

rrtheonlyone reviewed Feb 28, 2019

View reviewed changes

Update GET CourseAction to support student and instructor

8bc936f

ChooJeremy force-pushed the course-get branch from dd1c178 to 45a6268 Compare February 28, 2019 13:26

ChooJeremy requested a review from xpdavid February 28, 2019 13:27

ChooJeremy added the s.ToReview The PR is waiting for review(s) label Feb 28, 2019

junming403 requested changes Mar 2, 2019

View reviewed changes

ChooJeremy force-pushed the course-get branch from 45a6268 to 5962415 Compare March 2, 2019 09:25

ChooJeremy added 2 commits March 2, 2019 21:49

Added tests for GetCourseAction

a2e3f18

Front-end update

286fb1d

ChooJeremy force-pushed the course-get branch from 5962415 to 286fb1d Compare March 2, 2019 13:50

Merge branch 'master' into course-get

c8824a6

xpdavid requested review from junming403 and removed request for xpdavid March 11, 2019 06:34

ChooJeremy added 2 commits March 13, 2019 16:27

Merge branch 'master' into course-get

1a5c13e

Update documentation and refactored tests.

a7de435

Adoby7 approved these changes Mar 14, 2019

View reviewed changes

wkurniawan07 requested a review from xpdavid March 14, 2019 14:03

xpdavid assigned xpdavid and Adoby7 Mar 14, 2019

junming403 approved these changes Mar 14, 2019

View reviewed changes

xpdavid assigned junming403 Mar 15, 2019

ChooJeremy added 2 commits March 15, 2019 21:47

Add student support for GetCourseAction

7e215ff

Updated tests to support students

f917a47

ChooJeremy force-pushed the course-get branch from 021ec54 to f917a47 Compare March 15, 2019 13:47

ChooJeremy requested review from junming403 and Adoby7 and removed request for xpdavid March 15, 2019 16:30

Adoby7 approved these changes Mar 16, 2019

View reviewed changes

Merge branch 'master' into course-get

d45e675

ChooJeremy requested a review from xpdavid March 18, 2019 07:21

xpdavid reviewed Mar 19, 2019

View reviewed changes

Removed extra check in action

fe92eda

ChooJeremy requested a review from xpdavid March 20, 2019 16:46

xpdavid approved these changes Mar 21, 2019

View reviewed changes

src/web/services/course.service.ts Outdated Show resolved Hide resolved

Updated documentation for frontend service

b78cf21

Co-Authored-By: ChooJeremy <ChooJeremy@Ymail.com>

ChooJeremy requested a review from wkurniawan07 March 21, 2019 07:13

ChooJeremy added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Mar 21, 2019

wkurniawan07 approved these changes Mar 21, 2019

View reviewed changes

wkurniawan07 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 Mar 21, 2019

Merge branch 'master' into course-get

36277f0

xpdavid merged commit dad85f3 into TEAMMATES:master Mar 21, 2019

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

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

wkurniawan07 moved this from Ongoing to Done in Front-end and RESTful back-end Migration Apr 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#9494] Consolidate GET course RESTFul API #9512

[#9494] Consolidate GET course RESTFul API #9512

ChooJeremy commented Feb 27, 2019 •

edited

Adoby7 left a comment

Adoby7 Feb 28, 2019

Adoby7 Feb 28, 2019

rrtheonlyone left a comment

ChooJeremy commented Feb 28, 2019

junming403 commented Mar 2, 2019

junming403 left a comment

junming403 Mar 2, 2019

ChooJeremy commented Mar 2, 2019

Adoby7 left a comment

junming403 left a comment

ChooJeremy commented Mar 15, 2019

Adoby7 left a comment

Adoby7 Mar 16, 2019

ChooJeremy Mar 16, 2019

ChooJeremy commented Mar 18, 2019

xpdavid Mar 19, 2019

xpdavid left a comment

wkurniawan07 Mar 21, 2019

ChooJeremy Mar 21, 2019

xpdavid Mar 21, 2019

wkurniawan07 Mar 21, 2019

[#9494] Consolidate GET course RESTFul API #9512

[#9494] Consolidate GET course RESTFul API #9512

Conversation

ChooJeremy commented Feb 27, 2019 • edited

Adoby7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrtheonlyone left a comment

Choose a reason for hiding this comment

ChooJeremy commented Feb 28, 2019

junming403 commented Mar 2, 2019

junming403 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChooJeremy commented Mar 2, 2019

Adoby7 left a comment

Choose a reason for hiding this comment

junming403 left a comment

Choose a reason for hiding this comment

ChooJeremy commented Mar 15, 2019

Adoby7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChooJeremy commented Mar 18, 2019

Choose a reason for hiding this comment

xpdavid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChooJeremy commented Feb 27, 2019 •

edited