Opened 10 years ago

Closed 9 years ago

#286 closed bug (worksforme)

Adminmassmail is broken - repaired now but need a review

Reported by: jeanyves Owned by: jeanyves
Priority: major Milestone: 0.1.4
Component: BW General Keywords: massmail
Cc: steinwinde, tobias, matrixpoint, lemonhead

Description

Adminmassmail is broken, its menus are not anymore displayed and the submit create SQL errors in test (it can be a simple test problem)

This will become a blocker the day BW will need to send a mass mail to its member

Change History (9)

comment:1 Changed 10 years ago by jeanyves

I forget to Add : the broken adminmassmail is caused by the layout change in the server done three weeks ago. The menus where in the left column. I will move them out of there not to be dependant anymore with this old thing

comment:2 Changed 10 years ago by jeanyves

  • Cc steinwinde tobias matrixpoint lemonhead added
  • Component changed from unknown to BW General
  • follow_up changed from none to review code
  • Summary changed from Adminmassmail is broken to Adminmassmail is broken - repaired now but need a review
  • version changed from all to test

I have repaired the adminmassmail in a way that it is now usable with PT changes

I have also activated a way to create specific query to allow specific range of massmail targets. This is restricted to Admin right, but I think this need some thinking.

The current adminmassmail (the one running on Live since summer) allows to :
1) create a word sentence for title of the broadcast message and subject of this broadcast
2) choose the criteria to define the targetted members
3) display the list of the targetted members (limited to avoid a too long page)
4) enqueue the broadcast
5) trigger the sending (mailbot doing the translation in appropriated languages of the receiver according to his prefered language if a translation is available).

The change I made :
A) allow it to work because now the option are visible again in the PT environment
B) allow the Admin to alter the query which will select the member to broadcast

The reason for B is that it was in fact allready waiting since a while in my local env, but never committed. And that we need ways to define "one shot" select of target range according to various need. In case this manual alterated query are overused, it mean an extra option more safe is to add to adminmassmail.
The way The B was done allow for SQL injection (but needs admin rights to be used). So I think some code review make sense here.

It is only committed on test they are two things to consider :
Q1 : Does the feature can work ?
Q2 : can we dare to put online such a thing (under admin rights control) ? Or (if it is a Yes for Q1) will it be to put online when needed and remove after being used ? (this to avoid the risk of admin acount beeing stolen for example)

Nota for now : it is not anymore possible to use the current Live adminmassmail

comment:3 Changed 10 years ago by micha

  • Milestone changed from 0.1.1-outreach-bugfixing to 0.1.2 - more improvements & bugfixing

comment:4 Changed 10 years ago by philipp

Q2: So far we used mass mail about 2 or three times. I would strongly opt to put it online only when needed. We should not focus on producing a safer system as it is rarely needed right now and other things are more important.

comment:5 Changed 10 years ago by jeanyves

@Philipp : Why don't you think it is not safe ? (I mean ok, it is to be use carefully, but is not the protection by massmail right a sufficient protection for now ?)

IMHO we should at least send a monthly global mass mail for news to our members (but it is communication team)

and in addition, some specific massmail (like one to remind not active members that BW works) is urgently needed (again this is a communication team job).

comment:6 Changed 10 years ago by philipp

No, I don't really trust in protection by rights or restriction to BW username. Anyhow my point is that we should not have something like the possibility for sql injections around (intentionally) if not even use the feature. It might be a tradeoff if we heavily use it. However for the to named most urgent purposes "mail to all members" "mail to inactive" we could put it online without the possibility to alter the query.

comment:7 Changed 10 years ago by jeanyves

ok philipp

comment:8 Changed 10 years ago by jeanyves

  • Priority changed from critical to major

I move adminmassmails with the needed fix (initialized variable, volunteer bar on main page) to production because we want to send a new letter in the following days
This version include the "open query".

The idea is that it will be removed after the sending, but it is needed for few days for test of the sent result.

Nota : moving/removing it any time is a small pain, could someone look a bit at it (yes, it allow someone with Admin right to enter an additional "where" criteria, so basically it can be dangerous for the database), I however personally think that this is under control.

This is old BW code, before someone jump on moving it to PT, please just keep in mind that this prehistoric interface works and is not to be seen by members ;-)

comment:9 Changed 9 years ago by philipp

  • freq_reported set to 1
  • Resolution set to worksforme
  • show_on_bw set to 0
  • Status changed from new to closed

guess we won't work on this for a while and it is fine as it is right now, closing this

Note: See TracTickets for help on using tickets.