Simplenews integrates with the node form very well. It reuses the submit button to add further actions like test send or definitive send.
When working with (enforced) revisions this combination is pretty weird:
Even if you only like to send a test newsletter a new revision is added.

At least here we understand that sending newsletters has nothing to do with editing them.

We should add a new Tab "Newsletter" and put all datat/actions into this tab which is not directly node data.

Open for your inputs now!

CommentFileSizeAuthor
#38 simplenews_781708_38_nodeapi_insert.patch1.32 KBsimon georges
#32 simplenews_781708_32_nodeapi.patch1.49 KBDrupOn
#29 simplenews_781708_29_tab.patch29.05 KBsimon georges
#23 simplenews_781708_23_tab.patch28.57 KBsimon georges
#19 simplenews_781708_19_tab.patch27.77 KBsimon georges
#15 simplenews_781708_15_tab.patch27.84 KBsimon georges
#6 simplenews_781708_6_tab.patch29.37 KBsimon georges

Comments

Thomas_Zahreddin’s picture

Thomas_Zahreddin commented

#1

I totally agree!

Let's make the user interface nice and clean.

My customers always moan if the UI is cluttered - even when they are used to the options they sometimes want more options.

So ’hiding’ in a tab settings a user does not need to modify is a very good approach.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Title: Move simplenews node settings into Tab » Move simplenews node actions into Tab

Renaming to more representative title.

sutharsan’s picture

sutharsan commented

In D7 development I made some changes that will help. I decided to move the newsletter email setting out of the newsletter node. In D7 these settings can only be set per newsletter category. According to my questionnaire receipt request and email priority are rarely used and I strongly doubt if html/plain is set on a per node basis. As a result we only have the Send and Send test options left in the node add/edit form.

Having the Send and Send test buttons/actions in the node add/edit form currently has some negative side effects: #758636: Preview button adds newsletter to spool, #795256: Mail Format settings gets lost when clicking preview first and this issue (#781708). Therefore I like to move these actions out of the node add/edit form. We have a few options: Use 'Send' and 'Send test' links below the node; Use a 'Send' tab; Use the Newsletter administration page to send (test) newsletters. Using tabs opens the interface for newsletter preview (#539742: Better Newsletter Preview). Using a page to send newsletters also provides a place to show a progress indicator when we decide to use the batch API to send newsletters (#742652: Use Batch API to send mails).

simon georges’s picture

simon georges commented

Just so I am clear about what's to do :

  • hook_menu() to add the tab
  • access callback of the tab
  • callback of the tab
  • cut / paste of the hook_form_alter() part of the node screen to a new form
  • cut / paste of the hook_nodeapi() part to a form_submit() function

Am I missing something obvious ?
I've actually been beginning the work, but it's a lot of refactoring regarding the node part (from one hand, it could be really good, by moving some part of simplenews logic outside of the node form, but on the other hand, half of the code would be moved, so I'm a litte bit worried of the consequences ;)). Anyway, if you think I'm not forgetting anything, I'll try to provide a patch as soon as possible, so people can test it deeply .

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

There's some more work needed to adapt rules / scheduler / service integration to trigger the send/test process.

I think i would like this as a start, especially for later progress bar implementation.

BTW: While sending we should output a warning if a user tries to edit a newsletter issue. We even could deny save in validate, but i think that's irritating.

Once we have a separate tab, we should even allow to cancel a send process (i think this should lead to a unrecoverable "cancelled" state... where only cloning helps to resend).

simon georges’s picture

simon georges commented
StatusFileSize
new simplenews_781708_6_tab.patch29.37 KB

Ok... first patch.

It seems to be working, although I have not tested multilingual setup yet. The patch is huge, so I just wanted to have your feedback about it.

I removed the hook_clone_alter, as it doesn't seem to be need anymore (the node part works quite as a standard node, the simplenews part is now isolated).

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Active » Needs review

Sounds great.
I'll review it later. Thanks!

simon georges’s picture

simon georges commented

Just so you know, I just tested it with a multilingual newsletter, and it works, too.
It seems I just have a little theming problem (the English newsletter went well, but the french version had not the right theming, it seems, I hope you can help with that ;)).

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

That's cool news!

As always ... everything relies on my part. ;-)

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Needs work

The submission form now makes only sense to send / test send. There's absolutely no need to hit save without it.

So i'd remove the "don't send now" option

Since we'll use this tab after send also for further information, we might use a different Title.

Note the preview currently doesn't apply the node body filter.
I even wouldn't output the node content in this tab. Instead - like preview - i would much more add a confirmation page with a preview.

Can you please have a look at webform.module?
It has a very similar situation and i'd prefer to follow its UI implementation. They e.g. still stay with the node title on the Webform tab.

What do you think?

simon georges’s picture

simon georges commented

I agree with removing the "don't send now" option, its current only purpose is to update the receipt / priority values, which is a pretty minor functionnality ;).

Regarding your point for the UI, I will try webform (just not tonight ;)). I'll keep you posted, thanks for the fast review !

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Please don't forget to update the hook_help :-)
I just did an update on the help text...

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

What do you think about the tnid (translated node) case?
I even thought about showing the send form of the tnid to send the full translation set instead of a dummy message.
We might even output a summary about the amount of current translations or the full translation set in the form.

Regular users don't have an understanding of a translation source. They're just two translations of the same newsletter.

simon georges’s picture

simon georges commented

I totally agree with you, it makes no sense for a regular user. I'll display the form whatever translation of the node we're in. I think we can figure out what else to display later, maybe even after the release, what do you think ?

simon georges’s picture

simon georges commented
Status: Needs work » Needs review
StatusFileSize
new simplenews_781708_15_tab.patch27.84 KB

New patch (done on the current 6.x-2.x-dev version), with :
- node preview removed
- "don't send now" option removed
- same behavior for every language

Apparent issue :
- when using on multilingual setup, the two mails don't have the same output (the one on the current language is ok, the one on another language has visible html tags (so multilingual theming issue, apparently)
- the test e-mail is the one in the default language (but that's not introduced by my patch, as before, only the default language could send mails)

Anybody who's willing to test it is welcome ! Please report back on this issue the issues you found within this patch.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Needs work

This is a visual review only. Not runned any tests.

FYI: If you use a test address with defined preference of the Language B, the user will receive the test language of Language B.
That's what the current ML behaviour as i'd have expected.

In the simplenews_admin_settings_newsl.. you're using numeric keys and not the constants.

I feel the new API for simplenews_update_newsletters is strange
- Please rename to simplenews_newsletter_update( ...
- pass parameter with a cleaner API.

All nodes have vids - not only translations. For the translation case you should check for $node->tnid (which is 0 if no translation set..).

Finally we need to decide if we log those information either per newsletter nid, or per revision (nid, vid). If vid is the primary key, then we should globally care about it.

Hard work! :-)

simon georges’s picture

simon georges commented

Ok, I've checked, and, indeed, when sending test to an existing subscription, the mail is sent using the language associated to the subscription, so that's still ok ;)

"In the simplenews_admin_settings_newsl.. you're using numeric keys and not the constants." : I've just adjusted the values consequently to my changes, but the function wasn't using the constants before. I could change that, I don't know if I should do that on the same patch, or if we keep the "cleaning" on a specific issue (we could to the drupal_add_css() call on only the page thats needs it, not on every page, things like that).

When tnid = 0, translation_node_get_translations() doesn't return anything, so the node is sent instead. I load the vid on translation only, because in case of a node, it's already loaded by node_load() (translation_node_get_translations() doesn't load the vid).

I agree with your other remarks, new patch coming... when I have the time, probably tomorrow night.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Right..
I see that it works correctly with the tnid. But if tnid==0, we shouldn't even call the API (get translation).
Note that translation_node_get_translations not even provides a return value... Let's just try to be clean.

Great progress.

simon georges’s picture

simon georges commented
Status: Needs work » Needs review
StatusFileSize
new simplenews_781708_19_tab.patch27.77 KB

The revision thing still needs to be figured out (where do we keep tab of revisions ?). Otherwise, I've added some of your remarks to the patch.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

:-) Looks better now.

Some further adjustments and we can roll it out once.
Wenn will you find time to check this issue again?

EDIT:
Please remove in simplenews_tab_node the drupal_set_title().
I'd change simplenews_tab_page to simplenews_node_tab_page.
And then tab_form to something like node_tab_send_form because of its send purpose..

Beside that i found no ugly side effect currently.

simon georges’s picture

simon georges commented

Today & tomorrow, I really want to see that committed before taking off for the christmas holidays ;)

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Needs work

Please, please!

;-)

(BTW: see my edit above)

simon georges’s picture

simon georges commented
Status: Needs work » Needs review
StatusFileSize
new simplenews_781708_23_tab.patch28.57 KB

New patch, integrating your last remarks (#20) as well as use of constants in simplenews.admin.inc.
Do you have an opinion about what to do for revision management (maybe on another patch, this one begins to be big enough ;)) ?

simon georges’s picture

simon georges commented

There actually is #910970: revision handling broken.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Needs work

Looks fine to me.

Tested some trivial sends (no multilanguage) and it worked.

Note that nid becomes vid if revisioning is enabled. This has nothing to do with translation.
If translation happens, a node with nid has a tnid now pointing to the nid containing the translation.
So at least the text here is wrong: simplenews_newsletter_update().
I think we should remove the node_load() here...

So - some work remaining. ;-)

For a quick commit you could remove all vid related things in node and say: revision handling broken (again).. ;-)

simon georges’s picture

simon georges commented

I agree we could remove everything related to vid, but that means changing a lot of the APIs, as vid are used everywhere in simplenews.mail.inc. So I think it would be a big whole other patch.

Regarding the the move into the tab, shoud I do something more ?

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

I'm not talking about complete change of vid handling... (which should happen in separate commit)
I'm talking about this in #25:

+function simplenews_newsletter_update($node, $param = array()) {
+  // If this is a translation, we have to load the vid
+  if (!isset($node->vid)) {
+    $node = node_load($node->nid);
+  }
...

The vid lines are completely wrong - at least their description. But i think we should remove them completely in this patch!

I'm fine with committing this patch as soon as descriptions are right and functionality seems OK for the cases previously worked.
(Revision handling excluded.)

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

BTW: Do we still need hook_content_extra_fields()?
I think this is only needed to place the extra fields on the node form using the weights management of CCK UI.

simon georges’s picture

simon georges commented
Status: Needs work » Needs review
StatusFileSize
new simplenews_781708_29_tab.patch29.05 KB

Good catch, Miro, I had not seen that one.

So, to sum up the current patch :
- remove the js
- move everything simplenews-related from hook_from_alter() & hook_nodeapi() to a complete different tab, with form, validate & submit callback
- remove hook_clone_node_alter() (as it's a simple node clone now)
- remove hook_content_extra_fields() (not needed any more)
- remove the "Don't send now" option, that does not make sense any more
- use constants in simplenews.admin.inc

Miro, I moved the lines regarding the node_load() to the submit callback, with a comment about data consistency, you were right (as always ;)), those lines were not really clear.

Let me know if you spot anything else.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Simon... ALWAYS! ;-D

You might won't believe it but that all sounds perfect and i really like what i see as a result!
(Note i did NOT test this with applying the patch)

Thank you very much for this hard work. Please commit and let's see what will happen... ;-)

Let's wait 2..3 days before going Beta. I'll take the opportunity to test the result in production.

simon georges’s picture

simon georges commented
Status: Reviewed & tested by the community » Fixed

Committed... YAY !

DrupOn’s picture

DrupOn commented
Status: Fixed » Needs review
StatusFileSize
new simplenews_781708_32_nodeapi.patch1.49 KB

Pity this introduced another issue:
When i add a new newsletter the nodeapi op=insert is now missing.
Thus the record in simplenews_newsletters now is missing after inserting a new newsletter node.

Result is that the new issue now is missing in admin/content/simplenews

We need to add the entry..

Attached patch readds needed nodeapi lines.

simon georges’s picture

simon georges commented

Oh, you're right...
I see you've kept the "update" operation too. Is there a specific reason ? In case the content type is associated to Simplenews after the node has been created ?

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

This is needed because a newsletter issue might switch the tid.

simon georges’s picture

simon georges commented

Ok.
I'm in favor of using the new simplenews_newsletter_update() function, to keep the code minimal (the existence check wouldn't be needed, as it is performed by the function) and coherent, what do you think ?

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Sure, i'm perfectly fine with that.
Do you find time to provide patch?

simon georges’s picture

simon georges commented
Assigned: Unassigned » simon georges
Status: Needs review » Needs work

Sure, 'will do.

simon georges’s picture

simon georges commented
Status: Needs work » Needs review
StatusFileSize
new simplenews_781708_38_nodeapi_insert.patch1.32 KB

This patch uses the new simplenews_newsletter_update() function.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Looks perfect. I'll commit later.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Version: 6.x-2.x-dev » 7.x-1.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

fixed in dev.
Again needs some D7 port.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Version: 7.x-1.x-dev » 6.x-2.0-alpha1
Priority: Normal » Critical
Status: Patch (to be ported) » Needs work

Simon, we have a critical issue with this...

The tab issue needs clean rework and is a blocker.
Note that currently all node saves lead to updates of the newsletter. Currently this leads to a PENDING again after each save.
Also there are a bunch of side effects because the send command has no neutral and the newsletter save code always expects test send or full send.

Also the tab display only shows a message "has been sent"... (not differing for pending VS sent)

I'm already working on a suggested rework of this part.

Meanwhile users please note that the sending process and the node update workflow is broken!

simon georges’s picture

simon georges commented

Gosh, I knew from the start this wasn't gonna be easy...

Ok, so the first thing I see to make it better is only use "insert" hook_nodeapi() operation.
That way, no more unwanted updates. The update was needed in case of term changing. Maybe we can do a specific test for it, and only update this field ?
(By the way, the possibility to have different terms for translated nodes is strange, and I don't know how the sending process takes that into account).

Anyway, as you started to work on this, what can I do to help ?

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

That's a i18n issue - see i18nsync. We don't handle different terms.
Note the insert case is not enough since one could change the target term. We should not make term readonly on further edits...

Trying to fix it on my own first, u can help most with e.g. release notes.

(off: Do you know translators? We should trigger them.)

roball’s picture

roball commented

So alpha1 is broken. Wouldn't it be good to revert that patch for now and release alpha2 so we have a working alpha version?

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Without this patch other things are broken...
We'll fix this asap, since i won't release a 6.x-2.x stable without this blocker implemented.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Assigned: simon georges » DrupOn

Assigning

DrupOn’s picture

DrupOn commented

Committed clean solution to dev in 7.x.
Backporting to 6.x-2.x.

DrupOn’s picture

DrupOn commented
Status: Needs work » Fixed

Committed new D6 version. D7 is much cleaner already... but this should work stable.

roball’s picture

roball commented
Priority: Critical » Normal

Can confirm that this works now in the latest 6.x-2.x-dev (2011-Jan-19). Thanks!

Status: Fixed » Closed (fixed)

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