[#9564] Consolidate PUT /students RESTFul API #9647

Adoby7 · 2019-04-05T08:25:11Z

Part of #9564

Outline of Solution

Use request DTO
Rename the old endpoint to PUT /students
Backend only returns a list of successful enrolled students. Logic of classifying new students, modified students, unchanged students are delegated to frontend.

xpdavid

Nice job! Let's learn TypeScript in one week.

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

src/main/java/teammates/ui/webapi/request/StudentsEnrollRequest.java

xpdavid · 2019-04-12T06:40:28Z

...pp/pages-instructor/instructor-course-enroll-page/instructor-course-enroll-page.component.ts

+    });
+  }
+
+  private propagateEnrollResultPanelList(existingStudents: Student[], enrolledStudents: Student[],


populate is a better word :)

xpdavid · 2019-04-12T06:41:56Z

...pp/pages-instructor/instructor-course-enroll-page/instructor-course-enroll-page.component.ts

+
+  private propagateEnrollResultPanelList(existingStudents: Student[], enrolledStudents: Student[],
+                                         enrollRequests: StudentEnrollRequest[]): EnrollResultPanel[] {
+    const updateStatus: { [key: string]: number } = {


Let's define this as an enum above

xpdavid · 2019-04-12T06:42:41Z

...pp/pages-instructor/instructor-course-enroll-page/instructor-course-enroll-page.component.ts

-            color: 'danger',
-          });
+    const panelClass: { [key: number]: string } = {
+      0: 'bg-primary',


Expose the enum to the HTML file and put the logic of choosing different CSS class in the HTML file.

...pp/pages-instructor/instructor-course-enroll-page/instructor-course-enroll-page.component.ts

Adoby7 · 2019-04-20T04:39:04Z

@tanhengyeow I have added some general error message in the prompted window. Probably this would help. If we want the detailed error message, probably it needs a separate PR to do

tanhengyeow

I think this is acceptable for now. Good job on this PR!

My original concern was that the functionalities (e.g. showing which row has specific errors) should be retained from v6 if we don't have a solution now that improves upon it. But yea, I can file a follow up issue for this since this PR is mostly about consolidating the current API to be RESTful.

Logic of classifying new students, modified students, unchanged students are delegated to frontend.

In addition, the API response contains students who are enrolled successfully, and I could not find a way to return both correct response and error response simultaneously.

Yes that's right but the API should still return a format that the frontend is able to do further work on it for example:

Request:

StudentEnrollRequest.studentEnrollRequests: [{
 name: ...
 email: ...
 },
...
]

Response:

[
    { "status": NEW_ENROLL_SUCCESS, "message": "" },
    { "status": UNMODIFIED_SUCCESS, "message": "" },
    { "status": FAILURE, "message": "Missing email" },
]

You are very welcome to work on the follow-up issue even after CS3282 😄

tanhengyeow · 2019-04-20T05:10:57Z

@Adoby7 Reminder to fix the lint error :)

damithc

Just a query. Can merge without further review from me.

damithc · 2019-04-21T05:14:18Z

src/main/java/teammates/common/util/Const.java

@@ -56,7 +56,7 @@

    public static final String ACTION_RESULT_FAILURE = "Servlet Action Failure";

-    public static final int SIZE_LIMIT_PER_ENROLLMENT = 150;
+    public static final int SIZE_LIMIT_PER_ENROLLMENT = 100;


Why is this changed to 100? I think the 150 was intentional, to allow courses with just over 100 students to be enrolled in one shot.

The enroll pages says maximum 100 students in one enrollment. Do we change it back?

It was a hidden feature. The advertised limit remains 100. Change it back to 150 and put a comment to explain why it is 150?

So, do we show the users maximum enroll size is 100, but if they try to enroll students slightly more than 100, we still allow it?

@damithc the bigger question is, why does it become a "hidden" feature?

100 is a good round number and easy for users too e.g., when dividing a spreadsheet into batches.
However, it turns out that it is fairly common for instructors to have just over 100 students and also fairly common for them to not see the limit and try to enroll all at once. Allowing a buffer of 50 more allow such requests to go through -> reduces annoyance to users and saves us some requests too. This is a bit similar to having a hidden buffer for submission deadlines.
Having said that, I just realized this hidden feature is no longer usable because the spreadsheet limits the number of rows to 100. Which means we can go back to 100. Just make sure there is no off-by-one error. i.e., we support 100, not 99, students at one go (I remember having a bug like that once)

wkurniawan07

Excellent job in not just consolidating the API but also improving the enrollment process!

# Conflicts: # src/e2e/java/teammates/e2e/cases/lnp/InstructorStudentEnrollmentLNPTest.java

Adoby7 · 2019-04-21T09:15:45Z

Hi @wkurniawan07, could you look at this PR again, as there were some conflicts after your approval. Thank you

amrut-prabhu · 2019-04-21T09:35:40Z

@Adoby7 If you're referring to the conflicts in commit 7318cd9, the changes look fine to me

* rename the old endpoint to /students * rewrite the old endpoint * remove deprecated stuffs * add testing on new endpoint

Adoby7 force-pushed the enroll_student branch 8 times, most recently from 664ddda to 87410f0 Compare April 9, 2019 03:35

Adoby7 marked this pull request as ready for review April 9, 2019 03:36

Adoby7 force-pushed the enroll_student branch 2 times, most recently from 8964556 to d1b7c95 Compare April 9, 2019 04:55

Adoby7 requested review from junming403 and ChooJeremy April 9, 2019 06:21

Adoby7 added the s.ToReview The PR is waiting for review(s) label Apr 9, 2019

Adoby7 requested a review from tanhengyeow April 9, 2019 08:01

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

Adoby7 requested review from monmanuela and rrtheonlyone April 9, 2019 16:13

Adoby7 assigned ChooJeremy, rrtheonlyone, tanhengyeow, junming403 and monmanuela Apr 11, 2019

Adoby7 force-pushed the enroll_student branch 4 times, most recently from bcf8c24 to 9d6d0f8 Compare April 12, 2019 05:14

xpdavid reviewed Apr 12, 2019

View reviewed changes

Adoby7 force-pushed the enroll_student branch 2 times, most recently from 66fe3da to 541bbd4 Compare April 13, 2019 14:48

Adoby7 force-pushed the enroll_student branch from c7b74c3 to 0cb90dd Compare April 20, 2019 04:35

tanhengyeow approved these changes Apr 20, 2019

View reviewed changes

Adoby7 force-pushed the enroll_student branch from 0cb90dd to b6b0b53 Compare April 20, 2019 05:33

Adoby7 requested a review from wkurniawan07 April 20, 2019 05:47

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

Adoby7 requested a review from damithc April 21, 2019 05:07

damithc reviewed Apr 21, 2019

View reviewed changes

Adoby7 force-pushed the enroll_student branch 2 times, most recently from 86c558e to 39d3de7 Compare April 21, 2019 05:59

Adoby7 added 4 commits April 21, 2019 16:32

rename the old endpoint to /students

cce34f1

rewrite the old endpoint

d0e729f

remove deprecated stuffs

63d2408

add testing on new endpoint

685bc6d

Adoby7 force-pushed the enroll_student branch from 39d3de7 to 685bc6d Compare April 21, 2019 08:33

wkurniawan07 approved these changes Apr 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 Apr 21, 2019

Merge branch 'master' into enroll_student

7318cd9

# Conflicts: # src/e2e/java/teammates/e2e/cases/lnp/InstructorStudentEnrollmentLNPTest.java

Merge branch 'master' into enroll_student

53b7ebc

Adoby7 merged commit 5cb6356 into TEAMMATES:master Apr 21, 2019

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

Adoby7 deleted the enroll_student branch April 21, 2019 12:43

Adoby7 mentioned this pull request Apr 21, 2019

LNP test of put /students is broken #9689

Closed

wkurniawan07 added this to the V7.0.0-beta.0 milestone Mar 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#9564] Consolidate PUT /students RESTFul API #9647

[#9564] Consolidate PUT /students RESTFul API #9647

Adoby7 commented Apr 5, 2019

xpdavid left a comment

xpdavid Apr 12, 2019

This comment was marked as outdated.

xpdavid Apr 12, 2019 •

edited

xpdavid Apr 12, 2019

Adoby7 commented Apr 20, 2019

tanhengyeow left a comment •

edited

tanhengyeow commented Apr 20, 2019

damithc left a comment

damithc Apr 21, 2019

Adoby7 Apr 21, 2019

damithc Apr 21, 2019

Adoby7 Apr 21, 2019

wkurniawan07 Apr 21, 2019

damithc Apr 21, 2019

Adoby7 Apr 21, 2019

wkurniawan07 left a comment

Adoby7 commented Apr 21, 2019

amrut-prabhu commented Apr 21, 2019

[#9564] Consolidate PUT /students RESTFul API #9647

[#9564] Consolidate PUT /students RESTFul API #9647

Conversation

Adoby7 commented Apr 5, 2019

xpdavid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

xpdavid Apr 12, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adoby7 commented Apr 20, 2019

tanhengyeow left a comment • edited

Choose a reason for hiding this comment

tanhengyeow commented Apr 20, 2019

damithc 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wkurniawan07 left a comment

Choose a reason for hiding this comment

Adoby7 commented Apr 21, 2019

amrut-prabhu commented Apr 21, 2019

xpdavid Apr 12, 2019 •

edited

tanhengyeow left a comment •

edited