If the user does not have permissions to customize the notification level, allow him to opt out of email notifications if they are not disabled by default.

This is not working due the value of the checkbox pm_email_notify_user is set 1 but it should be the value of the global pm email notification level.

CommentFileSizeAuthor
#4 privatemsg-pm_email_notify-notify-level-1329996-3.patch683 bytesosopolar
#1 privatemsg-pm_email_notify-notify-level-1329996-1.patch665 bytesosopolar
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

osopolar’s picture

osopolar
Primary language German
Location 🇩🇪 GER 🌐
CreditAttribution: osopolar commented
FileSize
privatemsg-pm_email_notify-notify-level-1329996-1.patch665 bytes

FIX pm_email_notify_user(): Add #return_value to field $form['privatemsg']['pm_email_notify_level'].

osopolar’s picture

osopolar
Primary language German
Location 🇩🇪 GER 🌐
CreditAttribution: osopolar commented
Status: Active » Needs review
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

It needs to be PM_EMAIL_NOTIFY_LEVEL_DEFAULT, not $is_enabled. This is necessary so that users use the new default if that is changed.

But yes, the #return_value is necessary, not sure why my tests didn't catch this..

osopolar’s picture

osopolar
Primary language German
Location 🇩🇪 GER 🌐
CreditAttribution: osopolar commented
FileSize
privatemsg-pm_email_notify-notify-level-1329996-3.patch683 bytes

OK, this means that -1 will be stored in Database to force the default value will be used, right? Due a lack of time I didn't dive to much in details.

Wondering about: 

'#default_value' => $is_enabled ? PM_EMAIL_NOTIFY_LEVEL_DEFAULT : PM_EMAIL_NOTIFY_LEVEL_DISABLED,
'#return_value' => PM_EMAIL_NOTIFY_LEVEL_DEFAULT,
'#access' => $is_enabled,

If 0 == $is_enabled, than '#access' will be false and the form won't appear, right? But then we don't need that default_value is PM_EMAIL_NOTIFY_LEVEL_DISABLED.

Anyway, I fixed the patch as mentioned in #3.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

The form will not appear, correct. However, that means that it will enforce the #default_value because even though the form is not rendered to the user, it will be present in $form_state['values'] when the form is submitted.

aidanlis’s picture

aidanlis CreditAttribution: aidanlis commented

What does this code even do? It makes no sense.

The user is presented the option "Receive email notification for incoming private messages" which is checked by default, but when they uncheck it and redisplay the form it's still checked? The default_value is set to the global default, no attempt is made at retrieving the users setting ... what's the point?

osopolar’s picture

osopolar
Primary language German
Location 🇩🇪 GER 🌐
CreditAttribution: osopolar commented

Maybe the code makes more sense if you read #5 and fix the mentioned problem. I don't have time to check, but I guess it worked as expected otherwise I wouldn't have written and used this patch.

aidanlis’s picture

aidanlis CreditAttribution: aidanlis commented

Your patch changes the return value which then has no effect because #default_value is either $is_enabled (a global setting) or a constant.

aidanlis’s picture

aidanlis CreditAttribution: aidanlis commented

Your patch changes the return value which then has no effect because #default_value is either $is_enabled (a global setting) or a constant.

osopolar’s picture

osopolar
Primary language German
Location 🇩🇪 GER 🌐
CreditAttribution: osopolar commented

I can't remember. From reading #4 I guess I also had some doubts but maybe for me it was enough.

Form API Reference says: #return_value Used by: checkbox ... Value element should return when selected.
... default value is 1. 'PM_EMAIL_NOTIFY_LEVEL_DEFAULT' is defined as -1 ... this means instead of -1 will be returned 1, what is never correct.

'PM_EMAIL_NOTIFY_LEVEL_DISABLED' is defined as 0.

Setting #access to 0 (false) means that the form isn't shown, the default_value will be 0 (not selected) ... this will be also the result value. So I guess it should work: In case it is checked, the value will be -1 in case it is not checked the value will be 0. The user can Opt Out (not Opt In).

@aidanlis: Did you apply the patch and tested it? Maybe something else has changed since the patch was created.

aidanlis’s picture

aidanlis CreditAttribution: aidanlis commented

I'm not sure what you're trying to say, but I suspect it isn't relevant. The #default_value is set to either the global site wide value ($is_enabled), or the constant PM_EMAIL_NOTIFY_LEVEL_DEFAULT, neither reflect the users setting.

Yes I've tried the patch - it doesn't change the behaviour I describe above.

ptmkenny’s picture

ptmkenny CreditAttribution: ptmkenny commented
Status: Needs review » Needs work
ptmkenny’s picture

ptmkenny CreditAttribution: ptmkenny commented

Marked https://drupal.org/node/1294808 as a duplicate.

oadaeh’s picture

oadaeh CreditAttribution: oadaeh as a volunteer commented
Issue summary: View changes
Status: Needs work » Closed (won't fix)

This issue is being closed because it is against a branch for a version of Drupal that is no longer supported.
If you feel that this issue is still valid, feel free to re-open and update it (and any possible patch) to work with the 7.x-1.x branch (bug fixes only) or the 7.x-2.x branch.
Thank you.