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

[#12048] Migrate UpdateStudentAction #12727

Merged
merged 45 commits into from
Feb 25, 2024
Merged

[#12048] Migrate UpdateStudentAction #12727

merged 45 commits into from
Feb 25, 2024

Conversation

marquestye
Copy link
Contributor

@marquestye marquestye commented Feb 5, 2024 •
edited
Loading

Part of #12048

Outline of solution

  • Change UpdateStudentAction to use sqlLogic if course is migrated
  • Modified and created methods in UsersLogic, FeedbackResponsesLogic and AccountsLogic.

Todo:

  • Add integration tests for UpdateStudentAction

domoberzin and others added 24 commits January 29, 2024 15:10
@domoberzin
Modify student entity
eaf3399
@domoberzin
Add update comment logic
8672116
@domoberzin
Modify logic files for cascading update and creation for student
e43cadf
@domoberzin
Add database queries for updating student
565e7bf
@domoberzin
Update EnrollStudentsAction
6c2dccf
@domoberzin
Fix checkstyle
b907544
@domoberzin
Remove extra query for editor update
9a3f142
@domoberzin
Remove email update logic
b9c08ed
@domoberzin
Update javadocs
8708e92
@domoberzin
Copy over logic for Team and Section validation
389ecb9
@domoberzin
Edit javadocs
059a5fa
@domoberzin
Change StudentAttributes to Student instead
21f6aea
@domoberzin
Fix lint issues
7634971
@domoberzin
Fix lint issues
0ea2b58
@domoberzin
Fix component tests and lint
fad80d0
@domoberzin
Merge branch 'v9-migration' into migrate-enroll-students-action
6ea397e
@domoberzin
Remove ununsed method
b02ef4d
@domoberzin
Fix lint
30d3c2a
@domoberzin
Update validation logic to use Student
2b53dcb
@domoberzin
Update test case
dbf4a2d
@domoberzin
Add tests for duplicate team across sections
ca26582
@marquestye
Migrate UpdateStudentAction and add tests
029fc01
@marquestye
Fix merge conflicts
ded4b35
@marquestye
Remove resetStudentGoogleId
ac29c3f
@marquestye marquestye changed the base branch from master to v9-migration February 5, 2024 10:28
@jayasting98 jayasting98 added the s.Ongoing The PR is being worked on by the author(s) label Feb 5, 2024
@jayasting98 jayasting98 added this to the V9.0.0-beta.0 milestone Feb 5, 2024
@jayasting98 jayasting98 self-requested a review February 5, 2024 11:02
@NicolasCwy
Copy link
Contributor

Hi @marquestye, there are alot of merges so try to keep your PR upto date with the base branch when you have time in case of conflicts, so we can catch any errors during the PR review

NicolasCwy
NicolasCwy reviewed Feb 21, 2024
View reviewed changes
Copy link
Contributor

@NicolasCwy NicolasCwy left a comment

Choose a reason for hiding this comment

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

Nice work but I think there are some clarifications needed on cascading of Student entity to the feedback comments, responses and account

src/main/java/teammates/storage/sqlapi/UsersDb.java Show resolved Hide resolved
src/main/java/teammates/sqllogic/core/UsersLogic.java Outdated
@@ -77,8 +77,8 @@ public static UsersLogic inst() {
}

void initLogicDependencies(UsersDb usersDb, AccountsLogic accountsLogic, FeedbackResponsesLogic feedbackResponsesLogic,
FeedbackResponseCommentsLogic feedbackResponseCommentsLogic,
Copy link
Contributor

Choose a reason for hiding this comment

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

Whitespace

src/main/java/teammates/sqllogic/core/UsersLogic.java Show resolved Hide resolved
src/main/java/teammates/sqllogic/core/UsersLogic.java Outdated
feedbackResponsesLogic.updateFeedbackResponsesForChangingEmail(courseId, originalEmail, student.getEmail());
feedbackResponseCommentsLogic.updateFeedbackResponseCommentsEmails(courseId, originalEmail, student.getEmail());

Account originalAccount = originalStudent.getAccount();
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to the previous comment, I think Account and Student emails can be different therefore, the email does not have to be cascaded to account. @wkurniawan07 is that correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the original code I think this is confused with googleid, which from my understanding was not a field that was brought over to the postgres entity, therefore this cascading is not needed @cedricongjh @weiquu

Copy link
Member

Choose a reason for hiding this comment

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

Not only it doesn't have to be cascaded, it must not be cascaded.

NicolasCwy
NicolasCwy reviewed Feb 22, 2024
View reviewed changes
Copy link
Contributor

@NicolasCwy NicolasCwy left a comment

Choose a reason for hiding this comment

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

Some comments, but also clarifications from the others @cedricongjh would you be able to help take a look at this as well

src/it/java/teammates/it/ui/webapi/UpdateStudentActionIT.java Show resolved Hide resolved
src/main/java/teammates/ui/webapi/EnrollStudentsAction.java Show resolved Hide resolved
src/main/java/teammates/sqllogic/core/UsersLogic.java Outdated
feedbackResponsesLogic.updateFeedbackResponsesForChangingEmail(courseId, originalEmail, student.getEmail());
feedbackResponseCommentsLogic.updateFeedbackResponseCommentsEmails(courseId, originalEmail, student.getEmail());

Account originalAccount = originalStudent.getAccount();
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the original code I think this is confused with googleid, which from my understanding was not a field that was brought over to the postgres entity, therefore this cascading is not needed @cedricongjh @weiquu

@NicolasCwy NicolasCwy self-requested a review February 24, 2024 05:24
NicolasCwy
NicolasCwy approved these changes Feb 24, 2024
View reviewed changes
Copy link
Contributor

@NicolasCwy NicolasCwy left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for working with us through this as you've mentioned in #12779 Hibernate modifies managed entities when we use a setter, so we'll fix that in a separate PR

@marquestye marquestye changed the base branch from v9-migration to master February 24, 2024 07:03
@NicolasCwy NicolasCwy 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 24, 2024
jayasting98
jayasting98 approved these changes Feb 25, 2024
View reviewed changes
Copy link
Contributor

@jayasting98 jayasting98 left a comment

Choose a reason for hiding this comment

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

Thanks for the work! There are still some nits, but I think it's not critical right now and it can be done separately in the future. Anyway, it looks good overall. 😄

@jayasting98 jayasting98 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 25, 2024
@cedricongjh cedricongjh merged commit 3191fd1 into TEAMMATES:master Feb 25, 2024
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wkurniawan07 wkurniawan07 wkurniawan07 left review comments

@NicolasCwy NicolasCwy NicolasCwy approved these changes

@jayasting98 jayasting98 jayasting98 approved these changes

Assignees

@marquestye marquestye

Labels
c.Task Other non-user-facing works, e.g. refactoring, adding tests s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Milestone
V9.0.0-beta.0
Development

Successfully merging this pull request may close these issues.
6 participants
@marquestye @NicolasCwy @wkurniawan07 @jayasting98 @cedricongjh @domoberzin