robogals / myrobogals

myRobogals is the global intranet and record-keeping tool for Robogals. It has been built to simplify many of our day-to-day tasks including organising school visits, maintaining a member database, communicating with members, storing records reliably for future generations and easily collecting statistics on a global scale.
https://my.robogals.org
10 stars 21 forks source link

Investigate proper filtering of emails #20

Closed U-238 closed 11 years ago

U-238 commented 12 years ago

Feedback from a user:

"Well the mysterious thing is that even though we select filter by 'student', emails are still being sent to 'alumni' members. I had an email from a 2011 member just today asking to be taken off the email list, but I had even unticked the box on his profile to not receive email updates from us... so I have no idea how he is still receiving emails."

U-238 commented 12 years ago

Canberra chapter had this problem

yfcheung commented 12 years ago

Finding Suspects:

find ../ -name '*.py' -exec grep -n 'EmailMessage(' '{}' \; -print (Nothing wrong)

send email to all users, and filter by student (Nothing wrong)

UserList: all active user can be added

def invitetovisit: (no status check) staff and userlist does not check email option

Some user subscribed to the amplifier, as well as having email newsletter option on. (Most likely) def api: does not deactivate newsletter subscribe when unsubscribe

Some user subscribed to little engineers digest and science challenge newsletter and have email newsletter option off (Most likely)


Suspicious account history

some staffs (Robogals UQ) have the following status history: student -> alumni -> student

some non-staff (Robogals ANU and Robogals UQ) have the following status history and is on some user list: student -> alumni -> student

U-238 commented 12 years ago

I'll find out from the user whether she was sending emails through "send email" or "invite to visit". I'll also ask what option she was using whether it was all users or a user list.

Regarding the newsletter API, I thought it did actually uncheck the "subscribe to amplifier" option if a myRobogals user's email address is given. I've just tried it myself and found that it doesn't uncheck that option in their myRobogals account if the same email address is found in NewsletterSubscribers. So it's good that we found that, and we should fix it, but I don't think that's the original problem experienced by the user who reported this, because only Global has access to send those newsletters.

yfcheung commented 12 years ago

def writeemail:

144 users = User.objects.filter(is_active=True, email_newsletter_optin=True).exclude(email='')

150 if statussel != '0' and request.POST['type'] != '4':

168 if request.POST['type'] == '4' and request.user.is_superuser:

consider the case: user A with email_newsletter_optin=True, and NwsletterSubscriber.objects.filter(newsletter=1, email=A.email) is not empty

The A receive amplifier newsletter 1 + NwsletterSubscriber.objects.filter(newsletter=1, email=A.email).count() times.

Is that a feature or a consequence not derived by the original author, or both?

And I am having trouble of finding all the entry points to subscribe newsletter!

U-238 commented 12 years ago

To fix in newsletter:

U-238 commented 12 years ago

Import CSV should also check if the user is subscribed through email_newsletter_optin

email_newsletter_optin is ONLY for the Amplifier (newsletter id 1)

yfcheung commented 12 years ago

def writeemail: 144 users = User.objects.filter(is_active=True, email_newsletter_optin=True).exclude(email='') # users selected for Amplifier

def api: 476 u = User.objects.get(email=p.email) # check if user exists if does, set email_newsletter_optin True

Is following situation significant:

user A is not active but subscribe to amplifier through api and api set user A email_newsletter_optin True. Since user A is not active it won't receive email newsletter due to "def writeemail:144"

I can not find where are the attributes "user.alt_email" and "user.new_email" used

U-238 commented 12 years ago

Regarding the newsletter stuff, I've pushed this to the live server and tested it through the subscribe form on the website - it works as intended!

Still need to find out what the problem is with the original bug report though :(

yfcheung commented 12 years ago

email option:

1: All members of Robogals ANU who have opted to receive email 2: Executive committee members of Robogals ANU

1 checks user attribute: email_chapter_optin therefore 1 is not the offending option. 2 does not check email_chapter_optin so it could be it.

likely theory: Some staff's status maybe changed to student accidentally

If that is not the case, then I think I may need the necessary information to reproduce the offending case.

U-238 commented 11 years ago

Ok I will ask the person who reported it again for steps to reproduce

U-238 commented 11 years ago

From: Bridgette Greenfield bridgette@robogals.org.au To: Mark Parncutt mark@robogals.org Date: 17 September 2012 14:35 Subject: Re: myRobogals change member settings in bulk

myrobogals -> Email & SMS -> Send email -> To: all members of Robogals ANU who have opted to receive emails; Filter by: student.

Alumni are all there as expected and not in Student fields.

However, I think this problem has gone away, or may have been a one-off error, or maybe the filter was set to 'all'. The last email I sent by this method, I asked 3 alumni members to tell me if they received the email, but none of them had received the email or heard much from us for a while.

Thanks

yfcheung commented 11 years ago

suggestion:

Should I produce a report for the relevant emails and to confirm if their sending options are indeed the intention of the senders? The list isn't long, but it does mean auditing responsibilities on the part of the senders.

U-238 commented 11 years ago

If I understand you correctly, does that mean there is a second page as part of "send email" where it shows who will receive the email, for the sender to approve? I think that would be a good solution.

yfcheung commented 11 years ago

It would seem perfectly obvious that it is a good solution, and the flaws of the implementation I am thinking of can not be exploit by any one except for insiders (Hackers).

No need to implement safe guard for ensuring step ordering since, if some one works hard enough to sent a POST request to go direct to the second page, then he must be sure about what he wants.

All write actions will only be done at the end of second page, so the danger period for race condition is not a value depends on human reaction time.

yfcheung commented 11 years ago

Should two-page policy be applied to "Send SMS" as well?

U-238 commented 11 years ago

Yes