Skip to content

Renames notification-center methods #120

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

Merged
merged 3 commits into from
Jul 2, 2018
Merged

Renames notification-center methods #120

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
de92ec6
Renames notification-center methods
rashidsp Jun 29, 2018
cf765ad
review changes
rashidsp Jul 2, 2018
62223ae
Merge branch 'master' into rashid/NC-deprecated-methods
mikeproeng37 Jul 2, 2018
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
32 changes: 29 additions & 3 deletions src/Optimizely/Notification/NotificationCenter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?php
/**
* Copyright 2017, Optimizely Inc and Contributors
* Copyright 2017-2018, Optimizely Inc and Contributors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -117,12 +117,25 @@ public function removeNotificationListener($notification_id)
return false;
}

/**
* Logs deprecation message
*
* @deprecated Use 'clearNotificationListeners' instead.
*/
public function clearNotifications($notification_type){
$this->_logger->log(
Logger::WARNING,
"'clearNotifications' is deprecated. Call 'clearNotificationListeners' instead."
);
$this->clearNotificationListeners($notification_type);
}

/**
* Removes all notification callbacks for the given notification type
*
* @param string $notification_type One of the constants defined in NotificationType
*/
public function clearNotifications($notification_type)
public function clearNotificationListeners($notification_type)
{
if (!NotificationType::isNotificationTypeValid($notification_type)) {
$this->_logger->log(Logger::ERROR, "Invalid notification type.");
Expand All @@ -134,11 +147,24 @@ public function clearNotifications($notification_type)
$this->_logger->log(Logger::INFO, "All callbacks for notification type '{$notification_type}' have been removed.");
}

/**
* Logs deprecation message
*
* @deprecated Use 'clearAllNotificationListeners' instead.
*/
public function cleanAllNotifications(){
$this->_logger->log(
Logger::WARNING,
"'cleanAllNotifications' is deprecated. Call 'clearAllNotificationListeners' instead."
);
$this->clearAllNotificationListeners();
}

/**
* Removes all notifications for all notification types
* from the notification center
*/
public function cleanAllNotifications()
public function clearAllNotificationListeners()
{
foreach (array_values(NotificationType::getAll()) as $type) {
$this->_notifications[$type] = [];
Expand Down
83 changes: 65 additions & 18 deletions tests/NotificationTests/NotificationCenterTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public function testAddNotificationWithInvalidParams()
public function testAddNotificationWithValidTypeAndCallback()
{
$notificationType = NotificationType::ACTIVATE;
$this->notificationCenterObj->cleanAllNotifications();
$this->notificationCenterObj->clearAllNotificationListeners();

////////////////////////////////////////////////////////////////////////////////////////////////////////////
// === should add, log and return notification ID when a plain function is passed as an argument === //
Expand Down Expand Up @@ -135,7 +135,7 @@ function () {

public function testAddNotificationForMultipleNotificationTypes()
{
$this->notificationCenterObj->cleanAllNotifications();
$this->notificationCenterObj->clearAllNotificationListeners();

//////////////////////////////////////////////////////////////////////////////////////////////////////////////////
// === should add, log and return notification ID when a valid callback is added for each notification type === //
Expand Down Expand Up @@ -179,7 +179,7 @@ function () {

public function testAddNotificationForMultipleCallbacksForASingleNotificationType()
{
$this->notificationCenterObj->cleanAllNotifications();
$this->notificationCenterObj->clearAllNotificationListeners();

///////////////////////////////////////////////////////////////////////////////////////
// === should add, log and return notification ID when multiple valid callbacks
Expand Down Expand Up @@ -243,7 +243,7 @@ public function testAddNotificationThatAlreadyAddedCallbackIsNotReAdded()

$functionToSend = function () {
};
$this->notificationCenterObj->cleanAllNotifications();
$this->notificationCenterObj->clearAllNotificationListeners();

///////////////////////////////////////////////////////////////////////////
// ===== verify that a variable method with same body isn't re-added ===== //
Expand Down Expand Up @@ -313,7 +313,7 @@ public function testAddNotificationThatAlreadyAddedCallbackIsNotReAdded()

public function testRemoveNotification()
{
$this->notificationCenterObj->cleanAllNotifications();
$this->notificationCenterObj->clearAllNotificationListeners();

// add a callback for multiple notification types
$this->assertSame(
Expand Down Expand Up @@ -407,10 +407,35 @@ function () {
);
}

public function testClearNotifications()
public function testclearNotificationsAndVerifyThatclearNotificationListenersWithArgsIsCalled()
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: should be testClear...

{
# Mock NotificationCenter
$this->notificationCenterMock = $this->getMockBuilder(NotificationCenter::class)
->setConstructorArgs(array($this->loggerMock, $this->errorHandlerMock))
->setMethods(array('clearNotificationListeners'))
->getMock();

# Log deprecation message
$this->loggerMock->expects($this->at(0))
->method('log')
->with(
Logger::WARNING,
sprintf("'clearNotifications' is deprecated. Call 'clearNotificationListeners' instead.")
);

$this->notificationCenterMock->expects($this->once())
->method('clearNotificationListeners')
->with(
NotificationType::ACTIVATE
);

$this->notificationCenterMock->clearNotifications(NotificationType::ACTIVATE);
}

public function testclearNotificationListeners()
Copy link
Contributor

Choose a reason for hiding this comment

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

Same typo here

{
// ensure that notifications length is zero for each notification type
$this->notificationCenterObj->cleanAllNotifications();
$this->notificationCenterObj->clearAllNotificationListeners();

// add a callback for multiple notification types
$this->notificationCenterObj->addNotificationListener(
Expand Down Expand Up @@ -457,7 +482,7 @@ function () {
->method('handleError')
->with(new InvalidNotificationTypeException('Invalid notification type.'));

$this->assertNull($this->notificationCenterObj->clearNotifications($invalid_type));
$this->assertNull($this->notificationCenterObj->clearNotificationListeners($invalid_type));

// Verify that notifications length for NotificationType::ACTIVATE is still 2
$this->assertSame(
Expand All @@ -482,7 +507,7 @@ function () {
sprintf("All callbacks for notification type '%s' have been removed.", NotificationType::ACTIVATE)
);

$this->notificationCenterObj->clearNotifications(NotificationType::ACTIVATE);
$this->notificationCenterObj->clearNotificationListeners(NotificationType::ACTIVATE);

// Verify that notifications length for NotificationType::ACTIVATE is now 0
$this->assertSame(
Expand All @@ -499,11 +524,11 @@ function () {
///////////////////////////////////////////////////////////////////////////////////////////////////////////
// == Verify that no error is thrown when clearNotification is called again for the same notification type === //
///////////////////////////////////////////////////////////////////////////////////////////////////////////
$this->notificationCenterObj->clearNotifications(NotificationType::ACTIVATE);
$this->notificationCenterObj->clearNotificationListeners(NotificationType::ACTIVATE);
}


public function testCleanAllNotifications()
public function testclearAllNotificationListeners()
{
// using a new notification center object to avoid using the method being tested,
// to reset notifications list
Expand Down Expand Up @@ -558,10 +583,10 @@ function () {
);

////////////////////////////////////////////////////////////////////////////////////////////////////
// === verify that cleanAllNotifications removes all notifications for each notification type === //
// === verify that clearAllNotificationListeners removes all notifications for each notification type === //
////////////////////////////////////////////////////////////////////////////////////////////////////

$notificationCenterA->cleanAllNotifications();
$notificationCenterA->clearAllNotificationListeners();

// verify that notifications length for each type is now set to 0
$this->assertSame(
Expand All @@ -574,15 +599,37 @@ function () {
);

///////////////////////////////////////////////////////////////////////////////////////
//=== verify that cleanAllNotifications doesn't throw an error when called again === //
//=== verify that clearAllNotificationListeners doesn't throw an error when called again === //
///////////////////////////////////////////////////////////////////////////////////////
$notificationCenterA->cleanAllNotifications();
$notificationCenterA->clearAllNotificationListeners();
}

public function testcleanAllNotificationsAndVerifyThatclearAllNotificationListenersIsCalled()
Copy link
Contributor

Choose a reason for hiding this comment

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

testClean...

{
# Mock NotificationCenter
$this->notificationCenterMock = $this->getMockBuilder(NotificationCenter::class)
->setConstructorArgs(array($this->loggerMock, $this->errorHandlerMock))
->setMethods(array('clearAllNotificationListeners'))
->getMock();

# Log deprecation message
$this->loggerMock->expects($this->at(0))
->method('log')
->with(
Logger::WARNING,
sprintf("'cleanAllNotifications' is deprecated. Call 'clearAllNotificationListeners' instead.")
);

$this->notificationCenterMock->expects($this->once())
->method('clearAllNotificationListeners');

$this->notificationCenterMock->cleanAllNotifications();
}

public function testsendNotificationsGivenLessThanExpectedNumberOfArguments()
Copy link
Contributor

Choose a reason for hiding this comment

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

Can probably fix this as well

{
$clientObj = new FireNotificationTester;
$this->notificationCenterObj->cleanAllNotifications();
$this->notificationCenterObj->clearAllNotificationListeners();

// add a notification callback with arguments
$this->notificationCenterObj->addNotificationListener(
Expand Down Expand Up @@ -610,7 +657,7 @@ public function testsendNotificationsAndVerifyThatAllCallbacksWithoutArgsAreCall
->setMethods(array('decision_callback_no_args', 'decision_callback_no_args_2', 'track_callback_no_args'))
->getMock();

$this->notificationCenterObj->cleanAllNotifications();
$this->notificationCenterObj->clearAllNotificationListeners();

//add notification callbacks
$this->notificationCenterObj->addNotificationListener(
Expand Down Expand Up @@ -661,7 +708,7 @@ public function testsendNotificationsAndVerifyThatAllCallbacksWithArgsAreCalled(
->setMethods(array('decision_callback_with_args', 'decision_callback_with_args_2', 'track_callback_no_args'))
->getMock();

$this->notificationCenterObj->cleanAllNotifications();
$this->notificationCenterObj->clearAllNotificationListeners();

//add notification callbacks with args
$this->notificationCenterObj->addNotificationListener(
Expand Down