Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
9.92 KB

Let's just do that.

dww’s picture

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).

dww’s picture

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.

dww’s picture

Doh! The patch would help. ;)

dawehner’s picture

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.

dww’s picture

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

Yay,
-Derek

dawehner’s picture

Status: Patch (to be ported) » Needs review
FileSize
8.78 KB

So here we go.

xjm’s picture

Project: Views (for Drupal 7) » 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.

xjm’s picture

dawehner’s picture

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

Cameron Tod’s picture

FileSize
8.78 KB

Reupping to retest.

Status: Needs review » Needs work

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

Cameron Tod’s picture

Status: Needs work » Needs review
FileSize
2.13 KB
8.81 KB

Let's try this.

dawehner’s picture

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

damiankloip’s picture

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?

damiankloip’s picture

Status: Needs review » Needs work

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

damiankloip’s picture

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..

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.18 KB
7.41 KB

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

damiankloip’s picture

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

damiankloip’s picture

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.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
16.77 KB

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!

damiankloip’s picture

FileSize
1.14 KB
16.82 KB

Maybe just use removeItem in the test instead.

damiankloip’s picture

Issue tags: -Needs tests +VDC

.

dawehner’s picture

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?

damiankloip’s picture

FileSize
2.83 KB
17.05 KB

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

tim.plunkett’s picture

+++ 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

dawehner’s picture

FileSize
3.16 KB
16.83 KB

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
16.8 KB
1.05 KB

Ups, failed to convert the annotation properly.

Status: Needs review » Needs work

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.01 KB
16.8 KB

That's not complicated to fix.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Looking good! No complaints

dww’s picture

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

xjm’s picture

Assigned: Unassigned » catch

These are for catch. :)

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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