Make it clear that a newsletter can only be sent once

Add suggestion to perhaps show an inactivate greyed-out button, which says "Sent".

It's a balance - we don't want to clutter the UI for experienced users with stuff they already know. I feel that help text should be separate from essential UI text.

Here's a suggestion. Instead of the changes in the current MR, we make just one change to NodeTabForm in the case that the newsletter has been sent (this is the point at which the "send now" button is 'missing').

There is already some indication of what's going on:

Newsletter issue sent to 4 subscribers, 0 errors.

We can make it more obvious. We could make the '#type' => 'details', wrapper be present always (move it outside the if test). Then in the case where there is no button, we can if you like make the button appear but disabled, and add a sentence "Newsletters can only be sent once.".

Thanks for taking a look at this @adamps, and that's a great and simpler suggestion, since only a single file needs to be updated. I had to unset the fieldset, since a convoluted elseif solution could become too complex, I think ...

A customer of mine just ran into a big problem: They cloned a newsletter with Quick Node Clone → and spent quite some time editing it.

But when they wanted to send it, the "Send now" button was missing, since values, including the "Sent"-status had been cloned.

I think overall, that it would be nice to clearly signal to a user editing an already sent newsletter, that it has indeed already been sent, to prevent them from spending much time in vain. So I am expanding the scope, to include a warning on the Edit page.

Also, editing an already sent newsletter will mostly not make any sense, so adding this message would be nice.

Thanks.

Good idea for the edit page. There is already code in simplenews_form_node_form_alter() for SIMPLENEWS_STATUS_SEND_PENDING. I suggest we expand that to include other status types.

Thanks for the screenshot. I would like to move "Newsletter issue sent to 1 subscribers, 0 errors." to be above the disabled "sent" button? This would match the message prior to sending "Send newsletter issue to 1 subscribers. " Currently I find it hard to notice at the bottom. We could use the same code $form['send']['count'] = , just the actual message differs based on the status. We could even put "Newsletters can only be sent once." into $form['send']['method']. I like the idea that the form becomes much more consistent across the different states.

However there is a question of back-compatibility in case anyone customised the form. Perhaps if we make a new minor release and document it?

Great feedback and tips. I moved the warning above the button like you suggested, and that does work better.

I also added the message on the edit form, where you suggested, that'll be helpful, and prevent pointless editing. At first, it was inside the if (!$node->isNew()) { section, but then it wouldn't be shown if you use Quick Clone, like I did ... Placing it on its own like now, it will be shown both when editing or cloning a sent newsletter.

This idea sounds a bit like my initial MR?:

We could even put "Newsletters can only be sent once." into $form['send']['method']. I like the idea that the form becomes much more consistent across the different states.

Creating a new minor release and documenting it sounds like a plan :)

Great I like it. Yes your initial MR had some good ideas.

Please can we take one more step to simplify the code (and it also solves a bug)? I made some detailed comments in the MR.

Thanks for the review. It's not clear to me though, what exactly needs to happen ... Do you know about the Insert suggestion button?

I think it would be easier if you used that one for each suggested change. Then I can simply click "Apply suggestion" and your suggested code change gets implemented. I think that's easier for this refactoring, since I am not sure what needs to be done ...

Good idea. I didn't see the suggest changes button, so I've just posted my code here. Please let me know what you think.

    // Show newsletter sending options.
    $form['send'] = [
      '#type' => 'details',
      '#open' => TRUE,
      '#title' => $this->t('Send'),
    ];

    // Add some text to describe the send situation.
    $form['send']['count'] = [
      '#type' => 'item',
      '#markup' => $summary['description'],
    ];

    if (($status == SIMPLENEWS_STATUS_SEND_NOT) || ($status == SIMPLENEWS_STATUS_SEND_READY)) {
      if ($status == SIMPLENEWS_STATUS_SEND_READY) {
        $send_text = $this->t('Newsletters can only be sent once.');
      }
      elseif (!$node->isPublished()) {
        $send_text = $this->t('Mails will be sent when the issue is published.');
        $button_text = $this->t('Send on publish');
      }
      elseif (!$config->get('mail.use_cron')) {
        $send_text = $this->t('Mails will be sent immediately.');
      }
      else {
        $send_text = $this->t('Mails will be sent when cron runs.');
      }

      $form['send']['method'] = [
        '#type' => 'item',
        '#markup' => $send_text,
      ];

      $form['send']['send'] = [
        '#type' => 'submit',
        '#button_type' => 'primary',
        '#disabled' => $status == SIMPLENEWS_STATUS_SEND_READY,
        '#value' => $button_text ?? $this->t('Send now'),
      ];
    }
    else {
      $form['send']['stop'] = [
        '#type' => 'submit',
        '#submit' => ['::submitStop'],
        '#value' => $this->t('Stop sending'),
      ];
    }

    return $form;

Thanks! Though, it's not a button, sorry.

Here is a link on how to create suggestions in GitLab. I believe everyone would benefit from knowing about it, since it can make the development process easier. And Drupal will be using GitLab going forward after all:

https://docs.gitlab.com/user/project/merge_requests/reviews/suggestions/...

If you can't be bothered that's all right, then I'll do it the other way. Just let me know.

Thanks for explaining I agree that is useful for me to know. I had a look and I feel that the suggestions work better for smaller changes.

Yes, you're probably right. I updated the MR, and the new code works well: I get the "Newsletters can only be sent once." message after sending a newsletter, and a greyed out button, so it looks good to me.

Great thanks, the code looks good.

It now needs a few simple test fixes for the changed wording. Plus it would be great to have one new test to check the new message appears. There should likely be an existing test where we could just add a single line.

Sounds great, and I have updated the failing tests, and found a place where I could add a test for the new message.

Great thanks, I like the improved form and helpful text.

Nice, thanks for maintaining Simplenews, which I really appreciate -- it works really well.

PS: I just noticed a detail: This sentence is missing a dot at the end: "This newsletter issue has already been sent"
simplenews.module, line 300.