Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[#9420] Consolidate account, student RESTFul API #9436

Merged
merged 48 commits into from Feb 28, 2019
Merged

[#9420] Consolidate account, student RESTFul API #9436

merged 48 commits into from Feb 28, 2019

Conversation

junming403
Copy link
Member

@junming403 junming403 commented Feb 14, 2019 •
edited

Part of #9420

@junming403 junming403 changed the title [# 9420] API Consolidation [#9420] API Consolidation Feb 14, 2019
@junming403
Copy link
Member Author

I found a bug while writing test cases for account search action.

See 84006a5

@junming403 junming403 changed the title [#9420] API Consolidation [#9420] RESTFul API consolidation - junming Feb 14, 2019
@junming403 junming403 changed the title [#9420] RESTFul API consolidation - junming [#9420] RESTFul API consolidation for /account(GET, POST, DELETE), /account/reset, /account/downgrade, /accounts/search, /students/csv, /student/profile(PUT) Feb 14, 2019
@junming403 junming403 changed the title [#9420] RESTFul API consolidation for /account(GET, POST, DELETE), /account/reset, /account/downgrade, /accounts/search, /students/csv, /student/profile(PUT) [#9420] * [#9420] Consolidate account, student RESTFul API Feb 14, 2019
@junming403 junming403 changed the title [#9420] * [#9420] Consolidate account, student RESTFul API [#9420] Consolidate account, student RESTFul API Feb 14, 2019
@xpdavid xpdavid added the s.Ongoing The PR is being worked on by the author(s) label Feb 15, 2019
@junming403 junming403 closed this Feb 15, 2019
@junming403 junming403 reopened this Feb 15, 2019
@TEAMMATES TEAMMATES deleted a comment from teammates-bot Feb 15, 2019
@junming403
Copy link
Member Author

Waiting for AccountData and CourseData to be merged. (E2E tests does not pass for this reason)

Ready for review.

ChooJeremy
ChooJeremy requested changes Feb 17, 2019
View reviewed changes
Copy link
Contributor

@ChooJeremy ChooJeremy left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry in advance XD

src/main/java/teammates/common/util/Const.java Outdated Show resolved Hide resolved
src/main/java/teammates/ui/webapi/action/SearchAccountsAction.java Outdated Show resolved Hide resolved
src/main/java/teammates/ui/webapi/action/PutStudentProfileAction.java Outdated Show resolved Hide resolved
src/main/java/teammates/ui/webapi/action/GetAccountAction.java Outdated Show resolved Hide resolved
src/main/java/teammates/ui/webapi/action/CreateAccountAction.java Outdated Show resolved Hide resolved
src/web/services/StudentProfile.service.ts Outdated Show resolved Hide resolved
src/web/services/StudentProfile.service.ts Outdated Show resolved Hide resolved
src/web/services/StudentProfile.service.ts Outdated Show resolved Hide resolved
src/web/services/account.service.ts Show resolved Hide resolved
src/web/services/account.service.ts Outdated Show resolved Hide resolved
@junming403
Copy link
Member Author

A regression bug has been found in InstructorLogic......

                    InstructorAttributes.updateOptionsWithEmailBuilder(originalEmail, originalEmail)

The first argument should be courseId instead, fixed in this PR

@junming403
Copy link
Member Author

All updates has been made. currently for get Account, there are two functions.
The one frontend used is getAccountsAction while I have written a getAccountAction for further use.

Ready for review

@junming403
Copy link
Member Author

junming403 commented Feb 25, 2019 •
edited

Tests added according to review, see a667168, ready for review

xpdavid
xpdavid approved these changes Feb 25, 2019
View reviewed changes
Copy link
Contributor

@xpdavid xpdavid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@xpdavid xpdavid added s.FinalReview The PR is ready for final review and removed s.Ongoing The PR is being worked on by the author(s) labels Feb 25, 2019
@xpdavid xpdavid added this to Ongoing in Front-end and RESTful back-end Migration via automation Feb 25, 2019
wkurniawan07
wkurniawan07 requested changes Feb 26, 2019
View reviewed changes
Copy link
Member

@wkurniawan07 wkurniawan07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good mostly, just some classes need renaming.

src/main/java/teammates/common/datatransfer/CommonBundle.java Outdated
* * <br> * account name, email, google id, course name, institution.
* * <br> * link for join course, home page and manage account.
*/
public class CommonBundle {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was partially my mistake before to give is a vague name such as CommonBundle, but at that time this class was confined at the action class. Now that this class is in the DTO package, it needs a better name. A possible name would be CommonAccountSearchResult.

src/main/java/teammates/common/datatransfer/StudentBundle.java Outdated
* * <br> * The section name, team name, comments.
* * <br> * Associated opened, not opened, and published sessions.
*/
public class StudentBundle extends CommonBundle {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per the previous comment, this could be named StudentAccountSearchResult.

src/main/java/teammates/common/datatransfer/InstructorBundle.java Outdated
/**
* Represents details of an instructor account.
*/
public class InstructorBundle extends CommonBundle {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per the previous comment, this could be named InstructorAccountSearchResult.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, see 64de35f

@junming403 junming403 force-pushed the api-consolidation branch 3 times, most recently from 89d2146 to 64de35f Compare February 27, 2019 03:19
@junming403 junming403 force-pushed the api-consolidation branch 3 times, most recently from c0eed9d to deac5a5 Compare February 27, 2019 05:20
wkurniawan07
wkurniawan07 approved these changes Feb 27, 2019
View reviewed changes
Copy link
Member

@wkurniawan07 wkurniawan07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wkurniawan07 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 Feb 27, 2019
@wkurniawan07 wkurniawan07 added this to the V7.0.0-beta.0 milestone Feb 27, 2019
@wkurniawan07 wkurniawan07 merged commit dcf7deb into TEAMMATES:master Feb 28, 2019
Front-end and RESTful back-end Migration automation moved this from Ongoing to Done Feb 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChooJeremy ChooJeremy ChooJeremy approved these changes

@wkurniawan07 wkurniawan07 wkurniawan07 approved these changes

@xpdavid xpdavid xpdavid approved these changes

Assignees

@xpdavid xpdavid

Labels
s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
No open projects
Front-end and RESTful back-end Migration
  
Done
Milestone
V7.0.0-beta.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@junming403 @ChooJeremy @wkurniawan07 @xpdavid @darrenwee