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.

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

Create SQL logic for UpdateNotificationAction and add relevant tests for v9 migration #12085

Conversation

hhdqirui
Copy link
Member

Part of #12048

Create SQL logic for UpdateNotificationAction and add relevant tests

@hhdqirui hhdqirui force-pushed the v9-migration-notification-UpdateNotificationAction branch from 2298e1d to 533399c Compare February 15, 2023 09:36
@hhdqirui hhdqirui marked this pull request as ready for review February 15, 2023 09:38
@hhdqirui hhdqirui added the s.ToReview The PR is waiting for review(s) label Feb 15, 2023
@hhdqirui hhdqirui changed the title Create SQL logic for UpdateNotificationAction and add relevant tests Create SQL logic for UpdateNotificationAction and add relevant tests for v9 migration Feb 15, 2023
@hhdqirui hhdqirui force-pushed the v9-migration-notification-UpdateNotificationAction branch from 533399c to eaf3c0c Compare February 16, 2023 06:03
@@ -34,7 +35,7 @@ public JsonResult execute() throws InvalidHttpRequestBodyException {
.build();

try {
return new JsonResult(new NotificationData(logic.updateNotification(newNotification)));
return new JsonResult(new NotificationData(sqlLogic.updateNotification(newNotification)));
Copy link
Member

Choose a reason for hiding this comment

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

Can we fetch the notification then use the setters to update rather than create a detached object. This ensures that it's in a managed state (less chance of random errors when session is flushed, slight performance benefits as well).

@hhdqirui hhdqirui force-pushed the v9-migration-notification-UpdateNotificationAction branch from d47b2d9 to b481ea7 Compare February 22, 2023 08:37
@samuelfangjw samuelfangjw added s.Ongoing The PR is being worked on by the author(s) and removed s.ToReview The PR is waiting for review(s) labels Feb 22, 2023
Copy link
Member

@samuelfangjw samuelfangjw left a comment

Choose a reason for hiding this comment

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

Changes look great!

@@ -18,23 +19,20 @@ public class UpdateNotificationAction extends AdminOnlyAction {

@Override
public JsonResult execute() throws InvalidHttpRequestBodyException {
String notificationId = getNonNullRequestParamValue(Const.ParamsNames.NOTIFICATION_ID);
UUID notificationId = UUID.fromString(getNonNullRequestParamValue(Const.ParamsNames.NOTIFICATION_ID));
Copy link
Member

Choose a reason for hiding this comment

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

I added a method for this, getUUIDRequestParamValue. See GetNotificationAction. Main difference is that string is validated to ensure it is a proper UUID.

notificationRequest.getStyle(), notificationRequest.getTargetUser(), notificationRequest.getTitle(),
notificationRequest.getMessage());

HibernateUtil.flushSession();
Copy link
Member

Choose a reason for hiding this comment

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

No need to flush (unless we need a generated (in db) value such as ID, createdAt/updatedAt timestamp)

public class NotificationsLogicIT extends BaseTestCaseWithSqlDatabaseAccess {

private NotificationsLogic notificationsLogic = NotificationsLogic.inst();
private NotificationsDb notificationsDb = NotificationsDb.inst();
Copy link
Member

Choose a reason for hiding this comment

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

Might be slightly cleaner to use getNotifications in logic instead of db, then we can remove this declaration.

Comment on lines 27 to 30
@BeforeClass
public void initLogicDependencies() {
notificationsLogic.initLogicDependencies(notificationsDb);
}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe move this up to parent class and initialise all dependencies? We will need to do this for all test cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just realise it is already initialised in setUpClass method

@hhdqirui hhdqirui force-pushed the v9-migration-notification-UpdateNotificationAction branch from db28ea7 to 3a91abf Compare February 25, 2023 08:13
@samuelfangjw samuelfangjw merged commit 2611338 into TEAMMATES:v9-migration Feb 25, 2023
samuelfangjw pushed a commit that referenced this pull request Feb 26, 2023
samuelfangjw pushed a commit that referenced this pull request Apr 8, 2023
@wkurniawan07 wkurniawan07 added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging c.Task Other non-user-facing works, e.g. refactoring, adding tests and removed s.Ongoing The PR is being worked on by the author(s) labels Jan 21, 2024
@wkurniawan07 wkurniawan07 added this to the V9.0.0-beta.0 milestone Jan 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants