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

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

Merged
merged 9 commits into from
Mar 22, 2022
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
349a91b
Add test for NotificationAttributes
ziqing26 Mar 13, 2022
d146b69
Add notifications to typicalDataBundles and init GET api test
ziqing26 Mar 13, 2022
7270e4c
Add tests for GET api
ziqing26 Mar 13, 2022
b268b5d
Add access control and its tests for GET api
ziqing26 Mar 14, 2022
8e13258
Fix lint errors
ziqing26 Mar 14, 2022
6be5200
Update data in dataBundle and add more abstractions
ziqing26 Mar 20, 2022
7da5380
Fix style
ziqing26 Mar 20, 2022
c11b70a
Refactor NotificationAttributesTest and add tests for FieldValidator
ziqing26 Mar 22, 2022
a33ab4e
Use variable in string concatenation and update expected number
ziqing26 Mar 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/main/java/teammates/common/datatransfer/DataBundle.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import teammates.common.datatransfer.attributes.FeedbackResponseCommentAttributes;
import teammates.common.datatransfer.attributes.FeedbackSessionAttributes;
import teammates.common.datatransfer.attributes.InstructorAttributes;
import teammates.common.datatransfer.attributes.NotificationAttributes;
import teammates.common.datatransfer.attributes.StudentAttributes;
import teammates.common.datatransfer.attributes.StudentProfileAttributes;

Expand All @@ -31,4 +32,5 @@ public class DataBundle {
public Map<String, FeedbackResponseAttributes> feedbackResponses = new LinkedHashMap<>();
public Map<String, FeedbackResponseCommentAttributes> feedbackResponseComments = new LinkedHashMap<>();
public Map<String, StudentProfileAttributes> profiles = new LinkedHashMap<>();
public Map<String, NotificationAttributes> notifications = new LinkedHashMap<>();
}
4 changes: 3 additions & 1 deletion src/main/java/teammates/common/datatransfer/attributes/NotificationAttributes.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ public void setUpdatedAt(Instant updatedAt) {
}

/**
* Sorts the list of notifications by the start time.
* Sorts the list of notifications by the start time, with the latest as the first element.
*/
public static void sortByStartTime(List<NotificationAttributes> notifications) {
notifications.sort(Comparator.comparing(NotificationAttributes::getStartTime));
Expand Down Expand Up @@ -365,6 +365,8 @@ public B withEndTime(Instant endTime) {
}

public B withType(NotificationType type) {
assert type != null;

updateOptions.typeOption = UpdateOption.of(type);
return thisBuilder;
}
Expand Down
9 changes: 9 additions & 0 deletions src/main/java/teammates/logic/core/DataBundleLogic.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import teammates.common.datatransfer.attributes.FeedbackResponseCommentAttributes;
import teammates.common.datatransfer.attributes.FeedbackSessionAttributes;
import teammates.common.datatransfer.attributes.InstructorAttributes;
import teammates.common.datatransfer.attributes.NotificationAttributes;
import teammates.common.datatransfer.attributes.StudentAttributes;
import teammates.common.datatransfer.attributes.StudentProfileAttributes;
import teammates.common.exception.InvalidParametersException;
Expand All @@ -33,6 +34,7 @@
import teammates.storage.api.FeedbackResponsesDb;
import teammates.storage.api.FeedbackSessionsDb;
import teammates.storage.api.InstructorsDb;
import teammates.storage.api.NotificationsDb;
import teammates.storage.api.ProfilesDb;
import teammates.storage.api.StudentsDb;

Expand All @@ -55,6 +57,7 @@ public final class DataBundleLogic {
private final FeedbackQuestionsDb fqDb = FeedbackQuestionsDb.inst();
private final FeedbackResponsesDb frDb = FeedbackResponsesDb.inst();
private final FeedbackResponseCommentsDb fcDb = FeedbackResponseCommentsDb.inst();
private final NotificationsDb nfDb = NotificationsDb.inst();

private DataBundleLogic() {
// prevent initialization
Expand Down Expand Up @@ -90,6 +93,7 @@ public DataBundle persistDataBundle(DataBundle dataBundle) throws InvalidParamet
Collection<FeedbackQuestionAttributes> questions = dataBundle.feedbackQuestions.values();
Collection<FeedbackResponseAttributes> responses = dataBundle.feedbackResponses.values();
Collection<FeedbackResponseCommentAttributes> responseComments = dataBundle.feedbackResponseComments.values();
Collection<NotificationAttributes> notifications = dataBundle.notifications.values();

// For ensuring only one account per Google ID is created
Map<String, AccountAttributes> googleIdAccountMap = new HashMap<>();
Expand All @@ -115,6 +119,7 @@ public DataBundle persistDataBundle(DataBundle dataBundle) throws InvalidParamet

List<FeedbackResponseAttributes> newFeedbackResponses = frDb.putEntities(responses);
List<FeedbackResponseCommentAttributes> newFeedbackResponseComments = fcDb.putEntities(responseComments);
List<NotificationAttributes> newNotifications = nfDb.putEntities(notifications);

updateDataBundleValue(newAccounts, dataBundle.accounts);
updateDataBundleValue(newAccountRequests, dataBundle.accountRequests);
Expand All @@ -126,6 +131,7 @@ public DataBundle persistDataBundle(DataBundle dataBundle) throws InvalidParamet
updateDataBundleValue(createdQuestions, dataBundle.feedbackQuestions);
updateDataBundleValue(newFeedbackResponses, dataBundle.feedbackResponses);
updateDataBundleValue(newFeedbackResponseComments, dataBundle.feedbackResponseComments);
updateDataBundleValue(newNotifications, dataBundle.notifications);

return dataBundle;

Expand Down Expand Up @@ -379,6 +385,9 @@ public void removeDataBundle(DataBundle dataBundle) {
dataBundle.accountRequests.values().forEach(accountRequest -> {
accountRequestsDb.deleteAccountRequest(accountRequest.getEmail(), accountRequest.getInstitute());
});
dataBundle.notifications.values().forEach(notification -> {
nfDb.deleteNotification(notification.getNotificationId());
});
}

private void deleteCourses(Collection<CourseAttributes> courses) {
Expand Down
17 changes: 15 additions & 2 deletions src/main/java/teammates/ui/webapi/GetNotificationAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,28 @@
public class GetNotificationAction extends Action {

private static final String INVALID_TARGET_USER = "Target user can only be STUDENT or INSTRUCTOR.";
private static final String UNAUTHORIZED_ACCESS = "You are not allowed to view this resource!";

@Override
AuthType getMinAuthLevel() {
return AuthType.PUBLIC;
return AuthType.LOGGED_IN;
}

@Override
void checkSpecificAccessControl() throws UnauthorizedAccessException {
// Any user can get notifications as long as its parameters are valid
if (userInfo.isAdmin) {
return;
}
String targetUserString = getRequestParamValue(Const.ParamsNames.NOTIFICATION_TARGET_USER);
String targetUserErrorMessage = FieldValidator.getInvalidityInfoForNotificationTargetUser(targetUserString);
if (!targetUserErrorMessage.isEmpty()) {
throw new InvalidHttpParameterException(targetUserErrorMessage);
}
NotificationTargetUser targetUser = NotificationTargetUser.valueOf(targetUserString);
if (targetUser == NotificationTargetUser.INSTRUCTOR && userInfo.isStudent
|| targetUser == NotificationTargetUser.STUDENT && userInfo.isInstructor) {
throw new UnauthorizedAccessException(UNAUTHORIZED_ACCESS);
}
}

@Override
Expand Down
269 changes: 269 additions & 0 deletions src/test/java/teammates/common/datatransfer/attributes/NotificationAttributesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,269 @@
package teammates.common.datatransfer.attributes;

import java.time.Instant;

import org.testng.annotations.Test;

import teammates.common.datatransfer.NotificationTargetUser;
import teammates.common.datatransfer.NotificationType;
import teammates.storage.entity.Notification;
import teammates.test.BaseTestCase;

/**
* SUT: {@link NotificationAttributes}.
*/
public class NotificationAttributesTest extends BaseTestCase {
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);
ziqing26 marked this conversation as resolved.
Show resolved Hide resolved

@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());
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@jianhandev jianhandev Mar 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

NotificationAttributes nfa = NotificationAttributes.valueOf(notification);
assertEquals(notification.getNotificationId(), nfa.getNotificationId());
assertEquals(notification.getStartTime(), nfa.getStartTime());
assertEquals(notification.getEndTime(), nfa.getEndTime());
assertEquals(notification.getType(), nfa.getType());
assertEquals(notification.getTargetUser(), nfa.getTargetUser());
assertEquals(notification.getTitle(), nfa.getTitle());
assertEquals(notification.getMessage(), nfa.getMessage());
assertEquals(notification.isShown(), nfa.isShown());
assertEquals(notification.getCreatedAt(), nfa.getCreatedAt());
assertEquals(notification.getUpdatedAt(), nfa.getUpdatedAt());
}

@Test
public void testBuilder_withNullArguments_shouldThrowException() {
assertThrows(AssertionError.class, () -> {
NotificationAttributes
.builder(null)
.build();
});

assertThrows(AssertionError.class, () -> {
NotificationAttributes
.builder("notificationId")
.withStartTime(null)
.build();
});

assertThrows(AssertionError.class, () -> {
NotificationAttributes
.builder("notificationId")
.withEndTime(null)
.build();
});

assertThrows(AssertionError.class, () -> {
NotificationAttributes
.builder("notificationId")
.withType(null)
.build();
});

assertThrows(AssertionError.class, () -> {
NotificationAttributes
.builder("notificationId")
.withTargetUser(null)
.build();
});

assertThrows(AssertionError.class, () -> {
NotificationAttributes
.builder("notificationId")
.withTitle(null)
.build();
});

assertThrows(AssertionError.class, () -> {
NotificationAttributes
.builder("notificationId")
.withMessage(null)
.build();
});
}

@Test
public void testBuilder_withTypicalData_shouldBuildCorrectAttributes() {
Notification notification = new Notification(STARTTIME_ONE, ENDTIME_ONE,
NotificationType.TIPS, NotificationTargetUser.GENERAL,
"Another tip for usage", "Here the message starts");
NotificationAttributes nfa = NotificationAttributes.valueOf(notification);
ziqing26 marked this conversation as resolved.
Show resolved Hide resolved
assertEquals(STARTTIME_ONE, nfa.getStartTime());
assertEquals(ENDTIME_ONE, nfa.getEndTime());
assertEquals(NotificationType.TIPS, nfa.getType());
assertEquals(NotificationTargetUser.GENERAL, nfa.getTargetUser());
assertEquals("Another tip for usage", nfa.getTitle());
assertEquals("Here the message starts", nfa.getMessage());
}

@Test
public void testCopyConstructor_shouldDoDeepCopyOfNotificationDetails() {
NotificationAttributes nfa1 = NotificationAttributes.builder("notificationId")
.withStartTime(STARTTIME_ONE)
.withEndTime(ENDTIME_ONE)
.withType(NotificationType.DEPRECATION)
.withTargetUser(NotificationTargetUser.INSTRUCTOR)
.withTitle("valid notification title")
.withMessage("The first message")
.build();
NotificationAttributes nfa2 = nfa1.getCopy();
nfa2.setMessage("The second message");

assertEquals(nfa1.getMessage(), "The first message");
assertEquals(nfa2.getMessage(), "The second message");
}

@Test
public void testUpdateOptions_withTypicalUpdateOptions_shouldUpdateAttributeCorrectly() {
NotificationAttributes.UpdateOptions updateOptions =
NotificationAttributes.updateOptionsBuilder("notificationId")
.withStartTime(STARTTIME_TWO)
.withEndTime(ENDTIME_TWO)
.withType(NotificationType.VERSION_NOTE)
.withTargetUser(NotificationTargetUser.STUDENT)
.withTitle("The edited title")
.withMessage("The edited message")
.build();

assertEquals("notificationId", updateOptions.getNotificationId());

NotificationAttributes notificationAttributes =
NotificationAttributes.builder("notificationId")
.withStartTime(STARTTIME_ONE)
.withEndTime(ENDTIME_ONE)
.withType(NotificationType.DEPRECATION)
.withTargetUser(NotificationTargetUser.INSTRUCTOR)
.withTitle("valid notification title")
.withMessage("The first message")
.build();
ziqing26 marked this conversation as resolved.
Show resolved Hide resolved

notificationAttributes.update(updateOptions);

assertEquals(STARTTIME_TWO, notificationAttributes.getStartTime());
assertEquals(ENDTIME_TWO, notificationAttributes.getEndTime());
assertEquals(NotificationType.VERSION_NOTE, notificationAttributes.getType());
assertEquals(NotificationTargetUser.STUDENT, notificationAttributes.getTargetUser());
assertEquals("The edited title", notificationAttributes.getTitle());
assertEquals("The edited message", notificationAttributes.getMessage());
}

@Test
public void testUpdateOptionsBuilder_withNullInput_shouldFailWithAssertionError() {
assertThrows(AssertionError.class, () ->
NotificationAttributes.updateOptionsBuilder((String) null));
assertThrows(AssertionError.class, () ->
NotificationAttributes.updateOptionsBuilder("notificationId")
.withStartTime(null));
assertThrows(AssertionError.class, () ->
NotificationAttributes.updateOptionsBuilder("notificationId")
.withEndTime(null));
assertThrows(AssertionError.class, () ->
NotificationAttributes.updateOptionsBuilder("notificationId")
.withType(null));
assertThrows(AssertionError.class, () ->
NotificationAttributes.updateOptionsBuilder("notificationId")
.withTargetUser(null));
assertThrows(AssertionError.class, () ->
NotificationAttributes.updateOptionsBuilder("notificationId")
.withTitle(null));
assertThrows(AssertionError.class, () ->
NotificationAttributes.updateOptionsBuilder("notificationId")
.withMessage(null));
}

@Test
public void testEquals() {
NotificationAttributes notificationAttributes =
NotificationAttributes.builder("notificationId")
.withStartTime(STARTTIME_ONE)
.withEndTime(ENDTIME_ONE)
.withType(NotificationType.DEPRECATION)
.withTargetUser(NotificationTargetUser.INSTRUCTOR)
.withTitle("valid notification title")
.withMessage("valid message")
.build();
ziqing26 marked this conversation as resolved.
Show resolved Hide resolved

// When the two notifications are exact copies
NotificationAttributes notificationAttributesCopy = notificationAttributes.getCopy();

assertTrue(notificationAttributes.equals(notificationAttributesCopy));

// When the two notifications have same values but created at different time
NotificationAttributes notificationAttributesSimilar =
NotificationAttributes.builder("notificationId")
.withStartTime(STARTTIME_ONE)
.withEndTime(ENDTIME_ONE)
.withType(NotificationType.DEPRECATION)
.withTargetUser(NotificationTargetUser.INSTRUCTOR)
.withTitle("valid notification title")
.withMessage("valid message")
.build();

assertTrue(notificationAttributes.equals(notificationAttributesSimilar));

NotificationAttributes notificationAttributesDifferent =
NotificationAttributes.builder("differentId")
.withStartTime(STARTTIME_ONE)
.withEndTime(ENDTIME_ONE)
.withType(NotificationType.DEPRECATION)
.withTargetUser(NotificationTargetUser.INSTRUCTOR)
.withTitle("valid notification title")
.withMessage("valid message")
.build();

assertFalse(notificationAttributes.equals(notificationAttributesDifferent));

// When the other object is of different class
assertFalse(notificationAttributes.equals(3));
}

@Test
public void testHashCode() {
NotificationAttributes notificationAttributes =
NotificationAttributes.builder("notificationId")
.withStartTime(STARTTIME_ONE)
.withEndTime(ENDTIME_ONE)
.withType(NotificationType.DEPRECATION)
.withTargetUser(NotificationTargetUser.INSTRUCTOR)
.withTitle("valid notification title")
.withMessage("valid message")
.build();

// When the two notifications are exact copies
NotificationAttributes notificationAttributesCopy = notificationAttributes.getCopy();

assertTrue(notificationAttributes.hashCode() == notificationAttributesCopy.hashCode());

// When the two notifications have same values but created at different time
NotificationAttributes notificationAttributesSimilar =
NotificationAttributes.builder("notificationId")
.withStartTime(STARTTIME_ONE)
.withEndTime(ENDTIME_ONE)
.withType(NotificationType.DEPRECATION)
.withTargetUser(NotificationTargetUser.INSTRUCTOR)
.withTitle("valid notification title")
.withMessage("valid message")
.build();

assertTrue(notificationAttributes.hashCode() == notificationAttributesSimilar.hashCode());

NotificationAttributes notificationAttributesDifferent =
NotificationAttributes.builder("differentId")
.withStartTime(STARTTIME_ONE)
.withEndTime(ENDTIME_ONE)
.withType(NotificationType.DEPRECATION)
.withTargetUser(NotificationTargetUser.INSTRUCTOR)
.withTitle("valid notification title")
.withMessage("valid message")
.build();

assertFalse(notificationAttributes.hashCode() == notificationAttributesDifferent.hashCode());
}
}
Loading