Following on from discussions in other issues (linked) it has become clear that we need to provide a means for administrators to (a) detect the problem of missing nodes and (b) tidy up the Scheduler table. I have made some enhancements to the 'Scheduled' tab which lists nodes that are scheduled. Currently the rows have an edit link which is broken when the node does not exist.

Here is an example of the existing table

Here is the same data with my enhancements

The changes made are detailed as follows:

  1. New menu link admin/content/scheduler_delete/% which calls confirmation form for deleting the row. Access only for users with 'Administer Scheduler' permission
  2. Added Node Type and Node Status into the scheduler content list
  3. In SQL, take nid from Scheduler table not the node table, to allow more complete info on the data being displayed.
  4. Use $destination = drupal_get_destination() so that after Edit or Delete is performed (or cancelled) we return to the calling page (currently goes to front page)
  5. Added 'delete' operation for good nodes using normal node/$nid/delete path
  6. Added 'delete' operation for bad rows, using new admin/content/scheduler_delete/$nid path
  7. Removed the 'edit' operation for bad rows as this does not work and gives 'page not found'
  8. For the pager row at the bottom of the table, make the colspan dynamic, for easier future addition/deletion of columns
  9. If there are no scheduled nodes, have a different message if viewing just one users nodes
  10. New function _scheduler_delete_row_confirm() to provide confirmation before deleting the bad row in Scheduler table
  11. Associated submit function _scheduler_delete_row_confirm_submit() to call scheduler_node_delete() is user confirmed.
  12. Change in scheduler_node_delete() to accept a plain node id as the parameter in addition to a node object

Patch coming up

CommentFileSizeAuthor
#7 2110983_7.scheduled_list_ui.patch6.53 KBjonathan1055
#6 2110983_3_to_6.interdiff.txt2.54 KBjonathan1055
#6 2110983_6.scheduled_list_ui.patch6.52 KBjonathan1055
#3 2110983_3.scheduled_list_ui.patch6.87 KBjonathan1055
#2 2110983_2.scheduled_list_ui.patch6.83 KBjonathan1055
#1 2110983_1.scheduled_list_ui.patch6.85 KBjonathan1055
missing nodes 2.png90.07 KBjonathan1055
missing nodes 1.png78.77 KBjonathan1055

Comments

jonathan1055’s picture

jonathan1055 commented
Status: Active » Needs review
StatusFileSize
new 2110983_1.scheduled_list_ui.patch6.85 KB

Patch against 1.1+22

jonathan1055’s picture

jonathan1055 commented
Title: Improved user interface for scheduled nodes list » Improve UI functionality for scheduled nodes list
StatusFileSize
new 2110983_2.scheduled_list_ui.patch6.83 KB

Re-rolled patch because #1 no longer applies, due to other commits since. This is against 1.1+27

jonathan1055’s picture

jonathan1055 commented
StatusFileSize
new 2110983_3.scheduled_list_ui.patch6.87 KB

Re-rolled following 1.1+34

jonathan1055’s picture

jonathan1055 commented
Issue summary: View changes
Related issues: +#1410184: Scheduler breaks on Cron if a node has been deleted outside Drupal., +#1516532: "Publish on" date not cleared after time has passed if article manually published from content list
pfrenssen’s picture

pfrenssen
Location Sofia
commented

The screenshots look really nice and useful. I have 15 minutes left on my commute, will attempt a code review in that time span! 

  1. +++ b/scheduler.module
@@ -81,6 +81,17 @@ function scheduler_menu() {
+  // Menu callback for confirmation before manually deleting corrupt rows in the
+  // Scheduler table. Cannot use normal node delete link because these rows no
+  // longer exist in the {node} table.
+  $items['admin/content/scheduler_delete/%'] = array(
+    'title' => 'Delete Scheduled Data',
+    'description' => 'Delete corrupt rows from Scheduler table',

    I would use the term "stale" or "obsolete" rather than "corrupt".

    And maybe a better fit for the path would be "admin/content/scheduler/delete/%" since it is a child page of "admin/content/scheduler"? 

  2. +++ b/scheduler.module
@@ -779,7 +855,9 @@ function scheduler_node_update($node) {
 function scheduler_node_delete($node) {
-  db_delete('scheduler')->condition('nid', $node->nid)->execute();
+  // Cater for passing a node object or just the node id.
+  $nid = is_object($node) ? $node->nid : $node;
+  db_delete('scheduler')->condition('nid', $nid)->execute();

    I don't really like this kind of type juggling. You could either factor out the deletion to a separate function scheduler_delete_data($nid), or simply copy the line of code that performs the deletion into the submit handler. It's only one line of very obvious code after all.

Code looks good overall!

jonathan1055’s picture

jonathan1055 commented
Issue summary: View changes
StatusFileSize
new 2110983_6.scheduled_list_ui.patch6.52 KB
new 2110983_3_to_6.interdiff.txt2.54 KB

Thanks for the review, good use of your last 15 minutes. I am also on the train commuting now. Pleased you like it overall.

I've addressed all your points. 'Obsolete' is better tha 'corrupt' you are right. Changed the menu path as that makes more sense. Also added the single db_delete() call into the handler.

Here is a patch against the latest code in git (which is not the latest -dev being shown) and an interdiff file

Jonathan

jonathan1055’s picture

jonathan1055 commented
StatusFileSize
new 2110983_7.scheduled_list_ui.patch6.53 KB

No changes to code but re-rolled as the latest patch failed to apply against 1.1+49-dev

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Status: Needs review » Fixed

Looking great. I tested it and it works well. It is more consistent with the standard content listing from the node module, and it is easier to use. Some nice small additions as well, like the destination parameter, and the possibility to clean up stale database entries! Thanks a lot Jonathan!

Committed to 7.x-1.x: commit 59635801.

jonathan1055’s picture

jonathan1055 commented

Glad you like it.
Happy Christmas!

Status: Fixed » Closed (fixed)

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