Problem/Motivation

history_node_view_alter() tracks the viewing history for all nodes that are viewed in the full view mode.

That means also previews of unsaved nodes are tracked.

Proposed resolution

Return early if $node->isNew() or if isset($node->in_preview).

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

None.

CommentFileSizeAuthor
#15 3122056-14.patch2.92 KBwim leers
#15 3122056-13-tests-only.patch2.24 KBwim leers
#14 3122056-14.patch3.14 KBwim leers
#14 interdiff-2.txt3.05 KBwim leers
#14 interdiff.txt602 byteswim leers
#14 3122056-13-tests-only.patch2.46 KBwim leers
#10 interdiff-3122056-2-10.txt9.42 KBmohit_aghera
#10 3122056-10.patch10.06 KBmohit_aghera
#6 Screen Shot 2020-07-12 at 11.47.36 PM.png31.14 KBkristen pol
#6 Screen Shot 2020-07-12 at 11.47.12 PM.png26.74 KBkristen pol
#6 Screen Shot 2020-07-12 at 11.31.04 PM.png37.77 KBkristen pol
#6 Screen Shot 2020-07-12 at 11.33.09 PM.png50.25 KBkristen pol
#6 Screen Shot 2020-07-12 at 11.41.42 PM.png67.69 KBkristen pol
#2 3122056-2.patch743 byteswim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new 3122056-2.patch743 bytes

.

geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented
Status: Needs review » Reviewed & tested by the community

The patch is trivial and makes sense. Did not test it though.

geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented
Status: Reviewed & tested by the community » Needs review

Ups.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Category: Task » Bug report
Issue tags: +Bug Smash Initiative
StatusFileSize
new Screen Shot 2020-07-12 at 11.41.42 PM.png67.69 KB
new Screen Shot 2020-07-12 at 11.33.09 PM.png50.25 KB
new Screen Shot 2020-07-12 at 11.31.04 PM.png37.77 KB
new Screen Shot 2020-07-12 at 11.47.12 PM.png26.74 KB
new Screen Shot 2020-07-12 at 11.47.36 PM.png31.14 KB

Thanks for the patch.

1) Patch applies cleanly to 9.1.x.

2) Code is fine and addresses problem noted in the issue summary.

3) Started automated tests on 9.1.x since they were previously run against 8.9.x. This is RTBC IMO if these pass which is expected.

4) Manually tested with and without the patch as follows:

  1. Created a content view that has a node title field and a "Content: Has new content (New or updated)" field
  2. Created a couple nodes normally and then look at the views listing
  3. None of the nodes show as "Updated" because I looked at the node view pages for the new nodes
  4. Edit a node normally and use preview and then look at the views listing
  5. Before patch:
    Node being previewed shows as "Updated" because of bug

    After patch:
    None of the nodes show as "Updated" because of fix

  6. Edit one of the nodes adding a ?destination=/ parameter so that it won't load the node view page after editing and then look at the views listing
  7. Edited node show as "Updated" because I haven't looked at the node view page since it was edited
  8. Look at the last-edited node and then look at the views listing
  9. None of the nodes show as "Updated" because I looked at the node view pages
  10. Go to /admin/content, choose the nodes in the list, select the Save content action, and then look at the views listing
  11. All of the nodes show as "Updated" because I "saved" them all and didn't look at their node view pages

Note: I didn't upload screenshots for all the variations. Also, IMO this is a bug so reclassifying.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Status: Needs review » Reviewed & tested by the community

RTBC per #6 now that tests have passed.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Thanks for filing this bug report and for fixing the issue. Bug fixing is very valuable. In order to commit a bug fix, we need an automated to test to prove that we've fixed the bug and ensure that we don't break it again in the future. For more information about writing tests in Drupal 8 see the following links:

  1. https://www.drupal.org/docs/8/testing
  2. https://api.drupal.org/api/drupal/core%21core.api.php/group/testing/8.9.x

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

mohit_aghera’s picture

mohit_aghera commented
Status: Needs work » Needs review
StatusFileSize
new 3122056-10.patch10.06 KB
new interdiff-3122056-2-10.txt9.42 KB

Adding a WIP patch with test cases to get inputs from other contributors related to blocker

Status: Needs review » Needs work

The last submitted patch, 10: 3122056-10.patch, failed testing. View results

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » wim leers 
  1. +++ b/core/modules/history/tests/src/Functional/HistoryTrackingTest.php
@@ -0,0 +1,65 @@
+   * Modules to enable.
+   *
+   * @var array
+   */

    Nit: {@inheritdoc} 

  2. +++ b/core/modules/history/tests/src/Functional/HistoryTrackingTest.php
@@ -0,0 +1,65 @@
+  /**
+   * Views used by this test.
+   *
+   * @var array
+   */
+  public static $testViews = ['test_viewing_history'];

    Interesting … I would not have used a view to test this. I'd just have checked this using history_read(). 

  3. +++ b/core/modules/history/tests/src/Functional/HistoryTrackingTest.php
@@ -0,0 +1,65 @@
+  protected static $modules = ['node', 'history', 'views_ui'];

    We definitely do not need views_ui. 

  4. +++ b/core/modules/history/tests/src/Functional/HistoryTrackingTest.php
@@ -0,0 +1,65 @@
+    $this->drupalGet('admin/structure/views/view/test_viewing_history');
+    $this->submitForm([], 'Update preview');
+    $this->assertSession()->pageTextContains('New');

    Oh I see … this is testing the views preview, not the node preview.

Hm, yeah, this is unfortunately not actually testing what we need to test. Let's fix that.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Do not track viewing history for unsaved entities » Do not track viewing history for unsaved entities, nor when previewing existing entities
Assigned: wim leers » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new 3122056-13-tests-only.patch2.46 KB
new interdiff.txt602 bytes
new interdiff-2.txt3.05 KB
new 3122056-14.patch3.14 KB

… and in doing so, I found a bug in #2, or rather, an unhandled edge case. 🤓

The interdiff is relative to my patch in #2 since I was unable to reuse anything in #10 unfortunately. On the bright side, I was able to just expand the pre-existing test coverage :)

Note that interdiff-2.txt shows the full interdiff, interdiff.txt shows only the functional changes (no test coverage).

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new 3122056-13-tests-only.patch2.24 KB
new 3122056-14.patch2.92 KB

😬 The fact that this did not run any tests at all is infuriating.

Hacked the patch files by hand. 🤞

The last submitted patch, 15: 3122056-13-tests-only.patch, failed testing. View results

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

Makes sense to me. My only nitpick, and it is a nitpick, is that we should probably use \Drupal::database()->select() in the test, rather than ->query().

But that's non-blocking feedback; marking RTBC anyway.

The last submitted patch, 15: 3122056-13-tests-only.patch, failed testing. View results

The last submitted patch, 15: 3122056-13-tests-only.patch, failed testing. View results

The last submitted patch, 15: 3122056-13-tests-only.patch, failed testing. View results

The last submitted patch, 15: 3122056-13-tests-only.patch, failed testing. View results

The last submitted patch, 15: 3122056-13-tests-only.patch, failed testing. View results

The last submitted patch, 15: 3122056-13-tests-only.patch, failed testing. View results

The last submitted patch, 15: 3122056-13-tests-only.patch, failed testing. View results

  • catch committed 23d59bb on 9.3.x 
    Issue #3122056 by Wim Leers, mohit_aghera, Kristen Pol: Do not track...

  • catch committed 6511a43 on 9.2.x 
    Issue #3122056 by Wim Leers, mohit_aghera, Kristen Pol: Do not track...
catch’s picture

catch
he/him
Primary language English
commented
Version: 9.3.x-dev » 9.2.x-dev
Status: Reviewed & tested by the community » Fixed

The ->query() in the test is fine - as long as it's a proper static query it's still OK to use.

Committed/pushed to 9.3.x and cherry-picked to 9.2.x, thanks!

Status: Fixed » Closed (fixed)

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