Fixed #19740 -- Ensured that denied feeds are unsubscribed. #60

Merged
merged 1 commit into from Oct 14, 2013

Projects

None yet

3 participants

Owner

No description provided.

@timgraham timgraham commented on an outdated diff Oct 14, 2013
aggregator/models.py
def delete(self, **kwargs):
super(Feed, self).delete(**kwargs)
- if settings.SUPERFEEDR_CREDS != None:
- Subscription.objects.unsubscribe(self.feed_url, settings.PUSH_HUB)
+ if settings.SUPERFEEDR_CREDS is not None:
+ self.unsubscribe()
+
+ def unsubscribe(self):
+ Subscription.objects.unsubscribe(self.feed_url, settings.PUSH_HUB)
timgraham
timgraham Oct 14, 2013 Owner

is a try/except needed here? for example, what will happen if we call unsubscribe on a feed we're not subscribed to?

@brutasse brutasse and 1 other commented on an outdated diff Oct 14, 2013
aggregator/models.py
def delete(self, **kwargs):
super(Feed, self).delete(**kwargs)
if settings.SUPERFEEDR_CREDS is not None:
- Subscription.objects.get(topic=self.feed_url).unsubscribe()
+ self.unsubscribe()
+
+ def unsubscribe(self):
+ Subscription.objects.get(topic=self.feed_url).unsubscribe()
brutasse
brutasse Oct 14, 2013 Contributor

This should be wrapped in a try… except Subscription.DoesNotExist block: transitioning from an approved -> denied state willl unsubscribe correctly but for pending -> denied transitions the related subscription doesn't exist.

In any case this situation is also taken care of in the new update_subscriptions management command which runs daily on the server: https://github.com/django/djangoproject.com/blob/master/aggregator/management/commands/update_subscriptions.py#L32-L35

timgraham
timgraham Oct 14, 2013 Owner

done, thanks for the review!

@aaugustin aaugustin merged commit f4ce13e into master Oct 14, 2013
@timgraham timgraham deleted the 19740a branch Oct 14, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment