[#11572] Notification: GET API test and access control #11648

ziqing26 · 2022-03-14T05:43:01Z

Part of #11572

Outline of Solution

Add tests for NotificationAttributes
Add notifications to typical data bundle
Add tests for GET API

Has not include:

isfetchingall related feature / tests.

jianhandev

Good test coverage!

src/test/java/teammates/ui/webapi/GetNotificationActionTest.java

src/test/resources/data/typicalDataBundle.json

jianhandev · 2022-03-19T09:07:33Z

src/test/java/teammates/ui/webapi/GetNotificationActionTest.java

+        ______TS("Request to fetch notification");
+        int expectedNumberOfNotifications = 4;
+        InstructorAttributes instructor = typicalBundle.instructors.get("instructor1OfCourse1");


The expected number of active notifications here is dependent on the current time, we are in 2022 but two years later the end time for some of these notifications e.g. notification-4 would have passed, then this test will fail 😄 .

"notification-4": { "notificationId": "notification-4", "startTime": "2011-04-04T00:00:00Z", "endTime": "2024-04-04T00:00:00Z", "createdAt": "2011-01-01T00:00:00Z", "updatedAt": "2011-01-01T00:00:00Z", "type": "MAINTENANCE", "targetUser": "GENERAL", "title": "The note of maintenance", "message": "The content of maintenance", "shown": false },

Would it be okay if I change the endtime to for example 2099?

Would it be okay if I change the endtime to for example 2099?

This has been updated in the newly pushed code. Thank you!

src/test/java/teammates/ui/webapi/GetNotificationActionTest.java

jianhandev · 2022-03-19T09:52:38Z

src/test/java/teammates/common/datatransfer/attributes/NotificationAttributesTest.java

+    private static final Instant STARTTIME_ONE = Instant.now().plusSeconds(3600);
+    private static final Instant ENDTIME_ONE = Instant.now().plusSeconds(7200);
+    private static final Instant STARTTIME_TWO = Instant.now().plusSeconds(1000);
+    private static final Instant ENDTIME_TWO = Instant.now().plusSeconds(10000);
+
+    @Test
+    public void testValueOf_withAllFieldPopulatedNotificationAttributes_shouldGenerateAttributesCorrectly() {
+        Notification notification = new Notification("valid-notification-id",
+                STARTTIME_ONE, ENDTIME_ONE,
+                NotificationType.DEPRECATION, NotificationTargetUser.INSTRUCTOR,
+                "valid notification title", "valid notification message", false,
+                Instant.now(), Instant.now());


If these data are going to be used frequently across different tests, you may want to use a helper method like generateTypicalNotificationAttributesObject to generate a NotificationAttributes object for you and reuse them across tests.

The builder might have slight variations in certain fields and in numbers of fields. If we abstract a method out for that, my concern is it will become just a wrapper around the builder, which might not be necessary.

Yes, for cases where we need to test with different values we can build the NotificationAttributes separately, but for typical NotificationAttributes we can use a helper method method like generateTypicalNotificationAttributesObject to improve readability.

I have highlighted a few examples below, there are many others places where we can simply call generateTypicalNotificationAttributesObject to generate a default NotificationAttributes for us.

Thank you for the suggestion! I have changed accordingly in the new commit.

src/test/java/teammates/common/datatransfer/attributes/NotificationAttributesTest.java

ziqing26 · 2022-03-22T05:32:02Z

@jianhandev In the new commit, I add methods in NotificationAttributesTest to abstract the creation of typical attributes and remove declaration of Instant variables. Meanwhile I have also added tests for FieldValidator as that was missing previously. Thank you for your help!

jianhandev · 2022-03-22T12:05:51Z

src/test/java/teammates/common/util/FieldValidatorTest.java

+        String notificationType = "invalid type";
+        assertEquals("\"invalid type\" is not an accepted notification type to TEAMMATES. ",
+                FieldValidator.getInvalidityInfoForNotificationType(notificationType));


Suggested change

String notificationType = "invalid type";

assertEquals("\"invalid type\" is not an accepted notification type to TEAMMATES. ",

FieldValidator.getInvalidityInfoForNotificationType(notificationType));

String notificationType = "invalid type";

assertEquals(notificationType + " is not an accepted notification type to TEAMMATES. ",

FieldValidator.getInvalidityInfoForNotificationType(notificationType));

The error messages have double quotes around the variable. So we might still need to keep the \" there.

src/test/java/teammates/common/util/FieldValidatorTest.java

jianhandev · 2022-03-22T12:07:30Z

src/test/java/teammates/common/util/FieldValidatorTest.java

+        String user = "invalid user";
+        assertEquals("\"invalid user\" is not an accepted notification target user to TEAMMATES. ",
+                FieldValidator.getInvalidityInfoForNotificationTargetUser(user));


Suggested change

String user = "invalid user";

assertEquals("\"invalid user\" is not an accepted notification target user to TEAMMATES. ",

FieldValidator.getInvalidityInfoForNotificationTargetUser(user));

String invalidUser = "invalid user";

assertEquals(invalidUser + " is not an accepted notification target user to TEAMMATES. ",

FieldValidator.getInvalidityInfoForNotificationTargetUser(invalidUser));

src/test/java/teammates/common/util/FieldValidatorTest.java

* Add test for NotificationAttributes * Add notifications to typicalDataBundles and init GET api test * Add tests for GET api * Add access control and its tests for GET api * Update data in dataBundle and add more abstractions * Refactor NotificationAttributesTest and add tests for FieldValidator

TEAMMATES deleted a comment from teammates-bot Mar 14, 2022

NicolasCwy mentioned this pull request Mar 17, 2022

[#11572] Add delete notification route #11650

Merged

jianhandev force-pushed the notification-feature branch from cd49a85 to 43fa06b Compare March 17, 2022 08:45

ziqing26 force-pushed the 11572-test-notification-get-api branch from 5a33221 to 2d962e6 Compare March 18, 2022 07:09

ziqing26 added 5 commits March 18, 2022 15:10

Add test for NotificationAttributes

349a91b

Add notifications to typicalDataBundles and init GET api test

d146b69

Add tests for GET api

7270e4c

Add access control and its tests for GET api

b268b5d

Fix lint errors

8e13258

ziqing26 force-pushed the 11572-test-notification-get-api branch from 2d962e6 to 8e13258 Compare March 18, 2022 07:11

ziqing26 marked this pull request as ready for review March 18, 2022 07:11

ziqing26 requested review from NicolasCwy, zhaojj2209, fsgmhoward and jianhandev March 18, 2022 07:11

TEAMMATES deleted a comment from teammates-bot Mar 18, 2022

ziqing26 added the s.ToReview The PR is waiting for review(s) label Mar 19, 2022

jianhandev requested changes Mar 19, 2022

View reviewed changes

jianhandev mentioned this pull request Mar 19, 2022

[#11572] Notification feature - Database Test #11651

Merged

Update data in dataBundle and add more abstractions

6be5200

ziqing26 requested a review from jianhandev March 20, 2022 12:16

NicolasCwy mentioned this pull request Mar 20, 2022

[#11572] Notification Feature - Add tests for notification POST route #11660

Merged

2 tasks

Fix style

7da5380

This was referenced Mar 20, 2022

[#11572] Notification Feature - Add mark as read to notification GET API #11661

Merged

[#11572] Notification Feature - POST API for readNotifications in AccountEntity #11664

Merged

jianhandev reviewed Mar 21, 2022

View reviewed changes

src/test/java/teammates/common/datatransfer/attributes/NotificationAttributesTest.java Outdated Show resolved Hide resolved

jianhandev reviewed Mar 21, 2022

View reviewed changes

src/test/java/teammates/common/datatransfer/attributes/NotificationAttributesTest.java Outdated Show resolved Hide resolved

jianhandev reviewed Mar 21, 2022

View reviewed changes

src/test/java/teammates/common/datatransfer/attributes/NotificationAttributesTest.java Outdated Show resolved Hide resolved

jianhandev reviewed Mar 21, 2022

View reviewed changes

src/test/java/teammates/common/datatransfer/attributes/NotificationAttributesTest.java Outdated Show resolved Hide resolved

jianhandev 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 Mar 22, 2022

Refactor NotificationAttributesTest and add tests for FieldValidator

c11b70a

ziqing26 added s.ToReview The PR is waiting for review(s) and removed s.Ongoing The PR is being worked on by the author(s) labels Mar 22, 2022

ziqing26 requested a review from jianhandev March 22, 2022 05:27

jianhandev reviewed Mar 22, 2022

View reviewed changes

Use variable in string concatenation and update expected number

a33ab4e

ziqing26 requested a review from jianhandev March 22, 2022 13:12

jianhandev approved these changes Mar 22, 2022

View reviewed changes

jianhandev merged commit 2934849 into TEAMMATES:notification-feature Mar 22, 2022

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.ToReview The PR is waiting for review(s) labels Apr 6, 2022

wkurniawan07 added this to the V8.13.0 milestone Apr 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#11572] Notification: GET API test and access control #11648

[#11572] Notification: GET API test and access control #11648

ziqing26 commented Mar 14, 2022 •

edited

Loading

jianhandev left a comment

jianhandev Mar 19, 2022 •

edited

Loading

ziqing26 Mar 20, 2022

ziqing26 Mar 21, 2022

jianhandev Mar 19, 2022

ziqing26 Mar 20, 2022

jianhandev Mar 21, 2022 •

edited

Loading

ziqing26 Mar 22, 2022

ziqing26 commented Mar 22, 2022

jianhandev Mar 22, 2022

ziqing26 Mar 22, 2022

jianhandev Mar 22, 2022

[#11572] Notification: GET API test and access control #11648

[#11572] Notification: GET API test and access control #11648

Conversation

ziqing26 commented Mar 14, 2022 • edited Loading

jianhandev left a comment

Choose a reason for hiding this comment

jianhandev Mar 19, 2022 • edited Loading

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

jianhandev Mar 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziqing26 commented Mar 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziqing26 commented Mar 14, 2022 •

edited

Loading

jianhandev Mar 19, 2022 •

edited

Loading

jianhandev Mar 21, 2022 •

edited

Loading