Breaking code when subscribing twice #5

Closed
rubenstolk opened this Issue Mar 26, 2011 · 11 comments

Projects

None yet

3 participants

@rubenstolk

if subscription.subscribed: line 60 in forms.py should probably just be removed.

@dokterbob
Owner

Hey Ruben,

Thanks for your suggestion! It is a bit hard for me to know like this just why this line should be removed.

Could you please be a bit more verbose in your bug description?

A general guideline for filing bug reports is something like this:

  • Under what user cases does this bug occur (what where you doing?)
  • What was the expected behaviour
  • What is the erroneous behaviour that occurred instead
@rubenstolk

Sorry man, didn't have time by then. Guess you have fixed it now. By the way, where are you located? We have our office in Leidschendam...

@dokterbob
Owner

Hey Ruben,

Indeed it should be fixed by now. Is there any chance you could help me out by double-checking, or maybe even writing a unittest for it? Would greatly appreciated.

We have your office in the west of Amsterdam. Since you're so nearby maybe it'd be nice to share a coffee or beer sometime. Send me an email if that sounds like a plan. :)

Regards,
M

@rubenstolk

Hey Mathijs,

I am currently settled in Pune - India, setting up an office here so that will be difficult. You can meet with my co-workers though... where do you work?

@jleclanche
Contributor

FYI this is still broken if I subscribe three times.

First two times it registers it in the database, and then the third time:

MultipleObjectsReturned at /newsletter/travelbuddy-newsletter/subscribe/
get() returned more than one Subscription -- it returned 2! Lookup parameters were {'newsletter': <Newsletter: Travelbuddy Newsletter>, 'email_field__exact': u'a@b.com'}

There should be some check done on the email, I don't know where though

@dokterbob
Owner

@Adys I'm considering putting a full day into fixing this (and other) issues sometime soon - and push out a new release.

@dokterbob
Owner

@rubenstolk I am planning a 0.5 release of django-newsletter, and I need your feedback! Since GH has removed private messaging and you don't have an email address set on your profile, I thought I might as well abuse GH's issues to contact you:

Dear contributors to django-newsletter,

Your efforts, pull requests, bugreports and feature-suggestions have not gone unnoticed. Thanks so much for your contributions!

In the next two weeks I am planning to take about a day to:

  • Fix some long-standing issues
  • Implement some much-needed features
  • Merge in some of your contributions!

In order to have a good view on what to implement first, I would very much like some feedback from you guys. Both on currently opened issues (+1/-1) or in the form of new issues (bugs, feature requests etc).

Please take a look at all the currently open issues at:
https://github.com/dokterbob/django-newsletter/issues?state=open

The idea is to have django-newsletter 0.5 finished, tested and documented by the 16th of september. If you want some clue as to what day exactly I will take for this, look to see when there's rainy weather in Amsterdam. ;) It'll highly likely be the 15th or 16th.

Until that time: please feel free to nag, bug me, discuss and work with me on this upcoming release.

Kind regards,
Mathijs

'The long-lost maintainer of django-newsletter.' ;)

PS. Sadly enough, GH removed their 'Private Messages' functionality. This is why I am sending this directly to your email address - if published at all.

@jleclanche
Contributor

@dokterbob What is the status of the 0.5 release? This bug is somewhat trivial to fix and very easily hit, having it fixed upstream would be great.

@jleclanche
Contributor

FWIW, the fix is to change "if subscription.subscribed:" in forms.py to just "if subscription:".

Reasoning: the subscription does not have to be confirmed for it to exist. subscription.subscribed is only true once the sub has been confirmed, but the check needs to be broader than that.

@dokterbob
Owner

I'm currently in the progress of improving testing infrastructure. After this is finished I'll fix all bugs that require no serious kind of refactoring and push out a latest 0.3 compatbility release. Should be some time next week.

If you want to help me fix this bug (which might be a duplicate of #14) it would be great to have a unittest to replicate the issue.

@dokterbob
Owner

Addendum. Wiithout looking much deeper in the code and the actual situation, it seems to be the steps necessary to replicate this issue are as follows:

  1. Register and subscribe
  2. Unsubscribe and confirm unsubscription
  3. Register again FAIL

If this is the case, we should probably check whether a 'subscribed' user has not been 'unsubscribed' as well. In the latter case we should simply not raise an the error. Hence perhaps #74 in forms.py should spell:

if subscription.subscribed and not subscription.unsubscribed:

However, I seem to remember that subscribed and unsubscribed are mutually exclusive. (Note to self: confirm.)

@dokterbob dokterbob added a commit that referenced this issue Nov 4, 2012
@dokterbob Test and fix for #5. 46460f5
@dokterbob dokterbob closed this Nov 4, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment