1. Go to newsletter/subscriptions
2. Leave all newsletters unchecked
3. Enter an email address
4. Submit by clicking either Subscribe or Unsubscribe
Instead of an error message, the following wrong message is displayed:
You were added to the chosen mailing list.
| Comment | File | Size | Author |
|---|---|---|---|
| #23 | multi-subscribe-and-page-subscribe-with-tests.patch | 15.94 KB | berdir |
| #22 | multi-subscribe-with-tests.patch | 7.53 KB | berdir |
| #21 | includes_simplenews.subscription.patch | 2.71 KB | berdir |
| #17 | includes_simplenews.subscription.patch | 2.71 KB | berdir |
| #5 | includes_simplenews.subscription.patch | 2.71 KB | anrikun |
Comments
Comment #1
anrikun commentedActually, it is the form itself that is wrong:
The displayed message is the same :
when submit buttons may be different:
"Update" for authenticated users.
"Subscribe" and "Unsubscribe" for anonymous users.
Comment #2
simon georges commentedI'd eventually change the message to "You were added to the chosen newsletter(s)." to have the same words on every screen, but there is a different message for anonymous (this one) and authenticated ("your subscriptions have been updated").
What messages would you use (both on form header and after form submit) ?
Comment #3
anrikun commentedHere are 3 screenshots to better describe the issue (in French):
Issue #1
anonymous-form.png:
The header message is correct: "Select the newsletter(s) to which you want to subscribe or unsubscribe."
("Select the newsletters you want to subscribe to or unsubscribe from" would be better though but I'm not a native english speaker so I can't really tell...)
But when submitting without checking any newsletter, you get anonymous-form-submitted.png:
Instead of displaying the expected error message "You must select at least one newsletter.", it displays the wrong notice "You were added to the chosen mailing list." (nothing happens in the background, only the notice is wrong)
Issue #2
authenticated-form.png
The header message is wrong: it should be something like "Check the newsletters you want to subscribe to. Uncheck the ones you want to unsubscribe from."
Comment #4
anrikun commentedIssue #3
simplenews.subscription.inc: line 298
Checking !$subscription alone is not enough as simplenews_get_subscription() never returns an empty value.
Comment #5
anrikun commentedIssue #4
The message "You were added to the chosen newsletter(s)." is indeed inappropriate. Why not simply use the same message everywhere?
drupal_set_message(t('The newsletter subscriptions for %mail have been updated.', array('%mail' => $form_state['values']['mail'])));The attached patch seems to fix all the issues described here.
Comment #6
anrikun commentedI'm a bit worried: simplenews 6.x-2.0-alpha2 is out and this patch has been skipped, surely because nobody has reviewed it yet.
Am I the only one affected by the 4 issues above?
Especially, issue #3 is an important bug.
Comment #7
simon georges commentedI've released a new version because the "critical" bugs have been fixed. This patch has not been skipped, it just wasn't in our priorities. It will be taken into account some day.
Comment #8
anrikun commentedThanks for your answer Simon.
I'm just concerned about the fact that nobody else has reported having the same issues :-)
Comment #9
ambient.impactSubscribing. This bug is especially confusing to my client's multilingual site visitors, as it seems to only display the English version, regardless of the current language. Using 6.x-2.0-alpha2. Hope this gets fixed sooner rather than later.
Comment #10
anrikun commented@Ambient.Impact: Subscribing is OK but have you reviewed the patch? Does it work?
Comment #11
anrikun commentedBumping this. Did somebody review the patch?
Comment #12
berdirThis has probably a much higher chance of being applied to 7.x-1.x first (because that's what we are actively developing now) and then backport. Let's see if the patch applies...
Comment #13
sgurlt commentedGonna check this out today and will review it later ;)
Comment #14
anrikun commented@Berdir: I reported this bug for 6.x-2.x, not 7.x-1.x-dev.
I don't even know if the bug exists in 7.x-1.x-dev.
Before changing the version of this issue, we need to check whether the bug is also present in 7.x-1.x-dev or not.
Comment #15
berdirLooks like we're good at crossposting today..
Comment #16
berdir@anrikun
Yes, it needs to be tested and most likely this bug exists in 7.x-1.x too. This feature has been ported to 7.x-1.x recently and has not been changed since then, nor are there any tests for it. So it is very likely that the bug also exists in 7.x.
Active development from our side (MD Systems) is now focused on the 7.x-1.x branch. The patch has been lying around for more than half a year for 6.x-2.x, I will however try to commit this on 7.x-1.x before the end of the year. And once it has, including tests, it will be much more likely to be commited to 6.x-2.x as well.
Comment #17
berdirRe-uploading the patch from #5 to trigger the testbot.
Comment #19
sgurlt commentedI tried the patch from #5 and it worked perfectly for me, thx !
Comment #20
berdirPlease stop changing the version, thanks.
Comment #21
berdirOnly problem was that the patch wasn't using the new -p1 compatible format (e.g. an a/b prefix). Re-rolled.
Comment #22
berdirAnd now with tests which confirm that there was an error that is now fixed. The mail isn't nested inside ['subscriptions'].
Will commit once it has passed the tests.
Comment #23
berdirActually, the ['subscriptions'] behavior is different for the multi subscription block and the newsletter/subscriptions page.
Added tests for the latter as well and checking $user instead of the field now.
Comment #24
berdirOk, commited, back to 6.x-2.x with this...
Comment #25
berdir#23: multi-subscribe-and-page-subscribe-with-tests.patch queued for re-testing.
Comment #26
berdirComment #27
anrikun commentedWill this be committed to 6.x-2.x soon?
simplenews 6.x-2.0-alpha3 is out and it is not in it :-(
Comment #28
simon georges commented@anrikun, sorry about the delay, I've no client using the module, so I haven't reviewed every case you patched...
Comment #29
berdirNote that the patch most likely needs to make similar changes as I did in the 7.x-1.x, to work with the "new" multip-signup block. That's why the status is set to "patch (to be ported)".
Comment #30
anrikun commentedWho should port the patch then?
Comment #31
berdirYou could :)
I'm only working on the 7.x-1.x branch, do not expect any work from my side on the 6.x-2.x branch
Comment #32
truyenle commentedeither #5 or #22 does work for me. Thanks