Administrators may choose anonymous language preset for bulk subscriptions.
However, these values don't get updated later anymore and they're nor exposed to understand for an admin what happens nor are they editable.

In general, subscription data for anonymous users simply get too implicitly updated (or not) and the whole API concentrates on subscribing a mail to a list.

We should introduce an API to update the (list independent) mail properties and start outputting language data and more edit properties such as activation.

This patch contains some notes of things to check, introduces a simple api for language, calls it to enforce language switches on mass import and provides admins language information and switch capability. I really hope we can fix all the language related stuff very soon.

CommentFileSizeAuthor
#14 simplenews_579202_14_language.patch7.02 KBmiro_dietiker
#12 simplenews_579202_12_lang.patch8.94 KBmiro_dietiker
#8 simplenews_579202-2.patch31.25 KBmiro_dietiker
#7 simplenews.579202-1.patch6.53 KBsutharsan
#4 simplenews.579202.patch6.48 KBsutharsan
simplenews-language-expose.patch8.55 KBmiro_dietiker

Comments

sutharsan’s picture

sutharsan commented
Assigned: Unassigned » sutharsan
Status: Active » Needs review

Nice move :) I'll look into it.

dawehner’s picture

dawehner
Primary language German
commented 
+++ simplenews/simplenews.admin.inc	16 Sep 2009 15:27:19 -0000
@@ -446,6 +446,8 @@ function simplenews_subscription_list_ad
+      // update user data

I think its better to write:

Update user data.

+++ simplenews/simplenews.admin.inc	16 Sep 2009 15:27:19 -0000
@@ -606,6 +608,8 @@ function simplenews_subscription_list_re
+      // update user data

same.

+++ simplenews/simplenews.module	16 Sep 2009 15:27:19 -0000
@@ -701,6 +701,7 @@ function simplenews_user($op, &$edit, &$
+	  // remove anonymous mail subscription for users' new address

same.

+++ simplenews/simplenews.module	16 Sep 2009 15:27:19 -0000
@@ -713,6 +714,7 @@ function simplenews_user($op, &$edit, &$
+      // TODO visualize activated status in ui

same.

+++ simplenews/simplenews.module	16 Sep 2009 15:27:19 -0000
@@ -1028,6 +1032,22 @@ function simplenews_subscribe_user($mail
+ * Update user preferences

same. :)
Ok its enough.

+++ simplenews/simplenews.module	16 Sep 2009 15:27:19 -0000
@@ -1028,6 +1032,22 @@ function simplenews_subscribe_user($mail
+    if(!$result->uid) {

if ( Codestyle :p

+++ simplenews/simplenews.module	16 Sep 2009 15:27:19 -0000
@@ -1249,6 +1264,55 @@ function _simplenews_subscription_manage
+      $access = $subscription->uid ? FALSE : TRUE;

why not use !$subscription->uid

+++ simplenews/simplenews.module	16 Sep 2009 15:27:19 -0000
@@ -1249,6 +1264,55 @@ function _simplenews_subscription_manage
+        );

indentation.

I'm on crack. Are you, too?

More review will follow..

mrath’s picture

mrath commented

Hello,
I have tested the patch on a clean Drupal installation, with the required modules, and can say he perfectly functioned.

sutharsan’s picture

sutharsan commented
Category: bug » feature
StatusFileSize
new simplenews.579202.patch6.48 KB

* Why call simplenews_update_user() in simplenews_subscription_list_remove_submit()?
* Add Language to subscription admin page. Agreed.
* simplenews_update_user() Only update anonymous?
* Newsletter subscription must be as easy as possible. Therefore we should not place language selections in the subscription form. Users are subscribed with the current (not the default!) page language. Maintaining the language at the account maintenance pages is a right place.

A new patch is attached, like to have your comments.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Thanks for this cool update.
Looks promising.

At least there's one thing wrong while reading it:

+function simplenews_subscriber_update($snid, $data) {
+  $language = isset($data['language']) ? $snid['language'] : '';

$snid is the ID, not an array...

Updating the subscriber information always is much more flexible and proof of missing certain future updates..
And for the unsubscription there really seems no update required.

We're going to test this asap in deep detail.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Needs work

needs work due to snid bug (missed transition)

sutharsan’s picture

sutharsan commented
Status: Needs work » Needs review
StatusFileSize
new simplenews.579202-1.patch6.53 KB

I fixed the snid and added a tiny bit of documentation.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
StatusFileSize
new simplenews_579202-2.patch31.25 KB

Sutharsan
I've taken your patch and checked many things and added some more - some new and some from previous patches.
(Sorry, my dev environment also included some formatting corrections :-S Also i added some additional code comments!)

You've deleted my language exposure in _manager but added it to user settings. I think this is wrong logic:
What we wanted is administrators to be able to edit language settings (of anonymous users) while users shouldn't be able to choose them: They choose language settings in their account and simplenews should rely on that locale setting.
However looging at simplenews_subscribe_user in the present situation it turns out that no language setting gets updated ever.
New code displays language selection as radios - corresponding to locale.module

Since _manager is being used for hook_user and administration forms we added a $context to detect the form we render.

One issue is we can't detect correctly that current user has "default" language but much more need to reduce it to concrete languages. Also locale.module does right this. (Also user_load and subscribers should do this).
Since we keep both in sync this is perfectly OK. To finalize this we might need some update script to set language to default site language if currently empty. Introducing "default" value would lead to a very complex situation.

_simplenews_subscription_manager_form still has code in it for block form rendering but is unused now. Right?

Do you now agree some more? ;-)

sutharsan’s picture

sutharsan commented

Please make clean patches. Don't mix code style and other cosmetic changes with funtional changes. It slows down the patch reading and acceptance. Clean code style patches are no-brainers, mixed patches are a nightmare.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Needs work

As you've pointed out, context is not what you want in _manager.
We'll redo the patch once we've split the form codes as latest state was in:
#683724: Hidden newsletters are visible

Sorry, the codestyle part was really accidential. Going to submit another issue for that for the obvious issues.

So let's concentrate on this big step forward to later on make cleaner and simpler solutions (for all other current open related patches/issues).

sutharsan’s picture

sutharsan commented

Ok, for now we will drop the ability to modify the preferred newsletter language by the user and by the admin. Lets first get the underlying functions working properly. I agree that simplenews should follow the authenticated user's language selection. Admin import of email addresses should not override this.
Agree to set an explicit preferred language in the simplenews_subscription table and keep it in sync.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Version: 7.x-1.x-dev » 6.x-2.x-dev
Status: Needs work » Fixed
StatusFileSize
new simplenews_579202_12_lang.patch8.94 KB

I was sure i brought this in under 6.x-2.x...
I worked on the first part to add multilingual elements on lists. No saves yet due to form overhaul.
Committed attached patch as a step #1.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Fixed » Needs work

needs more work after:
#805114: Cleanup Form creation processes

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs work » Fixed
StatusFileSize
new simplenews_579202_14_language.patch7.02 KB

Now after form cleanup i've added settings in admin edit to change preferred language of subscriptions.
Patch also contains tiny other loading updates.

Status: Fixed » Closed (fixed)

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