Create SQL logic for CreateNotificationAction and add relevant tests for v9 migration #12077

hhdqirui · 2023-02-11T09:42:32Z

Part of #12048

Create SQL logic for CreateNotificationAction and add relevant tests.

samuelfangjw · 2023-02-11T09:57:27Z

src/test/java/teammates/storage/sqlapi/NotificationsDbTest.java

+
+    @Test
+    public void testCreateNotification_invalidNonNullParameters() {
+        ______TS("failure: start time is before end time");


For unit tests, let's split the test cases into smaller, independent test cases. The reason for ____TS was because for integration tests, resetting the db for every test can get very expensive. This doesn't really matter for unit tests.

In general, for unit tests we really want relatively short tests (~5 lines as a guideline) that can be written really quickly that test a single behaviour/aspect.

samuelfangjw · 2023-02-11T10:07:15Z

src/main/java/teammates/sqllogic/core/NotificationsLogic.java

+
+    private static final NotificationsLogic instance = new NotificationsLogic();
+
+    private final NotificationsDb notificationsDb = NotificationsDb.inst();


Pass in notificationsDb as a dependency instead (via initLogicDependencies). This way, it's easier to mock this out if we need to do unit testing (for the more complex logic methods, once we move the logic out of the db layer).

samuelfangjw · 2023-02-11T10:16:06Z

src/it/java/teammates/it/storage/sqlapi/NotificationDbIT.java

+
+        assertThrows(InvalidParametersException.class, () -> notificationsDb.createNotification(invalidNotification2));
+
+        ______TS("failure: empty message");


For integration tests, try not to test too much. There's quite significant costs to each additional test (running time, dev time, maintainability, etc.). Especially since in this context the devs are the ones that maintain both integration & unit tests, we can skip some redundant tests.

Not a "law", but in general, we want to avoid testing things that do not test the integration between modules/components and are already covered by our unit test. Let the unit tests, which have much less associated costs, check for these (usually edge) cases. In this case, we only want to test the integration between db layer and db/hibernate itself, so we can consider not testing for edge cases that do not interact with the db.

jianhandev · 2023-02-12T05:47:55Z

src/main/java/teammates/storage/sqlapi/NotificationsDb.java

-        if (getNotification(notification.getNotificationId().toString()) != null) {
+        if (notification.getNotificationId() != null && getNotification(notification.getNotificationId()) != null) {
            throw new EntityAlreadyExistsException(String.format(ERROR_CREATE_ENTITY_ALREADY_EXISTS,


At this point I think notification.getNotificationId() will always return null as the ID generation only happens when session is flushed, which means this block of code will never be run at all.

…dd relevant tests for v9 migration (TEAMMATES#12077)

…nt tests for v9 migration (#12077)

* [#12048] Set up github action workflows * [#12048] v9: Skeleton implementation (#12056) * [#12048] Add isMigrated flag to course (#12063) * [#12048] Add is migrated flag to datastore account (#12070) * Temporarily disable liquibase migrations * [#12048] Create Notification Entity for PostgreSQL migration (#12061) * [#12048] Create notification DB layer for v9 migration (#12075) * [#12048] Add UsageStatistics entity and db (#12076) * Migrate GetCourseAction.java * Migrate DeleteCourseAction.java and relevant logic functions * Migrate BinCourseAction.java and its related logic functions * Update checkSpecificAccessControl functions in BinCourseAction and DeleteCourseAction classes * Migrate RestoreCourseAction and its related logic functions * Migrate UpdateCourseAction with its related logic functions * [#12048] Add Account Entity (#12087) * [#12048] Create SQL logic for CreateNotificationAction and add relevant tests for v9 migration (#12077) * [#12048] Create Student, Instructor and User Entities for PostgreSQL Migration (#12071) * [#12048] V9: Cleanup and refactor (#12090) * Edit GetCourseAction and refactor out the old datastore code * [#12048] Remove redundant InstructorRole Enum (#12091) * Fix compilation error * Update check for database to fetch from * Add unit tests for CoursesDb * [#12048] Update GetUsageStatisticsAction to include SQL entities (#12084) * Add CoursesLogicTest class * Disable failing tests * Fix compilation error * Fix Checkstyle errors * Merge branch * Change flow for updating courses. * Update updateCourse JavaDoc comment. * Update CreateCourseAction and related methods * Update GetCourseAction. * Update UpdateCourseAction * Update BinCourseAction and RestoreCourseAction * Update DeleteCourseAction * Migrate GetCourseSectionNamesAction and related methods. * Add Unit tests for Logic layer of Course. * Fix Checkstyle errors * Add unit test for GetCourseAction's execute function * Add verify for CoursesDb unit tests and use assertNull and assertNotNull * Move fetching of course to logic layer. * Fix Checkstyle errors. * Move canCreateCourse logic to logic layer. * Change *CourseAction classes to use isCourseMigrated * Fix CoursesLogic's initLogicDependencies method call * Add unit tests for GetCourseAction. * Remove commented out method. * Add minimal unit tests for BinCourseAction, DeleteCourseAction and RestoreCourseAction. * Add minimal unit tests for GetCourseSectionAction and UpdateCourseAction. * Remove unused EntityType parameter. * Add minimal unit tests for CreateCourseAction. * Fix Checkstyle errors. * Ignore all old datastore test cases for *CourseAction classes. * Fix 'text' type to 'test'. * Change binCourseToRecycleBin to return the binned course. * Update moveCourseToRecycleBin test. * Update test name. --------- Co-authored-by: Samuel Fang <samuelfangjw@gmail.com> Co-authored-by: dao ngoc hieu <53283766+daongochieu2810@users.noreply.github.com> Co-authored-by: Samuel Fang <60355570+samuelfangjw@users.noreply.github.com> Co-authored-by: wuqirui <53338059+hhdqirui@users.noreply.github.com> Co-authored-by: Dominic Lim <46486515+domlimm@users.noreply.github.com>

…nt tests for v9 migration (#12077)

* [#12048] Set up github action workflows * [#12048] v9: Skeleton implementation (#12056) * [#12048] Add isMigrated flag to course (#12063) * [#12048] Add is migrated flag to datastore account (#12070) * Temporarily disable liquibase migrations * [#12048] Create Notification Entity for PostgreSQL migration (#12061) * [#12048] Create notification DB layer for v9 migration (#12075) * [#12048] Add UsageStatistics entity and db (#12076) * Migrate GetCourseAction.java * Migrate DeleteCourseAction.java and relevant logic functions * Migrate BinCourseAction.java and its related logic functions * Update checkSpecificAccessControl functions in BinCourseAction and DeleteCourseAction classes * Migrate RestoreCourseAction and its related logic functions * Migrate UpdateCourseAction with its related logic functions * [#12048] Add Account Entity (#12087) * [#12048] Create SQL logic for CreateNotificationAction and add relevant tests for v9 migration (#12077) * [#12048] Create Student, Instructor and User Entities for PostgreSQL Migration (#12071) * [#12048] V9: Cleanup and refactor (#12090) * Edit GetCourseAction and refactor out the old datastore code * [#12048] Remove redundant InstructorRole Enum (#12091) * Fix compilation error * Update check for database to fetch from * Add unit tests for CoursesDb * [#12048] Update GetUsageStatisticsAction to include SQL entities (#12084) * Add CoursesLogicTest class * Disable failing tests * Fix compilation error * Fix Checkstyle errors * Merge branch * Change flow for updating courses. * Update updateCourse JavaDoc comment. * Update CreateCourseAction and related methods * Update GetCourseAction. * Update UpdateCourseAction * Update BinCourseAction and RestoreCourseAction * Update DeleteCourseAction * Migrate GetCourseSectionNamesAction and related methods. * Add Unit tests for Logic layer of Course. * Fix Checkstyle errors * Add unit test for GetCourseAction's execute function * Add verify for CoursesDb unit tests and use assertNull and assertNotNull * Move fetching of course to logic layer. * Fix Checkstyle errors. * Move canCreateCourse logic to logic layer. * Change *CourseAction classes to use isCourseMigrated * Fix CoursesLogic's initLogicDependencies method call * Add unit tests for GetCourseAction. * Remove commented out method. * Add minimal unit tests for BinCourseAction, DeleteCourseAction and RestoreCourseAction. * Add minimal unit tests for GetCourseSectionAction and UpdateCourseAction. * Remove unused EntityType parameter. * Add minimal unit tests for CreateCourseAction. * Fix Checkstyle errors. * Ignore all old datastore test cases for *CourseAction classes. * Fix 'text' type to 'test'. * Change binCourseToRecycleBin to return the binned course. * Update moveCourseToRecycleBin test. * Update test name. --------- Co-authored-by: Samuel Fang <samuelfangjw@gmail.com> Co-authored-by: dao ngoc hieu <53283766+daongochieu2810@users.noreply.github.com> Co-authored-by: Samuel Fang <60355570+samuelfangjw@users.noreply.github.com> Co-authored-by: wuqirui <53338059+hhdqirui@users.noreply.github.com> Co-authored-by: Dominic Lim <46486515+domlimm@users.noreply.github.com>

hhdqirui requested review from samuelfangjw, daongochieu2810, jianhandev and zhaojj2209 February 11, 2023 09:42

hhdqirui added the s.ToReview The PR is waiting for review(s) label Feb 11, 2023

samuelfangjw requested changes Feb 11, 2023

View reviewed changes

jianhandev reviewed Feb 12, 2023

View reviewed changes

hhdqirui force-pushed the v9-migration-notification-CreateNotificationAction branch from 54bef92 to 2301922 Compare February 15, 2023 04:27

hhdqirui added 14 commits February 15, 2023 12:31

Create sql logic for CreateNotificationAction and add tests

ff83226

Fix lint tests

b203f50

Update db tests

42bd5ad

Update code for notification logic dependency initilisation

0a21088

Update notification db it

eeaad89

Fix lint tests

dce3922

Update tests

5371692

Remove irrelavant checks and update tests

eb14a64

Add missing implementation

345ea10

Remove import

8abb214

Update notification builder

aa41f2c

Update notification builder

0865c01

Update notification builder

22a4bd6

Fix typo

b17db61

hhdqirui force-pushed the v9-migration-notification-CreateNotificationAction branch from 2301922 to b17db61 Compare February 15, 2023 04:32

hhdqirui requested a review from samuelfangjw February 15, 2023 04:35

samuelfangjw approved these changes Feb 15, 2023

View reviewed changes

samuelfangjw added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.ToReview The PR is waiting for review(s) labels Feb 15, 2023

samuelfangjw assigned jianhandev and samuelfangjw Feb 15, 2023

samuelfangjw merged commit 97d54b8 into TEAMMATES:v9-migration Feb 15, 2023

domlimm pushed a commit to domlimm/teammates that referenced this pull request Feb 22, 2023

[TEAMMATES#12048] Create SQL logic for CreateNotificationAction and a…

6f0f999

…dd relevant tests for v9 migration (TEAMMATES#12077)

samuelfangjw pushed a commit that referenced this pull request Feb 26, 2023

[#12048] Create SQL logic for CreateNotificationAction and add releva…

2a5ee7a

…nt tests for v9 migration (#12077)

samuelfangjw pushed a commit that referenced this pull request Apr 8, 2023

[#12048] Create SQL logic for CreateNotificationAction and add releva…

69d2f71

…nt tests for v9 migration (#12077)

wkurniawan07 added the c.Task Other non-user-facing works, e.g. refactoring, adding tests label Jan 21, 2024

wkurniawan07 added this to the V9.0.0-beta.0 milestone Jan 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create SQL logic for CreateNotificationAction and add relevant tests for v9 migration #12077

Create SQL logic for CreateNotificationAction and add relevant tests for v9 migration #12077

hhdqirui commented Feb 11, 2023

samuelfangjw Feb 11, 2023

samuelfangjw Feb 11, 2023

samuelfangjw Feb 11, 2023

samuelfangjw Feb 11, 2023

jianhandev Feb 12, 2023


		private static final NotificationsLogic instance = new NotificationsLogic();

		private final NotificationsDb notificationsDb = NotificationsDb.inst();


		assertThrows(InvalidParametersException.class, () -> notificationsDb.createNotification(invalidNotification2));

		______TS("failure: empty message");

Create SQL logic for CreateNotificationAction and add relevant tests for v9 migration #12077

Create SQL logic for CreateNotificationAction and add relevant tests for v9 migration #12077

Conversation

hhdqirui commented Feb 11, 2023

samuelfangjw Feb 11, 2023

Choose a reason for hiding this comment

samuelfangjw Feb 11, 2023

Choose a reason for hiding this comment

samuelfangjw Feb 11, 2023

Choose a reason for hiding this comment

samuelfangjw Feb 11, 2023

Choose a reason for hiding this comment

jianhandev Feb 12, 2023

Choose a reason for hiding this comment