Skip to content

Loading…

Proposing removal of subscription record upon unsubscribe #3295

Closed
wants to merge 2 commits into from

2 participants

@quocvu

Currently both subscribe and unsubscribe operations to a feed execute the same code resulting into the same behavior. As a result subscription records created in the database have the same state and we can't tell which ones were unsubscribed and which ones are still subscribed.

I am proposing to keep the subscribe operation as currently implemented but tweak to unsubscribe operation as the following:
1) when the unsubscribe request is sent to the hub, update the subscription record STATE column as to_delete
2) once the hub sends the callback and it's been verified, delete the subscription record

Please let me know if this fit the original design ideas.

@weierophinney
Zend Framework member

Looks reasonable. @quocvu can you please provide some unit tests?

@quocvu

Both changes that I suggested manifest only inside the database. Furthermore it happens on a DB table that is unknown to Zend as it injected by the developer.

@weierophinney could you please advise on how we can unit test such case with your current setup?
Thanks

@weierophinney
Zend Framework member

@quocvu Mock the storage, and test that the deleteSubscription() method is called once with the appropriate data; this way, no DB is necessary.

@quocvu

@weierophinney I modified the existing test to reflect my changes but ran into these 2 problems:

1) w/ the current setup tests in ZendTest/Feed/PubSubHubbub/SubscriberHttpTest.php are executed since they are dynamic

2) while trying test the delete of record, I realized mock up storage object aren't deleted. How can this simulated in PHP Unit?

You can look at my repo. It has the changes checked in

@weierophinney weierophinney added a commit that referenced this pull request
@weierophinney weierophinney [#3295] Fixed tests and logic flow
- Fixed the unit tests for the unsubscribe functionality. The table
  gateway was expecting the "delete" method now (not "update"), with
  only the "id" member in the array argument; additionally, we needed to
  test the return value, so I added a "will" clause.
- Altered the workflow of Callback to use a switch() statement for the
  "hub_mode"; this will make adding additional cases in the future
  easier, and allows us to raise an exception on an invalid mode.
ccb9df6
@weierophinney weierophinney added a commit that referenced this pull request
@weierophinney weierophinney Merge branch 'hotfix/3295' into develop
Forward port #3295
5669636
@weierophinney weierophinney added a commit that closed this pull request
@weierophinney weierophinney Merge branch 'hotfix/3295'
Close #3295
e6d08c8
@weierophinney
Zend Framework member

I identified the issue with the mocking, and fixed it on merge. Thanks for the patch!

@ghost Unknown pushed a commit that referenced this pull request
@weierophinney weierophinney [#3295] Fixed tests and logic flow
- Fixed the unit tests for the unsubscribe functionality. The table
  gateway was expecting the "delete" method now (not "update"), with
  only the "id" member in the array argument; additionally, we needed to
  test the return value, so I added a "will" clause.
- Altered the workflow of Callback to use a switch() statement for the
  "hub_mode"; this will make adding additional cases in the future
  easier, and allows us to raise an exception on an invalid mode.
6478614
@ghost Unknown pushed a commit that referenced this pull request
@weierophinney weierophinney Merge branch 'hotfix/3295'
Close #3295
bd03272
@ghost Unknown pushed a commit that referenced this pull request
@weierophinney weierophinney Merge branch 'hotfix/3295' into develop
Forward port #3295
2fc547d
@weierophinney weierophinney added a commit to zendframework/zend-feed that referenced this pull request
@weierophinney weierophinney [zendframework/zf2#3295] Fixed tests and logic flow
- Fixed the unit tests for the unsubscribe functionality. The table
  gateway was expecting the "delete" method now (not "update"), with
  only the "id" member in the array argument; additionally, we needed to
  test the return value, so I added a "will" clause.
- Altered the workflow of Callback to use a switch() statement for the
  "hub_mode"; this will make adding additional cases in the future
  easier, and allows us to raise an exception on an invalid mode.
a7ef12f
@weierophinney weierophinney added a commit to zendframework/zend-feed that referenced this pull request
@weierophinney weierophinney Merge branch 'hotfix/3295' 6a72de0
@weierophinney weierophinney added a commit to zendframework/zend-feed that referenced this pull request
@weierophinney weierophinney Merge branch 'hotfix/3295' into develop 8abd6e8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 27, 2012
  1. @quocvu
Commits on Jan 9, 2013
  1. @quocvu
This page is out of date. Refresh to see the latest.
View
2 library/Zend/Feed/PubSubHubbub/Subscriber.php
@@ -744,7 +744,7 @@ protected function _getRequestParameters($hubUrl, $mode)
'verify_token' => hash('sha256', $params['hub.verify_token']),
'secret' => null,
'expiration_time' => $expires,
- 'subscription_state' => PubSubHubbub::SUBSCRIPTION_NOTVERIFIED,
+ 'subscription_state' => ($mode == 'unsubscribe')? PubSubHubbub::SUBSCRIPTION_TODELETE : PubSubHubbub::SUBSCRIPTION_NOTVERIFIED,
);
$this->getStorage()->setSubscription($data);
View
16 library/Zend/Feed/PubSubHubbub/Subscriber/Callback.php
@@ -93,13 +93,19 @@ public function handle(array $httpGetData = null, $sendResponseNow = false)
* Handle any (un)subscribe confirmation requests
*/
} elseif ($this->isValidHubVerification($httpGetData)) {
- $data = $this->currentSubscriptionData;
$this->getHttpResponse()->setContent($httpGetData['hub_challenge']);
- $data['subscription_state'] = PubSubHubbub\PubSubHubbub::SUBSCRIPTION_VERIFIED;
- if (isset($httpGetData['hub_lease_seconds'])) {
- $data['lease_seconds'] = $httpGetData['hub_lease_seconds'];
+
+ if ($httpGetData['hub_mode'] == 'subscribe') {
+ $data = $this->currentSubscriptionData;
+ $data['subscription_state'] = PubSubHubbub\PubSubHubbub::SUBSCRIPTION_VERIFIED;
+ if (isset($httpGetData['hub_lease_seconds'])) {
+ $data['lease_seconds'] = $httpGetData['hub_lease_seconds'];
+ }
+ $this->getStorage()->setSubscription($data);
+ } else {
+ $verifyTokenKey = $this->_detectVerifyTokenKey($httpGetData);
+ $this->getStorage()->deleteSubscription($verifyTokenKey);
}
- $this->getStorage()->setSubscription($data);
/**
* Hey, C'mon! We tried everything else!
*/
View
3 tests/ZendTest/Feed/PubSubHubbub/Subscriber/CallbackTest.php
@@ -286,7 +286,7 @@ public function testRespondsToValidConfirmationWith200Response()
'verify_token' => hash('sha256', 'cba'),
'created_time' => $t->getTimestamp(),
'lease_seconds' => 1234567,
- 'subscription_state'=> 'verified',
+ 'subscription_state'=> 'to_delete',
'expiration_time' => $t->add(new DateInterval('PT1234567S'))
->format('Y-m-d H:i:s'))),
$this->equalTo(array('id' => 'verifytokenkey'))
@@ -294,6 +294,7 @@ public function testRespondsToValidConfirmationWith200Response()
$this->_callback->handle($this->_get);
$this->assertTrue($this->_callback->getHttpResponse()->getStatusCode() == 200);
+ $this->assertTrue($this->_callback->getStorage()->hasSubscription('verifytokenkey') == false);
}
public function testRespondsToValidConfirmationWithBodyContainingHubChallenge()
View
3 tests/ZendTest/Feed/PubSubHubbub/SubscriberHttpTest.php
@@ -97,6 +97,9 @@ public function testUnsubscriptionRequestSendsExpectedPostData()
.'%3A%2F%2Fwww.example.com%2Ftopic&hub.verify=sync&hub.verify=async'
.'&hub.verify_token=abc',
$this->client->getResponse()->getBody());
+
+ $subscriptionRecord = $this->subscriber->getStorage()->getSubscription();
+ $this->assertEquals($subscriptionRecord['subscription_state'], PubSubHubbub::SUBSCRIPTION_TODELETE);
}
protected function _getCleanMock($className)
Something went wrong with that request. Please try again.