Problem/Motivation

Similar to #2410265: Replace subscriber overview with views

We want to have overviews as views.

Proposed resolution

Convert to views.
Enable tests and improve them.

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#82 simplenews-2413995-replace-newsletter-issue-81.patch56.31 KBAnushka-mp
#82 simplenews-2413995-replace-newsletter-issue-81.patch56.31 KBAnushka-mp
#82 simplenews-2413995-replace-newsletter-issue-81.patch56.31 KBAnushka-mp
#81 simplenews-2413995-replace-newsletter-issue-81.patch56.31 KBAnushka-mp
#81 simplenews-2413995-replace-newsletter-issue-81-interdiff.txt1.48 KBAnushka-mp
#70 simplenews-2413995-replace-newsletter-issue-70.patch56.06 KBAnushka-mp
#70 simplenews-2413995-replace-newsletter-issue-70-interdiff.txt8.99 KBAnushka-mp
#67 simplenews-2413995-replace-newsletter-issue-67.patch53.06 KBAnushka-mp
#67 simplenews-2413995-replace-newsletter-issue-67-interdiff.patch3.85 KBAnushka-mp
#59 simplenews-2413995-debug-added-interdiff.txt1.3 KBAnushka-mp
#59 simplenews-2413995-replace-newsletter-issue-59-DEBUG.patch53.04 KBAnushka-mp
#59 simplenews-2413995-replace-newsletter-issue-59-DEBUG.patch53.04 KBAnushka-mp
#59 simplenews-2413995-replace-newsletter-issue-59-DEBUG.patch53.04 KBAnushka-mp
#59 simplenews-2413995-replace-newsletter-issue-59-DEBUG.patch53.04 KBAnushka-mp
#59 simplenews-2413995-replace-newsletter-issue-59-DEBUG.patch53.04 KBAnushka-mp
#57 simplenews-2413995-replace-newsletter-issue-56.patch52.97 KBAnushka-mp
#57 simplenews-2413995-replace-newsletter-issue-56-interdiff.txt634 bytesAnushka-mp
#54 simplenews-2413995-replace-newsletter-issue-54.patch52.88 KBAnushka-mp
#54 simplenews-2413995-replace-newsletter-issue-54-interdiff.txt4.1 KBAnushka-mp
#48 simplenews-2413995-replace-newsletter-issue-48.patch52.57 KBAnushka-mp
#48 simplenews-2413995-replace-newsletter-issue-48.patch52.57 KBAnushka-mp
#48 simplenews-2413995-replace-newsletter-issue-48.patch52.57 KBAnushka-mp
#48 simplenews-2413995-replace-newsletter-issue-48.patch52.57 KBAnushka-mp
#48 simplenews-2413995-replace-newsletter-issue-48.patch52.57 KBAnushka-mp
#30 simplenews-2413995-replace-newsletter-issue-23.patch52.26 KBAnushka-mp
#30 simplenews-2413995-replace-newsletter-issue-23.patch52.26 KBAnushka-mp
#30 simplenews-2413995-replace-newsletter-issue-23.patch52.26 KBAnushka-mp
#30 simplenews-2413995-replace-newsletter-issue-23.patch52.26 KBAnushka-mp
#30 simplenews-2413995-replace-newsletter-issue-23.patch52.26 KBAnushka-mp
#25 simplenews-2413995-replace-newsletter-issue-23.patch52.26 KBAnushka-mp
#25 simplenews-2413995-replace-newsletter-issue-23-interdiff.txt855 bytesAnushka-mp
#22 simplenews-2413995-replace-newsletter-issue-22.patch52.29 KBAnushka-mp
#22 simplenews-2413995-replace-newsletter-issue-22-interdiff.txt879 bytesAnushka-mp
#17 simplenews-2413995-replace-newsletter-issue-17.patch51.65 KBAnushka-mp
#17 simplenews-2413995-replace-newsletter-issue-17-interdiff.txt3.84 KBAnushka-mp
#16 simplenews-2413995-replace-newsletter-issue-16.patch52.94 KBAnushka-mp
#16 simplenews-2413995-replace-newsletter-issue-16-interdiff.txt5.13 KBAnushka-mp
#14 simplenews-2413995-replace-newsletter-issue-14.patch51.22 KBAnushka-mp
#14 simplenews-2413995-replace-newsletter-issue-14-TESTS-ONLY.patch3.73 KBAnushka-mp
#12 simplenews-2413995-replace-newsletter-issue-12.patch47.11 KBAnushka-mp
#12 simplenews-2413995-replace-newsletter-issue-12-interdiff.txt14.48 KBAnushka-mp
#12 Screen Shot 2015-02-02 at 12.18.12.png81.87 KBAnushka-mp
#8 Screen Shot 2015-01-30 at 17.20.56.png93.2 KBAnushka-mp
#7 simplenews-2413995-replace-newsletter-issue-4.patch42.21 KBAnushka-mp
#7 simplenews-2413995-replace-newsletter-issue-4-interdiff.txt5.6 KBAnushka-mp
#4 simplenews-2413995-replace-newsletter-issue-2.patch41.7 KBAnushka-mp
#2 simplenews-2413995-replace-newsletter-issue-1.patch37.25 KBAnushka-mp
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

miro_dietiker’s picture

Issue tags: +8.x release target
Anushka-mp’s picture

Status: Active » Needs review
FileSize
37.25 KB

Newsletter issues view created.
Newsletter issue send action added.
NewsletterIssueList form and custom code for newsletter send dropped.
two custom plugins added for send status and subscriber count.
Newsletter filter added (working on top of #2410265: Replace subscriber overview with views).
Bulk operations added for send, publish and unpublish newsletter Issues.
Routes configured.
Submitting this for a review.

Status: Needs review » Needs work

The last submitted patch, 2: simplenews-2413995-replace-newsletter-issue-1.patch, failed testing.

Anushka-mp’s picture

Berdir’s picture

Status: Needs work » Needs review
miro_dietiker’s picture

Some quick feedback.

  1. +++ b/src/Plugin/views/field/SendStatus.php
    @@ -0,0 +1,45 @@
    +        '#theme' => 'simplenews_status',
    

    I don't see this implemented, is it?

  2. +++ b/src/Plugin/views/field/SendStatus.php
    @@ -0,0 +1,45 @@
    +        '#sent_count' => $node->simplenews_issue->sent_count
    

    I guess u want to pass in $subscriber_count instead.

  3. +++ b/src/Plugin/views/field/SendStatus.php
    @@ -0,0 +1,45 @@
    +      return $status_render_array;
    +      $send_status = $node->simplenews_issue->status == SIMPLENEWS_STATUS_SEND_PENDING ? $subscriber_count - $pending_count : drupal_render($status_render_array);
    +      return $send_status;
    

    2 Garbage lines. :-)

Anushka-mp’s picture

Subscriber count dropped. Send Status field will have the subscriber count.
fancy images added to the send status :)
node_send_action => simplenews_send_action

Anushka-mp’s picture

Screenshot of the view.

miro_dietiker’s picture

Status: Needs review » Needs work

1) The admin content overview lists change date labelled "UPDATED" instead of authored date labelled "AUTHORED ON".

2) "PUBLISHED STATUS" could be renamed to "PUBLISHED" only.

3) Instead of "Newsletter is not yet sent", there should only be a space (invisible icon) instead of the icon.

4) Operation delete should be added similarly to the regular admin content list.

2+3 will improve column width rendering.

I'm missing the author a little bit and i'm unsure about all the column sequence.

Berdir’s picture

1. Authored on is created. I'm not sure what we use right now/in 7.x, agreed that using updates is better.

3. Yes, that's what was defined. Except the invisible icon part, didn' think that far ;)

author is only useful if you have multiple authors I think, not sure if that should be there by default, as it is a view now, maybe we recommend to add if that applies to your site? Also not useful to scheduled/automated newsletters for example.

miro_dietiker’s picture

I see that order is also by creation, not by update date.

Agree that we can skip author. And sure, people should edit it, thus be encouraged to personalise it. I'm just looking for a feasible default.

Anushka-mp’s picture

Changes made as mentioned in #9. not sure what to do regarding the 'empty' image.
In addition, 'stop newsletter send' action added as a bulk operation.
simplenews_status theme dropped and created a new method getMessage() to get render elements array.
Working on tests now.

Anushka-mp’s picture

Status: Needs work » Needs review
Anushka-mp’s picture

Test created to test the newsletter issue overview.
text added to no results found.
uploading the test only patch and full path here.
one more fix is yet to be done, which is, the 'empty image' :)

Anushka-mp’s picture

As discussed with berdir,
the image removed if the newsletter is not sent.
and the tests changed accordingly.

Anushka-mp’s picture

some tests were commented out in the earlier patch.

The last submitted patch, 16: simplenews-2413995-replace-newsletter-issue-16.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 17: simplenews-2413995-replace-newsletter-issue-17.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 17: simplenews-2413995-replace-newsletter-issue-17.patch, failed testing.

Anushka-mp’s picture

Hmm.. seems like the Destination URL changed to navigate back to the subscriber view from edit-dubscriber page.
Leading slash added to the destination URL in the tests.
Let's see how the test bot responses, all green locally.

Anushka-mp’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 22: simplenews-2413995-replace-newsletter-issue-22.patch, failed testing.

Anushka-mp’s picture

Conflicting part from the URL comparison string removed.

Status: Needs review » Needs work

The last submitted patch, 25: simplenews-2413995-replace-newsletter-issue-23.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 25: simplenews-2413995-replace-newsletter-issue-23.patch, failed testing.

Status: Needs work » Needs review
Anushka-mp’s picture

The last submitted patch, 30: simplenews-2413995-replace-newsletter-issue-23.patch, failed testing.

The last submitted patch, 25: simplenews-2413995-replace-newsletter-issue-23.patch, failed testing.

The last submitted patch, 30: simplenews-2413995-replace-newsletter-issue-23.patch, failed testing.

The last submitted patch, 30: simplenews-2413995-replace-newsletter-issue-23.patch, failed testing.

The last submitted patch, 30: simplenews-2413995-replace-newsletter-issue-23.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 30: simplenews-2413995-replace-newsletter-issue-23.patch, failed testing.

Status: Needs work » Needs review

The last submitted patch, 30: simplenews-2413995-replace-newsletter-issue-23.patch, failed testing.

The last submitted patch, 25: simplenews-2413995-replace-newsletter-issue-23.patch, failed testing.

The last submitted patch, 30: simplenews-2413995-replace-newsletter-issue-23.patch, failed testing.

The last submitted patch, 30: simplenews-2413995-replace-newsletter-issue-23.patch, failed testing.

The last submitted patch, 30: simplenews-2413995-replace-newsletter-issue-23.patch, failed testing.

The last submitted patch, 30: simplenews-2413995-replace-newsletter-issue-23.patch, failed testing.

The last submitted patch, 48: simplenews-2413995-replace-newsletter-issue-48.patch, failed testing.

The last submitted patch, 48: simplenews-2413995-replace-newsletter-issue-48.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 48: simplenews-2413995-replace-newsletter-issue-48.patch, failed testing.

Berdir’s picture

So I think the problem here is that the order in the view is not as you expect it.

If multiple nodes are created in the same second, then a sort on the date does not work as expected.

You could maybe solve it by adding another sort on the ID, so that when the timestamp is the same, we order on which node was created first.

But the better fix is probably to make your assertions more dynamic.

Instead of hardcoding the row number, you need to loop over all rows, match the newsletter based on the node title table cell and then assert according.

if ($title_row == $pending_node_title) {
// assert the pending stuff.
elseif ($title_row == $sent_node_title) {
....

Anushka-mp’s picture

hm, bad testbot (should make it slower a bit) :-)
Tests modified to dynamically check the newsletter view.

Anushka-mp’s picture

Status: Needs work » Needs review
Berdir’s picture

+++ b/src/Tests/SimplenewsAdministrationTest.php
@@ -746,13 +746,17 @@ class SimplenewsAdministrationTest extends SimplenewsTestBase {
-
+    foreach ($rows as $row) {
+      if ($row->td[1]->a == 'Test_issue_2') {

You should also add an assertEqual(2, count($rows)) then, to make sure that it doesn't pass if the view does not list all expected results.

Anushka-mp’s picture

Status: Needs review » Needs work

The last submitted patch, 57: simplenews-2413995-replace-newsletter-issue-56.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

The last submitted patch, 59: simplenews-2413995-replace-newsletter-issue-59-DEBUG.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 59: simplenews-2413995-replace-newsletter-issue-59-DEBUG.patch, failed testing.

The last submitted patch, 59: simplenews-2413995-replace-newsletter-issue-59-DEBUG.patch, failed testing.

The last submitted patch, 59: simplenews-2413995-replace-newsletter-issue-59-DEBUG.patch, failed testing.

The last submitted patch, 59: simplenews-2413995-replace-newsletter-issue-59-DEBUG.patch, failed testing.

Berdir’s picture

  1. +++ b/src/Plugin/Action/SendIssue.php
    @@ -0,0 +1,76 @@
    +        drupal_set_message(t('Newsletter %title is unpublished and will be sent on publish.', array('%title' => $node->label())));
    

    Newsletter *issue*

    add continue, do not add to spool.

    Why is this not shown in the output?

    Then that solves the order problem, because you have two messages with one issue each.

  2. +++ b/src/Plugin/Action/SendIssue.php
    @@ -0,0 +1,76 @@
    +        drupal_set_message(t('Sent the following newsletter(s): %titles.', array('%titles' => implode(', ', $sent_nodes))));
    +        simplenews_issue_update_sent_status($node, SIMPLENEWS_STATUS_SEND_READY);
    ...
    +        drupal_set_message(t('The following newsletter(s) are now pending: %titles.', array('%titles' => implode(', ', $sent_nodes))));
    +        simplenews_issue_update_sent_status($node, SIMPLENEWS_STATUS_SEND_PENDING);
    

    This needs to be done for all $sent_nodes.

Anushka-mp’s picture

all the nodes are now added to the mail spool. and the display messages are corrected.
I hope we Tackled the famous 'last bug' ;)

Lets see if this works.

Berdir’s picture

Status: Needs review » Needs work
  1. +++ b/config/install/views.view.simplenews_newsletters.yml
    @@ -0,0 +1,699 @@
    +uuid: 6fb2e64d-29ac-4e25-bd00-eee2d5637c83
    

    ;)

  2. +++ b/src/Plugin/Action/StopIssue.php
    @@ -0,0 +1,61 @@
    + * Sends a newsletter issue.
    + *
    + * @Action(
    + *   id = "simplenews_stop_action",
    

    Lie! ;)

  3. +++ b/src/Plugin/Action/StopIssue.php
    @@ -0,0 +1,61 @@
    + *   label = @Translation("Stop newsletter issue"),
    

    I think a better label would be "Stop sending". We don't stop the issue, we stop sending it.

  4. +++ b/src/Plugin/Action/StopIssue.php
    @@ -0,0 +1,61 @@
    +      // Set newsletter issue to not sent yet.
    +      $node->simplenews_issue->status = SIMPLENEWS_STATUS_SEND_READY;
    +      $node->save();
    

    Do we have tests for stop sending? READY means sending was completed ( I know, weird...), so it is the opposite of what we want, should be SEND_NOT.

  5. +++ b/src/Plugin/views/field/SendStatus.php
    @@ -0,0 +1,87 @@
    +/**
    + * @file
    + * Definition of Drupal\simplenews\Plugin\views\field\SendStatus.
    

    Contains \Drupal...

  6. +++ b/src/Plugin/views/field/SendStatus.php
    @@ -0,0 +1,87 @@
    +   * @param $node
    +   *   The node object.
    ...
    +  protected function getMessage($node) {
    

    Should have a type hint on NodeInterface in @param and method.

  7. +++ b/src/Plugin/views/field/SendStatus.php
    @@ -0,0 +1,87 @@
    +      $message['description'] = 'Newsletter issue sent to ' . $sent_count . ' subscribers.';
    ...
    +    elseif ($status == SIMPLENEWS_STATUS_SEND_PENDING) {
    ...
    +      $message['description'] = 'Newsletter issue will be sent to ' . $subscriber_count . ' subscribers.';
    ...
    +      $message['description'] = 'Newsletter issue will be sent to ' . $subscriber_count . ' subscribers on publish.';
    

    This needs to use t() with placeholders.

  8. +++ b/src/Tests/SimplenewsAdministrationTest.php
    @@ -540,6 +540,7 @@ class SimplenewsAdministrationTest extends SimplenewsTestBase {
         $this->assertTrue(preg_match('|admin/people/simplenews/edit/(\d+)\?destination|', $this->getUrl(), $matches), 'Subscriber found');
    +
         $subscriber = Subscriber::load($matches[1]);
    

    unneeded change.

  9. +++ b/src/Tests/SimplenewsAdministrationTest.php
    @@ -703,4 +704,96 @@ class SimplenewsAdministrationTest extends SimplenewsTestBase {
    +  /**
    +   * Test newsletter issue overview.
    +   */
    +  function testNewsletterIssuesOverview() {
    +    $admin_user = $this->drupalCreateUser(array(
    

    We also have some existing, commented out tests for the overview. Check them, either uncomment if they are testing anything that we are not already testing here (filtering, maybe?) or drop what is duplicate.

  10. +++ b/src/Tests/SimplenewsAdministrationTest.php
    @@ -703,4 +704,96 @@ class SimplenewsAdministrationTest extends SimplenewsTestBase {
    +    // Send unpublished newsletter issue using bulk operations.
    

    comment is not really correct I think, you send a published and an unpublished issue?

  11. +++ b/src/Tests/SimplenewsAdministrationTest.php
    @@ -703,4 +704,96 @@ class SimplenewsAdministrationTest extends SimplenewsTestBase {
    +        $this->assertEqual('Newsletter issue is pending.0 mails sent out of 3.', trim((string) $row->td[5]->img['title']));
    

    No space after .? I'd suggest to make this a single sentence, with a ,.

Anushka-mp’s picture

Changes made as suggested by berdir,
Tests added to test the stop send bulk operation. and logic corrected in StopSend.

Anushka-mp’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 70: simplenews-2413995-replace-newsletter-issue-70.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 70: simplenews-2413995-replace-newsletter-issue-70.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 70: simplenews-2413995-replace-newsletter-issue-70.patch, failed testing.

Anushka-mp’s picture

Why u do this? :'(

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 70: simplenews-2413995-replace-newsletter-issue-70.patch, failed testing.

Anushka-mp’s picture

Changed due to core and module updates and patch rerolled.

Anushka-mp’s picture

Status: Needs review » Needs work

The last submitted patch, 82: simplenews-2413995-replace-newsletter-issue-81.patch, failed testing.

  • Berdir committed f62183b on 8.x-1.x authored by Anushka-mp
    Issue #2413995 by Anushka-mp: Replace newsletter issue overview with a...
Berdir’s picture

Status: Needs work » Fixed

And... committed!

Anushka-mp’s picture

Yay! ^_^

Status: Fixed » Closed (fixed)

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