Apparently it's done this way because when a node is updated it checks if the email field is empty or not to decide if it needs to send emails.

function protected_node_node_update($node) {
...
if ($node->protected_node_is_protected && !empty($node->protected_node_emails)) {
  module_load_include('mail.inc', 'protected_node');
  protected_node_send_mail($node);
}
..

This doesn't look like an expected user experience. When I edit a protected node I want to know to which email it has/will been sent. But in fact I need this feature for another reason. I've created a "please enter you email" form, and when a user enter his email a see a list of all protected nodes for which he has received a password. This is only possible if emails are stored somewhere.

So the proposed solution is to save the emails in the protected_nodes table. Then when we need to know if emails should be sent we can simply check if the input emails are different than the one saved in the database.

Is there any other reason why emails are currently not saved?

patch following

CommentFileSizeAuthor
#12 save_emails-1572036-12.patch7.05 KBgrimreaper
#2 save_emails-1572036-2.patch8.76 KBidflood
#1 save_emails-1572036-1.patch6.29 KBidflood

Comments

idflood’s picture

idflood commented
Status: Active » Needs review
StatusFileSize
new save_emails-1572036-1.patch6.29 KB

Running update.php is required after applying this patch since it adds a field table in the database. (Don't test this on production and make backups first)

idflood’s picture

idflood commented
StatusFileSize
new save_emails-1572036-2.patch8.76 KB

I've added an 'Optional email notifications' option which when enabled adds a 'Send email' checkbox on protected node forms.

So with this, the workflow is similar to: choose a password and remember it, create a node and add some emails but leave "send email" unchecked. edit/update the node and add emails, when ready edit the node and check the 'send email' option and save. Emails are then only sent only on the last 'save'.

To understand what I've done and the meaning of these patch let me explain a little more my setup. I've also created a 'submodule' which saves the password in clear in the database so that it can be retrieved and admins doesn't have to remember the password of each nodes. This is ok with the goal of my setup because protected nodes doesn't require a rock solid protection. But let me know if you think 'clear passwords' could be an option in this module, I could probably write a patch.

idflood’s picture

idflood commented
Issue summary: View changes

php markup

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Issue summary: View changes
Status: Needs review » Needs work

Hello,

I tested it, it does not remember the email addresses which already received the mail.

And when sending email I have this error :
Notice: Undefined property: stdClass::$protected_node_emails in protected_node_node_update() (line 558 of /my/path/sites/all/modules/protected_node/protected_node.module).

But I wonder if it is a good idea to save the password not hashed in the database. Maybe for your usecase yes, but maybe the other user don't want that.

I don't know what the maintener think of this. I will try to solve the problem if you don't have time.

Thank you.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

Hello,

Ok, I solved the bug of the email addresses not savec and load from the database.

But I don't see clearly the workflow you want or the maintener would want. And I have some other bug in the feature I need to solve before I upload the patch. For example : making the difference between the adresses where a mail has already been sent, etc..

I need a workflow well (and simply) explained.

Thank you

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: Unassigned » grimreaper
grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

I think about a simpler workflow :

  • adding a field in the database to store the email adresses which had already received a password. (already done by the previous patches)
  • instead of the checkbox "send mail" which I don't think it's very clear in the mind. on the edit node form, having two email boxes :
    • One is the "classical" one where all the email adresses will receive a mail when the node will be updated (to have the password in the node, you will need to reenter it)
    • The second one will be here to show the email adresses which already received the mail, so if you want to resend the email, a simple copy/paste from one box to the other and it's ok.

If it's ok with this workflow I will make it.

izus’s picture

izus commented

actually you can change the password meanwhile, and what is interesting is the last list that received the password, so that you can update them with the new one.

so i think it's better to keep it simple : whenever you save, you update the record in the database.
whenever you load the edit form, you query the database to populate the field if the email feature is active

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

Ok, so if I understand :

  • one box ?
  • if you edit the node without changing the password ? It's erase the list of the last person who received the password ?
  • the checkbox "send mail" introduced by the pathes on this issue is different from the checkbox "email feature active". So I use the "send mail" checkbox ?

Do not hesitate to write the full workflow you have in mind, with all the cases described.

izus’s picture

izus commented

i think the condition will be just: if the field is not empty and it contains valid adress mail separated by commas, then we update the record in the database.
else if the field is empty we do nothing
but if the syntax is not good or there are no valid address mail, we throw an error.
i think we can avoid extra checkbox.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

Ok,

so if you prepopulate with the previous email addresses to remember who received the password. Even if you don't change the password, they will receive an email : sort of spam, no?

Ahh... And you avoid this spam emptying the box (by the user who edit the node) so the previous email addresses will not receive email and will not be erased in the database. right ?

Hum... it needs to the user who edit to really care about the email feature. In the worst case, we can add the "send mail" checkbox later ?

If you confirm I understood your workflow, I will make it.

izus’s picture

izus commented

yes i think you understood it well :)

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Status: Needs work » Needs review
StatusFileSize
new save_emails-1572036-12.patch7.05 KB

Hello,

I attached a patch to review/test.

Thanks.

izus’s picture

izus commented
Status: Needs review » Fixed

merged.
Thanks idfloor and Grimreaper !

@Grimreaper FYI, somehow your patch couldn't be applyed with git apply, i found that only what it missed is a blank line at the end !

izus’s picture

izus commented

cross referencing #2142803: Error when trying to update database

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: grimreaper » Unassigned