Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comments
Comment #1
miro_dietikerComment #2
Anushka-mp CreditAttribution: Anushka-mp commentedNewsletter 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.
Comment #4
Anushka-mp CreditAttribution: Anushka-mp commentedTrying again.
Comment #5
BerdirComment #6
miro_dietikerSome quick feedback.
I don't see this implemented, is it?
I guess u want to pass in $subscriber_count instead.
2 Garbage lines. :-)
Comment #7
Anushka-mp CreditAttribution: Anushka-mp commentedSubscriber count dropped. Send Status field will have the subscriber count.
fancy images added to the send status :)
node_send_action => simplenews_send_action
Comment #8
Anushka-mp CreditAttribution: Anushka-mp commentedScreenshot of the view.
Comment #9
miro_dietiker1) 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.
Comment #10
Berdir1. 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.
Comment #11
miro_dietikerI 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.
Comment #12
Anushka-mp CreditAttribution: Anushka-mp commentedChanges 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.
Comment #13
Anushka-mp CreditAttribution: Anushka-mp commentedComment #14
Anushka-mp CreditAttribution: Anushka-mp commentedTest 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' :)
Comment #16
Anushka-mp CreditAttribution: Anushka-mp commentedAs discussed with berdir,
the image removed if the newsletter is not sent.
and the tests changed accordingly.
Comment #17
Anushka-mp CreditAttribution: Anushka-mp commentedsome tests were commented out in the earlier patch.
Comment #22
Anushka-mp CreditAttribution: Anushka-mp commentedHmm.. 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.
Comment #23
Anushka-mp CreditAttribution: Anushka-mp commentedComment #25
Anushka-mp CreditAttribution: Anushka-mp commentedConflicting part from the URL comparison string removed.
Comment #30
Anushka-mp CreditAttribution: Anushka-mp commenteduploading multiple the same patch multiple times to see testbot results
Comment #48
Anushka-mp CreditAttribution: Anushka-mp commentedOh :(
patch rerolled.
Comment #53
BerdirSo 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) {
....
Comment #54
Anushka-mp CreditAttribution: Anushka-mp commentedhm, bad testbot (should make it slower a bit) :-)
Tests modified to dynamically check the newsletter view.
Comment #55
Anushka-mp CreditAttribution: Anushka-mp commentedComment #56
BerdirYou 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.
Comment #57
Anushka-mp CreditAttribution: Anushka-mp commentedrow count assertion added as suggested.
Comment #59
Anushka-mp CreditAttribution: Anushka-mp commentedDebug added to see the test bot outputs.
Comment #60
BerdirComment #66
BerdirNewsletter *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.
This needs to be done for all $sent_nodes.
Comment #67
Anushka-mp CreditAttribution: Anushka-mp commentedall 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.
Comment #69
Berdir;)
Lie! ;)
I think a better label would be "Stop sending". We don't stop the issue, we stop sending it.
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.
Contains \Drupal...
Should have a type hint on NodeInterface in @param and method.
This needs to use t() with placeholders.
unneeded change.
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.
comment is not really correct I think, you send a published and an unpublished issue?
No space after .? I'd suggest to make this a single sentence, with a ,.
Comment #70
Anushka-mp CreditAttribution: Anushka-mp commentedChanges made as suggested by berdir,
Tests added to test the stop send bulk operation. and logic corrected in StopSend.
Comment #71
Anushka-mp CreditAttribution: Anushka-mp commentedComment #77
Anushka-mp CreditAttribution: Anushka-mp commentedWhy u do this? :'(
Comment #81
Anushka-mp CreditAttribution: Anushka-mp commentedChanged due to core and module updates and patch rerolled.
Comment #82
Anushka-mp CreditAttribution: Anushka-mp commenteduploading few more times because of random failures.
Comment #85
BerdirAnd... committed!
Comment #86
Anushka-mp CreditAttribution: Anushka-mp commentedYay! ^_^