See #928110: Expose tracker2 tables and columns to views for background.

dawehner said he doesn't want any more context than that... blame him. ;)

Cheers,
-Derek

Files: 
CommentFileSizeAuthor
#32 drupal-1829734-32.patch16.8 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 54,742 pass(es).
[ View ]
#32 interdiff.txt1.01 KBdawehner
#30 interdiff.txt1.05 KBdawehner
#30 drupal-1829734-30.patch16.8 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 54,913 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#28 drupal-1829734-28.patch16.83 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 54,123 pass(es), 72 fail(s), and 69 exception(s).
[ View ]
#28 interdiff.txt3.16 KBdawehner
#26 1829734-26.patch17.05 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 53,158 pass(es).
[ View ]
#26 interdiff.txt2.83 KBdamiankloip
#23 1829734-23.patch16.82 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 53,103 pass(es).
[ View ]
#23 interdiff.txt1.14 KBdamiankloip
#22 1829734-22.patch16.77 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 53,207 pass(es).
[ View ]
#18 drupal-1829734-18.patch7.41 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 52,601 pass(es), 65 fail(s), and 64 exception(s).
[ View ]
#18 interdiff.txt4.18 KBdawehner
#13 1829734-VDC-tracker_data-13.patch8.81 KBcam8001
PASSED: [[SimpleTest]]: [MySQL] 49,699 pass(es).
[ View ]
#13 interdiff.txt2.13 KBcam8001
#11 drupal-1829734-7.patch8.78 KBcam8001
FAILED: [[SimpleTest]]: [MySQL] 41,594 pass(es), 35 fail(s), and 195 exception(s).
[ View ]
#7 drupal-1829734-7.patch8.78 KBdawehner
Test request sent.
[ View ]
#4 1829734-3.views-tracker.patch9.95 KBdww
PASSED: [[SimpleTest]]: [MySQL] 1,603 pass(es).
[ View ]
#1 views-1829734-1.patch9.92 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 1,603 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new9.92 KB
PASSED: [[SimpleTest]]: [MySQL] 1,603 pass(es).
[ View ]

Let's just do that.

Status:Needs review» Needs work

I just tried testing this on a local site. Fundamentally, it's working (at least the uid_touch replacement, which is the main thing I care about). A few things I found in the process:

A)

Strict warning: Declaration of views_handler_argument_tracker_comment_user_uid::query() should be compatible with that of views_handler_argument_comment_user_uid::query() in require_once() (line 13 of /.../sites/all/modules/views/modules/tracker/views_handler_argument_tracker_comment_user_uid.inc).

B) UI-wise, I was expecting the uid_touch handler to live in the 'Tracker - User' group, not directly in 'Tracker' (since it's user-centric tracker data).

Otherwise, it all seems to be working great in my mild testing (I also added all the 'Tracker - User' fields and they're working, too).

Status:Needs work» Needs review

Like so. Tested somewhat extensively. ;) This is working for me locally to resolve #1828756: D7 views don't honor tracker and issue following.

StatusFileSize
new9.95 KB
PASSED: [[SimpleTest]]: [MySQL] 1,603 pass(es).
[ View ]

Doh! The patch would help. ;)

Version:7.x-3.x-dev» 8.x-3.x-dev
Status:Needs review» Patch (to be ported)
Issue tags:+VDC

Mh it seems to be that parts of my php configuration is a bit borked.

Your changes look perfect, so I committed and pushed it, to help the drupal.org update.
Let's push this to d8.

Sweet, thanks! I don't have bandwidth to re-roll this for core, but it should be fairly straight-forward.

Yay,
-Derek

Status:Patch (to be ported)» Needs review
StatusFileSize
new8.78 KB
Test request sent.
[ View ]

So here we go.

Project:Views» Drupal core
Version:8.x-3.x-dev» 8.x-dev
Component:tracker data» tracker.module
Issue tags:+Needs tests

Ah-ha, it was hiding out in the Views queue. :)

+++ b/core/modules/tracker/lib/Drupal/tracker/Plugin/views/filter/TrackerCommentUserUid.phpundefined
@@ -0,0 +1,34 @@
+ * UID filter to check for nodes that user posted or commented on.
+++ b/core/modules/tracker/tracker.views.incundefined
@@ -0,0 +1,184 @@
+ * Implementation of hook_views_data().
...
+ * Implementation of hook_views_data_alter().

Just a quick note since this has come up in a couple patches in a row. :) The doxygen standard say these summaries should start with a third-person verb, as in "Defines a UID filter for nodes that the user posted or commented on" and "Implements hook_views_data()."

Also moving to the correct queue, tagging for test coverage, and adding to the meta at #1853522: [META] (Re)introduce Views data integration for core modules.

Oh right, this happens if you steal some code from contrib :)

StatusFileSize
new8.78 KB
FAILED: [[SimpleTest]]: [MySQL] 41,594 pass(es), 35 fail(s), and 195 exception(s).
[ View ]

Reupping to retest.

Status:Needs review» Needs work

The last submitted patch, drupal-1829734-7.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.13 KB
new8.81 KB
PASSED: [[SimpleTest]]: [MySQL] 49,699 pass(es).
[ View ]

Let's try this.

#13: 1829734-VDC-tracker_data-13.patch queued for re-testing.

Status:Needs work» Needs review

+++ b/core/modules/tracker/lib/Drupal/tracker/Plugin/views/argument/TrackerCommentUserUid.phpundefined
@@ -0,0 +1,35 @@
+ *   id = "tracker_comment_user_uid"

This needs a 'module' key too.

+++ b/core/modules/tracker/lib/Drupal/tracker/Plugin/views/filter/BooleanOperator.phpundefined
@@ -0,0 +1,42 @@
+ *   id = "boolean_operator"

same here.

+++ b/core/modules/tracker/lib/Drupal/tracker/Plugin/views/filter/TrackerCommentUserUid.phpundefined
@@ -0,0 +1,35 @@
+ *   id = "tracker_comment_user_uid"

and here.

+++ b/core/modules/tracker/tracker.views.incundefined
@@ -0,0 +1,184 @@
+ * Implementation of hook_views_data().

Implements ...

+++ b/core/modules/tracker/tracker.views.incundefined
@@ -0,0 +1,184 @@
+      'click sortable' => TRUE,

+++ b/core/modules/tracker/tracker.views.incundefined
@@ -0,0 +1,184 @@
+      'click sortable' => TRUE,

+++ b/core/modules/tracker/tracker.views.incundefined
@@ -0,0 +1,184 @@
+      'click sortable' => TRUE,

We can remove these now.

Also, the BooleanOperator class has a plugin id of 'boolean_operator' but this is not being used in the views data. However, 'tracker_boolean' is. I guess that's what the plugin id is meant to be?

Status:Needs review» Needs work

I guess that also confirms we need the extra tests for this :)

Status:Needs review» Needs work

+++ b/core/modules/tracker/lib/Drupal/tracker/Plugin/views/filter/BooleanOperator.phpundefined
@@ -0,0 +1,42 @@
+  /**
+   * Overrides \Drupal\tracker\Plugin\views\filter\BooleanOperator::query().
+   */
+  public function query() {
+    $this->ensure_my_table();
+    $where = "$this->table_alias.$this->real_field ";
+    if (empty($this->value)) {
+      $where .= '= 0';
+      if ($this->accept_null) {
+        $where = '(' . $where . " OR $this->table_alias.$this->real_field IS NULL)";
+      }
+    }
+    else {
+      $where .= '= 1';
+    }
+    $this->query->add_where_expression($this->options['group'], $where);

Thinking about it, do we even need this handler? I think the BooleanOperator can handle most of this? If not, could we add what we need to that, as it won't really be much..

Status:Needs work» Needs review
StatusFileSize
new4.18 KB
new7.41 KB
FAILED: [[SimpleTest]]: [MySQL] 52,601 pass(es), 65 fail(s), and 64 exception(s).
[ View ]

You are so right, ... this file is not needed anymore.

#18: drupal-1829734-18.patch queued for re-testing.

Let's see what happens, but this is looking good. I created #1941830: Convert tracker listings to a view to create a new default tracker view.

I might look at some test coverage.

Status:Needs review» Needs work
Issue tags:-VDC

The last submitted patch, drupal-1829734-18.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new16.77 KB
PASSED: [[SimpleTest]]: [MySQL] 53,207 pass(es).
[ View ]

Let's try this out. I have:

  • Changed the filter handler so it wasn't extending comment modules UserUid handler, this didn't really make sense? In D7 it extended the user name handler? Otherwise we can't do anything with this filter (We might want to change the argument too, not sure).
  • Added some basic test coverage for these two handlers, so we can atleast see they are filtering correctly.
  • Change the plugin names a bit

And, sorry, forgot an interdiff!

StatusFileSize
new1.14 KB
new16.82 KB
PASSED: [[SimpleTest]]: [MySQL] 53,103 pass(es).
[ View ]

Maybe just use removeItem in the test instead.

Issue tags:-Needs tests+VDC

.

Well, the reason to name have comment in the name was that this tracker not only takes care of the UID (which often just refers to the author)
but yeah I don't care.

Thanks for continuing to work on that.

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/Views/TrackerTestBase.phpundefined
@@ -0,0 +1,50 @@
+abstract class TrackerTestBase extends ViewTestBase {
...
+    $this->comment = array(
+      'subject' => $this->randomName(),
+      'comment_body[' . LANGUAGE_NOT_SPECIFIED . '][0][value]' => $this->randomName(20),
+    );
+    $this->drupalPost('comment/reply/' . $this->node->id(), $this->comment, t('Save'));

Why not use entity_create or similar instead of executing a post request, this seems to be too much.

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/Views/TrackerUserUidTest.phpundefined
@@ -0,0 +1,65 @@
+    $view->destroy();
...
+    $view->destroy();
...
+    $view->destroy();

Just a nitpick, but we seem to destroy after executing and before something else. This seems to make it a bit easier to read.

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/Views/TrackerUserUidTest.phpundefined
@@ -0,0 +1,65 @@
+    $this->assertEqual(count($view->result), 1);
...
+    $this->assertEqual(count($view->result), 0);
...
+    $this->assertEqual(count($view->result), 1);

Shouldn't we also check the actual result?

StatusFileSize
new2.83 KB
new17.05 KB
PASSED: [[SimpleTest]]: [MySQL] 53,158 pass(es).
[ View ]

Thanks! All good points, The test coverage was thrown together pretty quickly, so yeah some of those improvements are welcome!

+++ b/core/modules/tracker/lib/Drupal/tracker/Plugin/views/argument/TrackerUserUid.phpundefined
@@ -0,0 +1,36 @@
+use Drupal\comment\Plugin\views\argument\UserUid as UserUid;

This 'as' is redundant now

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/Views/TrackerTestBase.phpundefined
@@ -0,0 +1,51 @@
+    $this->user = $this->drupalCreateUser($permissions);
+
+    $this->drupalLogin($this->user);

Do you actually use $this->user elsewhere? If so, it should be a protected property above the method. If not, just have $this->drupalLogin($this->drupalCreateUser($permissions));

+++ b/core/modules/tracker/tracker.views.incundefined
@@ -0,0 +1,181 @@
+ * Implementation of hook_views_data_alter().

Implements

StatusFileSize
new3.16 KB
new16.83 KB
FAILED: [[SimpleTest]]: [MySQL] 54,123 pass(es), 72 fail(s), and 69 exception(s).
[ View ]

Let's fix the classname and some other stuff as well.

Status:Needs review» Needs work

The last submitted patch, drupal-1829734-28.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new16.8 KB
FAILED: [[SimpleTest]]: [MySQL] 54,913 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
new1.05 KB

Ups, failed to convert the annotation properly.

Status:Needs review» Needs work

The last submitted patch, drupal-1829734-30.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.01 KB
new16.8 KB
PASSED: [[SimpleTest]]: [MySQL] 54,742 pass(es).
[ View ]

That's not complicated to fix.

Status:Needs review» Reviewed & tested by the community

Looking good! No complaints

Yes, looks great to me, too. Happy to see my code from tracker2 finally getting into core, now with tests and lots of other goodness. :)

Thanks!
-Derek

Assigned:Unassigned» catch

These are for catch. :)

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

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