View Issue Details

IDProjectCategoryView StatusLast Update
0016707phplist applicationMessage Managementpublic22-05-16 15:16
Reporterduncanc 
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Product Version2.11.7 
Target Versionnext minorFixed in Version3.2.5 
Summary0016707: Difficult to use admin attributes as placeholders
DescriptionIt is currently very difficult to use admin attributes as placeholders in a message. It doesn't appear to be documented but seems to work this way:
include ##LISTOWNER=n in the body of the message to identify the admin whose attributes are to be used. n is the id of the admin.
use the syntax [LISTOWNER.attribute] as the placeholder in the body of the message

I don't know the background to why it is like this, it might have something to do with prepared messages, but it doesn't seem very intuitive. I suggest that by default the attributes of the message owner should be used.

In file admin/sendemaillib.php lines 1422 - 1427 are
  if (preg_match("/##LISTOWNER=(.*)/",$cached[$messageid]['content'],$regs)) {
    $cached[$messageid]['listowner'] = $regs[1];
    $cached[$messageid]['content'] = str_replace($regs[0],"",$cached[$messageid]['content']);
  } else {
    $cached[$messageid]['listowner'] = 0;
  }

The regular expression is incorrect as it includes everything up to the end of the line. The default FCK editor adds some html to each line making it difficult to enter exactly the right text. You need to go to html source mode to enter exactly, say, ##LISTOWNER=2 with no surrounding html.
As the intention is to extract the admin user id, the regexp should try to match a sequence of digits.
Also the match can be made case insensitive, so the line can be changed to:
  if (preg_match("/##LISTOWNER=(\d+)/i",$cached[$messageid]['content'],$regs)) {

To make the message owner's attributes be used as default the else can be change to
    $cached[$messageid]['listowner'] = $message["owner"];

Consequently lines 378-383 can be changed. They are currently
  if (!empty($cached[$messageid]['listowner'])) {
    $att_req = Sql_Query("select name,value from {$GLOBALS["tables"]["adminattribute"]},{$GLOBALS["tables"]["admin_attribute"]} where {$GLOBALS["tables"]["adminattribute"]}.id = {$GLOBALS["tables"]["admin_attribute"]}.adminattributeid and {$GLOBALS["tables"]["admin_attribute"]}.adminid = ".$cached[$messageid]['listowner']);
    while ($att = Sql_Fetch_Array($att_req)) {
      $htmlmessage = preg_replace("#\[LISTOWNER.".strtoupper(preg_quote($att["name"]))."\]#i",$att["value"],$htmlmessage);
    }
  }
As $cached[$messageid]['listowner'] will now always be set then the outer if statement can be removed.
Again the match can be made case insensitive, so line 381 can be changed to
      $htmlmessage = preg_replace("#\[LISTOWNER.".preg_quote($att["name"])."]#i",$att["value"],$htmlmessage);

Two similar lines in file admin/lib.php would then need to be changed in line:
line 505
      $template = preg_replace("#\[LISTOWNER.".strtoupper(preg_quote($att["name"]))."\]#",$att["value"],$template);
line 536
    $template = preg_replace("#\[LISTOWNER.".strtoupper(preg_quote($att["name"]))."\]#",$att["value"],$template);


The same code is used in the 2.10.x version but had been restructured for 2.11.7.
Steps To ReproduceCreate an admin attribute called say name and populate the value for the first administrator.
Include that as a placeholder in a message

##LISTOWNER=1
[LISTOWNER.NAME]

I get a mysql error on the syntax of the query and the placeholder is not replaced.
TagsConfiguration and sending

Relationships

related to 0018121 new LISTOWNER attribute when listowner is "admin" not filled untill list ownership re-saved 

Activities

michiel

07-11-12 11:45

manager   ~0051868


hmm, yes, that can do with a review. It's a bit of a leftover of the "prepare message" era, which is now completely obsolete.

But being able to have admin details as placeholders is still fairly useful and sensible.

michiel

14-11-12 16:33

manager   ~0051902

It may be better to completely avoid the ##LISTOWNER=1 code and simply remember the "listowner" from the database.

The "##LISTOWNER=1" was added in the "Prepare a message" system, which is disfunctional. It was never meant to be added manually by the person who is sending the campaign.

The main issue is how to handle this. It used to be such that say a superuser could send a campaign to a list owned by "Subadmin A" and the details of "Subadmin A" would be included in the mails. I guess that would work if the campaign is sent to one list only. As soon as you send it to more than one lists, there's the chance that list A is owner by Subadmin A and list B by Subadmin B, in which case you'd have to reparse the message when sending. As the message is cached, that's not great for performance.

I think we can do it like this:

IF and only IF a campaign is sent to one list, we look up the "owner" of the list (which is set on list level) and allow [ADMIN.NAME] type placeholders.

What do you think?

duncanc

16-11-12 19:20

developer   ~0051909

That would rule out what I think is a common scenario. An admin sending a message to more than one list each of which he owns.

I had a quick look at the code in actions/processqueue.php for 2.11.7 and couldn't see a point at which the processing knows all the lists to which the message is being sent. The query at line 720 selects users who belong to any list to which the message is being sent, so there would need to be extra processing there to get the list ids from the retrieved user ids.

Alternatively, if the query is changed to return user ids in list order then there wouldn't be much overhead in processing the placeholders for each message but only when there is a change of list id.
However, as I don't know the code that might be more difficult than I imagine.

duncanc

16-11-12 22:35

developer   ~0051910

"processing the placeholders for each message " should be
"processing the placeholders for each user"

michiel

26-03-13 13:13

manager   ~0051984


removing from 2.11.8 target, as it's not stabilisation, but more feature enhancement.

michiel

30-05-13 01:56

manager   ~0052056

ok, so the spec becomes, "if the campaign is sent to lists owned all by the same admin, we can add admin placeholders that will be parsed"

I recently moved the parsing of admin attributes to the pre-caching stage, which means it's no problem for performance.

will mark it for the next dev, and hope to find some time for it.

duncanc

17-01-16 22:31

developer   ~0057460

See GitHub pull request https://github.com/phpList/phplist3/pull/42

michiel

21-01-16 18:00

manager   ~0057472

merged. Nice update, would be good to document this.

gingerling

21-01-16 18:09

manager   ~0057473

anyone have any preference over where the docs go? Is it simple enough for main manual attributes chapter? or more for recources.phplist.com?

duncanc

08-02-16 13:23

developer   ~0057494

I hadn't realised that my change would replace placeholders only in the message, and not in the template.
There is a new pull request to replace in the combined template/message, and add placeholders for the message owning admin - https://github.com/phpList/phplist3/pull/45

michiel

22-02-16 21:34

manager   ~0057533

PR merged

gingerling

28-04-16 14:42

manager   ~0057683

Hi

I have made an admin, made a list and set it owned by them, made an admin attribute for FIRSTNAME and given the admin an entry for that attribute.

So far though [ADMIN.FIRSTNAME] and [LISTOWNER.FIRSTNAME] just print as is in mails.

Can either of you spare 5 mins to talk me through this feature? I would like to blog it up as documentation and talk about it in the release notes.