This is because we currently only rely on #states to show/hide the fieldset.

IMHO, it should not be displayed if the newsletter has already been sent, opinions?

Note: I'm currently creating a bunch of issues as a reminder for myself that I discovered while using the module.

CommentFileSizeAuthor
#10 fix_tab_checks_more_tests3.patch5 KBberdir
#7 fix_tab_checks_more_tests2.patch4.87 KBberdir
#1 fix_tab_checks_more_tests.patch4.73 KBberdir

Comments

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
StatusFileSize
new fix_tab_checks_more_tests.patch4.73 KB

Here is a patch that cleans up the checks (much simpler now), part of if was a left-over from the times when it was on the node edit form.

I also noticed that the sent edition case was completely borked and added tests for that as well.

joachim’s picture

joachim commented

I'm not really clear what this fixes. Do you mean parent or edition nodes? Could you give a quick bullet point list of what I should or shouldn't be seeing with & without this patch?

I've tried looking at both tabs of both a parent and an edition with and without the patch and I can't see anything changing... :/

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

Most importantly, it fixes looking at a regular sent newsletter.

It should also fix the Newsletter tab of a sent newsletter edition and now show that this is an edition. The old code for this is dead because the most outer if already checks for it not being an edition so it is never run and that code was also broken because it tried to use an object as array.

This is possibly something that is broken in 6.x-2.x as well, see:

+++ b/simplenews_scheduler.moduleundefined
@@ -211,13 +207,9 @@ function simplenews_scheduler_form_simplenews_node_tab_send_form_alter(&$form, &
-      $title .= t('This node is part of a scheduled newsletter configuration. View the original newsletter <a href="@parent">here</a>.', array('@parent' => url('node/' . $node->simplenews_scheduler_edition['pid'])));
-      $form['simplenews']['none']['#title'] = array(
-        'type' => 'item',
-        '#title' => $title,

Neither is $title defined here, nor does the $form logic make any sense, it does set the #title property to an array consisting of type (note the missing #) item and another #title.

I don't think this is new code, so my assumption is that it's something that hasn't been touched in the port as it's never actually executed currently.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review

Didn't mean to set it to needs work, bad Dreditor, bad ;)

joachim’s picture

joachim commented
Status: Needs review » Needs work

ah right, now I get it.

So:

- parent newsletter: no change
- edition newsletter: no change
- newsletter node that doesn't participate in scheduling: fixed.

I can see the fix now :)

Did you intentionally set this back to needs work? One point of my own:

- is the check for a regular newsletter node in isset($form['simplenews']['send']) ? could you add a comment above that line explaining the if() expression in a bit more detail please?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Yes, it mis-uses the fact that there is no send form element if the newsletter was already sent. That's ugly however, I'll replace it with a check that actually looks for the correct newsletter state, then the code should be much easier to read even without a comment (which I will add as well if it makes sense)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new fix_tab_checks_more_tests2.patch4.87 KB

Doing that check would actually be much more complicated. So I simply added a comment.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Go for it, testbot.

Status: Needs review » Needs work

The last submitted patch, fix_tab_checks_more_tests2.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new fix_tab_checks_more_tests3.patch5 KB

Re-rolled.

joachim’s picture

joachim commented

Yikes, big patch!

I'd really appreciate a review from another user of the D7 branch :)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

This is the same patch as before, just re-rolled.

joachim’s picture

joachim commented
Status: Needs review » Fixed

Committed, thanks.

- #1461152 by Berdir: Fixed scheduler fieldset displayed when not relevant.

Status: Fixed » Closed (fixed)

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