Add test coverage for Views revision link handlers

/me sighs

Not sure whether you (other person) will find this code helpful, but maybe you do so here is it.

Working on that ... what a pain.

This is really just temporary code-tracking.

Re-roll. Interdiff is for minor change in core.

Updated the old view and minor nip tuck.

This will fix the test.

This will fix the test.

#10: 1863898-10.patch queued for re-testing.

Well according to verbose messages the test is correct. I have the same 8 fails locally.

Here's a re-roll of #10

I've fixed some minor things, like invoking camel cased views methods and fixing the "revisions" link as it should be in plural form, also the test view was disabled so no way to test the path.

Also removed testing the view link as the view op is always available (no particular permission for that).

Here is the reroll and some minor fixes.

:/

Here are a couple more fixes. This should also fix the RevisionLinkTest test too.

1. +++ b/core/modules/node/lib/Drupal/node/Plugin/views/field/RevisionLink.php
   @@ -21,29 +21,48 @@
   +  public $countRevisions;
   

   No need for a public property.

2. +++ b/core/modules/node/lib/Drupal/node/Plugin/views/field/RevisionLink.php
   @@ -21,29 +21,48 @@
   +  function preRender(&$values) {
   

   There should be a visibility.

3. +++ b/core/modules/node/lib/Drupal/node/Plugin/views/field/RevisionLink.php
   @@ -21,29 +21,48 @@
   +      $this->countRevisions = db_query("SELECT nid, COUNT(vid) as count FROM {node_field_revision} WHERE nid IN (:nids) GROUP BY nid", array(':nids' => array_unique($nids)))->fetchAllKeyed();
   

   Let's use an injected database connection.

4. +++ b/core/modules/node/lib/Drupal/node/Plugin/views/field/RevisionLinkDelete.php
   @@ -20,18 +20,19 @@
   +    if (!($access = (user_access("delete $type revisions") || user_access('delete all revisions') || user_access('administer nodes')) && node_access('delete', $revision_node))) {
   
   +++ b/core/modules/node/lib/Drupal/node/Plugin/views/field/RevisionLinkRevert.php
   @@ -20,18 +20,19 @@
   +    if (!($access = (user_access("revert $type revisions") || user_access('revert all revisions') || user_access('administer nodes')) && node_access('update', $node))) {
   

   We can also replace user_access with \Drupal::currentUser()->hasPermission

working on it.

Fixed #21 and some clean up.

Fixed tests and typo :S

1. +++ b/core/modules/node/lib/Drupal/node/Plugin/views/field/Link.php
   @@ -55,11 +56,21 @@ public function render(ResultRow $values) {
      protected function renderLink($node, ResultRow $values) {
   

   Can we also typehint on the actual code?

2. +++ b/core/modules/node/lib/Drupal/node/Tests/Views/RevisionLinkTest.php
   @@ -0,0 +1,106 @@
   +    $this->assertNoLinkByHref($uri['path'] . '/revisions/' . $this->revisions[0]->vid->value . '/delete');
   +    $this->assertNoLinkByHref($uri['path'] . '/revision/' . $this->revisions[0]->vid->value . '/revert');
   ...
   +    $this->assertLinkByHref($uri['path'] . '/revisions/' . $this->revisions[1]->vid->value . '/view');
   +    $this->assertLinkByHref($uri['path'] . '/revisions/' . $this->revisions[1]->vid->value . '/delete');
   +    $this->assertLinkByHref($uri['path'] . '/revisions/' . $this->revisions[1]->vid->value . '/revert');
   ...
   +    $this->assertNoLinkByHref($uri['path'] . '/revisions/' . $this->revisions[2]->vid->value . '/delete');
   +    $this->assertNoLinkByHref($uri['path'] . '/revisions/' . $this->revisions[2]->vid->value . '/revert');
   ...
   +          $this->assertLinkByHref($uri['path'] . '/revisions/' . $this->revisions[1]->vid->value . '/' . $operation);
   ...
   +          $this->assertNoLinkByHref($uri['path'] . '/revisions/' . $this->revisions[1]->vid->value . '/' . $operation);
   

   Can't we just use getRevisionId() ?

Fixed #27.

+++ b/core/modules/node/lib/Drupal/node/Plugin/views/field/Link.php
@@ -43,6 +44,7 @@ public function buildOptionsForm(&$form, &$form_state) {
+    $this->ensureMyTable();

Do we really need to add this call here? We probably don't really even need the query method? or empty query method?

Otherwise, this is looking pretty good.

I have removed query method. And reverting the typehinit of renderLink to match the parent definition.

or empty query method?

Let's try that.

I vote just reverting the changes to the query method and leaving it how it was. I think that will work for us?

Reverted.

It is a quite day for VDC team anyone want to RTBC.

Reverted all the changes of query method.

Sorry, let's go with the patch in #35! I guess we do need to ensure the table..

rtbc for #35.

+++ b/core/modules/node/lib/Drupal/node/Plugin/views/field/RevisionLinkRevert.php
@@ -20,18 +20,19 @@
+    if (!($access = ($this->currentUser->hasPermission("revert $type revisions") || $this->currentUser->hasPermission('revert all revisions') || $this->currentUser->hasPermission('administer nodes')) && node_access('update', $node))) {

Why's the access check being taken out of access() and put into the rendering?

Because access() is only called when handlers are initialised. If it failed here the handler would be removed from the view altogether. This needs to use the current node to determine the access, as well as user permissions.

Won't preRender() run before renderLink() though? If so we're counting all the revisions for all the nodes when generating the delete/revert links, even if the user never has a chance of seeing them.

Also it might be too early here, but I can't see the access check being added back for the view link?

-  public function access() {
-    return user_access('view revisions') || user_access('administer nodes');
+  /**

+  /**
+   * {@inheritdoc}
+   */
   protected function renderLink($data, ResultRow $values) {
-    list($node, $vid) = $this->get_revision_entity($values, 'view');
-    if (!isset($vid)) {
-      return;
-    }
+    list($node, , $vid) = $this->getRevisionEntity($values, 'view');
 
     // Current revision uses the node view path.
-    $path = 'node/' . $node->nid;
-    if (!$node->isDefaultRevision()) {
+    $uri = $node->uri();
+    $path = $uri['path'];
+    if ($this->countRevisions[$node->id()] > 1 && $node->getRevisionId() != $vid) {
       $path .= "/revisions/$vid/view";
     }

I spoke to catch about this, we need to add back access. We have two levels here, whether we can view the node or not and whether we have a revision to access etc.. We need to check user_access still, as probably node_access() to determine if we even show a view link. So I think the current logic to revision linking is ok, we just need to add back the access stuff in some form. However, probably not in the access() method like before?

35: drupal8.node-module.1863898-35.patch queued for re-testing.

(Merging "node system" and "node.module" components for 8.x; disregard.)

This seems like a major regression to me, and it's also blocking views conversions.

37: drupal8.node-module.1863898-37.patch queued for re-testing.

Re-roll. I ran out of time to figure out what to do about access(). Had to change node_access to node.access(). But, I think tests are going to fail there.

Fixed a double use statement.

Get the re-roll up to date with Core changes.

Patch needs reroll.

Rerolled.

Testing

Rerolled and patched successfully with no conflicts/errors.

From #52

I ran out of time to figure out what to do about access().

Has the link access issue been resolved in the subsequent rerolls (it's hard to tell without interdiffs)? That comment is the only mention of catch's concern from #43.

access() is only called on handler initialization, so the access check was moved to renderLink(). My understanding is that we need a better place for the access check, so that preRender doesn't get called?

There must be similar access checks somewhere that give a clue how this should be done.

Just making a couple of nitpick changes to get the ball rolling on this.

Regarding the access discussion, last comment from @damiankloip is #1863898-44: Add test coverage for Views revision link handlers which suggests that we need to get the access check back, it appears that there are two kind of checks here:

1. Generic user permissions, i.e. $this->currentUser->hasPermission('delete all revisions')
2. Specific check for the user in the context, i.e. $revision_node->access('delete')

Could we add 1) back to access() method and keep 2) in the renderLink() method?
I understand this is an issue just for the Revert and Delete revision links.

I need to re-review this myself to make sure it's still relevant, but here is a re-roll.

Still haven't look at the relevancy, but here is a start: Let's see the test only patch.

Hmm...the test only patch is passing, so it either isn't testing this issue, or this issue has been fixed elsewhere.

The test wasn't running. I moved them to the right place, but the test don't seem to work either.

Patch above is empty.

Oh yeah, these are new files so git diff didn't pick them up.

Nice. Can you upload the combined patch now that the test is being run?

Included interdiff to show what was changed in the view. Keep in mind that the test is failing (but not necessarily failing for the reasons that it should.) So, I still haven't tried to verify that the bug is still relevant. For example, the first fail is "Link containing href node/1 found." But, I think that we are expecting that part should work (even without the fix from this patch.)

First, Needs a reroll.
Then, Needs someone to look at the newly added test. Verify that the test is actually testing the bug properly.

I think the the problem has been fixed in #2322949: Implement generic entity link view field handlers.

I've rerolled the patch, the test demonstrates that the links are displayed correctly.

Which patch was rerolled? Also, was this just a reroll or does it also have additional changes?

The patch #87 contains only the test from #79. Other changes from #79 I think aren't relevant anymore.

It's just that 4 months ago the test was failing, and now it's passing. For completeness, it would be nice to know what changed.

@jeqq Basically, this needs an interdiff so that it the changes can be reviewed. It looks like 87 was more than a re-roll.

https://www.drupal.org/documentation/git/interdiff

+++ b/core/modules/node/src/Tests/Views/RevisionLinkTest.php
@@ -0,0 +1,96 @@
+  public $revisions = array();

I think this is now unused and can be deleted. Other than that, it looks like we should commit this test.

Thanks @lokapujya. I've removed the unused field. Unfortunately, for some reason I can't create an interdiff.

andrei@Andrei-NB:/var/www/drupal8dev$ interdiff views_revision_link-1863898-87.patch views_revision_link-1863898-94.patch > interdiff-87-94.txtThe next patch would create the file /tmp/interdiff-1.wZWeGm,
which already exists!  Assume -R? [n] 
Apply anyway? [n] 
interdiff: Error applying patch1 to reconstructed file
andrei@Andrei-NB:/var/www/drupal8dev$

It would probably be easier to create 2 branches. First with the old patch, Second with the new patch. Then just diff the 2 branches.

+++ b/core/modules/node/src/Tests/Views/RevisionLinkTest.php

@@ -0,0 +1,101 @@
+  }
+  ¶

Some whitespace snuck in there.

Removed the whitespace.

Interdiff for reviewing. Oops I can see this is wrong by the filesize. Will try again later.

Had to adjust the diff between 76 and 96 to only include the test. Also, it seems the directory name of the test changed; In order to see what actually changed in the file, in the older branch, I first moved the the file from the old directory to the new one.

@jeqq: Can you verify that the interdiff is right?

@lokapujya: The interdiff is right, thanks.

Now we can add this new test coverage.

Still needs an issue summary update... and a title update.

no_angel, are you still reviewing this?

RTBC per #104. If further IS update is needed, please specify.

1. +++ b/core/modules/node/src/Tests/Views/RevisionLinkTest.php
   @@ -0,0 +1,101 @@
   +/**
   + * @file
   + * Contains \Drupal\node\Tests\Views\RevisionLinkTest.
   + */
   

   Not needed

2. +++ b/core/modules/node/src/Tests/Views/RevisionLinkTest.php
   @@ -0,0 +1,101 @@
   +  /**
   +   * Test nodes.
   +   *
   +   * @var array
   +   */
   +  public $nodes = array();
   

   I don't think there is any need for this to be a public property on the class. How about just a variable $nodes in the scope of the testRevisionLinks() method?

3. +++ b/core/modules/node/src/Tests/Views/RevisionLinkTest.php
   @@ -0,0 +1,101 @@
   +    $operations = array_keys($accounts);
   +    array_shift($operations);
   ...
   +      foreach ($operations as $operation) {
   

   This is makes someone reading the test have to think way too hard. Just do foreach (['revert', 'delete'] as $operation) {

4. +++ b/core/modules/node/src/Tests/Views/RevisionLinkTest.php
   @@ -0,0 +1,101 @@
   +    foreach ($accounts as $op => $account) {
   ...
   +        if ($operation == $op) {
   

   Let's change $op to something more explicit that helps someone reading the test - maybe $allowed_operation?

5. +++ b/core/modules/node/src/Tests/Views/RevisionLinkTest.php
   --- /dev/null
   +++ b/core/modules/node/tests/modules/node_test_views/test_views/views.view.test_node_revision_links.yml
   

   Given the view is over 12 months old it could probably do with being re-exported.

Addressed everything from #113. Amazingly there were no changes to the view on export!

The changes in the interdiff look good to me, thanks Alex!

Random DrupalCI fail...

Committed 5d4300e and pushed to 8.2.x and ecedab9