Cleanup Form creation processes

This is largely what I had in mind too :)

Single Newsletter Public Block
This block should be kept simple by default. i.e. more simple than it currently is. I'm not a big fan of options, we should reduce the number of options in the admin interface but keep the variables availabe in the template file. Perhaps use a simple template by default and include an example template with all features enabled.

Multiple Newsletter Public Block
I welcome this block in simplenews 'core'. The admin should have the ability to select the newsletters which will be listed in the block. (Some sites have hundreds of newsletters)

Account Form
What you describe is the current functionality. We should discuss further what to do with email language setting.

Admin Form
The above form gives the admin a way to edit individual accounts. Admin forms should be focused on centralized subscription maintenance and mass maintenance operations. The current admin forms lack a page/form to display/edit all subscriber settings per user. Your description matches this form.

If we agree on this level we can detail each form with wire frames. When (re)writing the code we can start high level too after which each form can be coded separately.

Done the rewriting/separation for the account form and admin form.

I still didn't work on the blocks yet.

Added the multiple signup block (with separate code from other forms). Made sure to check the permissions.

- Form name changed to _public_all.
- 'Help Text' and 'Block Message' turned out to be the same, will use 'Block Message' for all.
- Translation issue: removed the use of the t function from the form editing. Now it's only used when displaying.
- Permission check is now done inside callback that builds the html for the multisignup block.
- Wording in the admin's form is fixed.
- Moved the form related functions to the inc file.

I have reviewed the patch and added my comments in the file. File is attached.

see # 15, new patch

patch against dev-Version of 2010-07-11

uses patch of #8 (since #11 was not possible to apply with the tool i used)

and corrects the format of the code.

patch Coder-clean
(one false positive with Coder 6.x-2.0 beta1)

patch against dev-Version of 2010-07-11

uses patch of #8

manually integrated the comments of #11

and corrects the format of the code.

I've tested the patch and it seems to work.

Found two issues already present:
#884338: Spool duplication on node resave
#884348: Unsubscribed users not filtered in newsletter subscriptions page

There's one typo in simplenews.module @1081

            'subject' => check_plain($newsletters[$delta]),
should be
            'subject' => check_plain($newsletters[$delta]->name),

Does #23 that mean that the current dev version is broken or has #24 fixed this already? If it is supposed to work now I think the status should be better set to "needs review".

Please let's try to ensure this issue is fixed on 6.x first. I have updated to the latest 6.x-2.x-dev from today which was critically broken due to the changes in the simplenews_subscriptions_account_form() function. Please bear in mind that you are altering Drupal core's complete user account profile page and not just subscriptions! Thus $form['#redirect'] should be FALSE, as core would behave. Now any changes from profile and subscription fields at user/[UID]/edit will simply be ignored!!!

Here I explain the behaviour in detail with corresponding 4 attached screenshots:

(1) I am on my user edit page (user/1/edit) *without* having the critical simplenews_subscriptions_account_form() function being active. I have deactivated that function by simply adding "return;" to the function's first line. So simplenews is not interfering with Drupal's default behaviour. All the fields within the "Personal information" fieldset are coming from core's profile.module. To reproduce, so please also activate that module, define some fields and assign some values.

(2) After changing some profile.module fields and clicking the [ Save ] button, I see the "The changes have been saved." message on top of the (same) user edit page. The changes have in fact be done. All fine. This is to demonstrate how it should work.

(3) This is the same screen as (1), but now with the original "simplenews.subscription.inc" file, meaning that the critical simplenews_subscriptions_account_form() function is now fully active. You can see the active function by the fact that a "Current newsletter subscriptions" fieldset has been added. But there is NO "newsletter tab" - core's profile edit page has been altered. Then I changed only one field - the profile.module's Country field, from Austria (AT) to Bahamas. I completely leaved the subscriptions settings untouched.

(4) After clicking the [ Save ] button, I am no longer on the same edit page (as it would be without the module), but instead I have been redirected to the User view page. Bad. And, I no longer see the message "The changes have been saved." (as it would be without the module), but "Your newsletter subscriptions have been updated." instead. Worse. I didn't change anything with my newsletter subscriptions, so why am I bothered with this confusing message? But the worst thing is that my Country field change has been ignored - the country still remains to Austria (AT).

Do you still need more info to be aware of this situation?

I am still getting the same error.

It is being triggered by the user_profile_form_validate() function in user.pages.inc. (function below for your reference)

It does not pass the check array_intersect(array_keys($form_state['values']), array('uid', 'init', 'session') because $form_state['values'] contains the key uid.

It doesn't seem necessary to have the uid key, because the user information is already loaded into the [_account] key. Do you have any idea why or how the uid key would find its way into $form_state['values']?

Here's the output of $form_state during the first #validate function.

Array
(
    [storage] => 
    [submitted] => 1
    [values] => Array
        (
            [newsletters] => Array
                (
                    [1523] => 1523
                )

            [uid] => 10
            [_category] => newsletter
            [_account] => stdClass Object
                (
                    [uid] => 10
                    [name] => mac
                    [pass] => /*removed for posting on drupal.org*/
                    [mail] => /*removed for posting on drupal.org*/
                    [mode] => 0
                    [sort] => 0

...

function user_profile_form_validate($form, &$form_state) {
  user_module_invoke('validate', $form_state['values'], $form_state['values']['_account'], $form_state['values']['_category']);
  // Validate input to ensure that non-privileged users can't alter protected data.
  if ((!user_access('administer users') && array_intersect(array_keys($form_state['values']), array('uid', 'init', 'session'))) || (!user_access('administer permissions') && isset($form_state['values']['roles']))) {
    watchdog('security', 'Detected malicious attempt to alter protected user fields.', array(), WATCHDOG_WARNING);
    // set this to a value type field
    form_set_error('category', t('Detected malicious attempt to alter protected user fields.'));
  }
}

I think I figured it out.

In the function simplenews_subscriptions_account_form() in simplenews.subscription.inc

There is an unecessary line that trips the security alert. (around line 74)

$form['subscriptions']['uid'] = array('#type' => 'value', '#value' => $account->uid);

It just needs to be commented out.

Then, a few lines later, in the submit function simplenews_subscriptions_account_form_submit(),
The line that depended on ['uid'] needs to get the uid from the ['_account'] variable instead.

So, delete this line:
$account = user_load(array('uid' => $form_state['values']['uid']));

and replace it with:
$account = user_load(array('uid' => $form_state['values']['_account']->uid));

and it works. (For me anyway.)

I am concerned about the usage of $edit['uid'] in the insert function of simplenews.module, but I don't know enough about the module to make any informed recommendations. I just hope this snippet will save others some grief.

Patch attached.

Hey miro_dietiker,

Actually I was going to build a patch and found the CVS version is out of sync with the latest -dev. It is fixed in the latest CVS.

I also took a look at HEAD and it appeared to be an old copy from February. Is that for D7 or something?

I forgot to say that the problem described in #30 only appears when the "One page profile" module is enabled. Now, with the latest 6.x-2.x-dev (2010-Sep-17), the redirecting problem has been solved, but the other two problems still remain (regarding "The changes have been saved." message and not saving any user profile changes). The workaround for the time being is just to disable the "One page profile" module, so "My newsletters" appears as an own tab under user/[UID]/edit. Should I file a separate issue for this?

Hm, maybe you could discuss how your module should best work together with One page profile with its maintainer - aidanlis. The release notes of One page profile 6.x-1.10 mentions "Provides better integration with other modules", so I guess it should be possible.

Thanks for considering One page profile module support.

subscribe

Apparently, there is an issue between Simplenews and One Page Profile since some time already : #615262: Empty fields with simplenews module. Anyway, maybe it's time to fill a new issue and to close this one, so it's clearer for everybody involved what it's left to do ? @miro_dietiker, what do you think ?

Update of priority.

Hmm I stepped into the same problem. I have got Simplenews 6.x-2.0-alpha1 along with the one page profile Module installed and the problems are as described by roball in comment #37: No changes are saved on the edit profile page and the misleading message "The changes have been saved.". Any further Ideas how to get both modules working together?

Tagging. (Hope this isn't incorrect.)

Attaching a first patch that seems to work when testing manually and also passes all tests locally, checking with testbot. A manual test from someone interested in this feature would also be great, I will however commit pretty soon I think.

Note that I started off with the 6.x-2.x version of simplenews.subscriptions.inc and updated that for 6.x-2.x, except some unrelated parts like the confirmation forms. So this should contain all the follow-up patches in this issue.

Also, it is possible that this patch fixes double confirmation for authenticated users, I think I have seen an issue about that and when looking at the code it is rather obvious that it didn't work. Basically, function simplenews_subscriptions_page_form_submit() created a faked account object with only the mail property and then later on check if $account->uid is set. Which never is...

Tests for the multi-sign up block would be nice and also for the bugs you find.

Basically, if possible, we should try to create a test case for each found bug. Because then we can be sure that this bug will not occur again.