Problem/Motivation

I have set up the Umami demo on my local machine and on simplytest.me.

The order of articles/recipes on the home page is inconsistent.

Local:

Simplytest.me

This is because all the recipes/articles all have the same creation time, so the sort is undefined.

This in no ways impacts Umami when used as a demo, but does make it more difficult to A/B test changes, especially if we start using BackStop or similar to check for regressions.

Proposed resolution

Add creation dates to the content – either fixed or relative to the install date.

CommentFileSizeAuthor
#42 interdiff.txt715 bytescferthorney
#42 inconsistent-ordering-front-page-2983665-42.patch3.36 KBcferthorney
#38 inconsistent-ordering-front-page-2983665-38.patch2.5 KBcferthorney
#34 inconsistent-dates-2983665-34.patch16.09 KBzuhair_ak
#31 inconsistent-dates-2983665-31.patch18.52 KBzuhair_ak
#27 inconsistent-dates-2983665-27.patch18.52 KBsavkaviktor16@gmail.com
#25 inconsistent-dates-2983665-25.patch18.52 KBcferthorney
#22 inconsistent-dates-2983665-22.patch18.62 KBcferthorney
#20 inconsistent-dates-2983665-20.patch15.77 KBcferthorney
#18 inconsistent-dates-2983665-18.patch19.31 KBcferthorney
#16 inconsistent-dates-2983665-16.patch15.48 KBcferthorney
#12 inconsistent-dates-2983665-12.patch19.12 KBcferthorney
#11 inconsistent-dates-2983665-11.patch19.15 KBcferthorney
#10 inconsistent-dates-2983665-9.patch18.83 KBcferthorney
#8 inconsistent-dates-2983665-8.patch18.78 KBcferthorney
#5 inconsistent-dates-2983665-5.patch18.12 KBcferthorney
Screen Shot 2018-07-04 at 17.07.52.png2.24 MBJohn Cook
Screen Shot 2018-07-04 at 17.08.02.png3.14 MBJohn Cook
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

John Cook created an issue. See original summary.

Eli-T’s picture

Priority: Normal » Minor

Setting priority to minor as whilst this is an area for improvement, there is very little impact on the usefulness of Umami due to this issue.

Note whilst adding a second sort criteria will make the sort more consistent, it won't guarantee consistency depending on how content is split over seconds, so I'd be minded to add creation dates to the article import process.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Eli-T’s picture

Title: Inconsistent ordering on import. » Inconsistent ordering of views of content in Umami
Issue summary: View changes
Issue tags: +Out of the Box Initiative, +Drupal Europe

Adding further sort criteria actually will not make the order reliably consistent, so removing it as a potential solution.

If we added a date to each node, we would need to decide whether to use absolute dates or an offset. It would be better to use an offset so that when installing Umami it looks like all the content was created recently.

cferthorney’s picture

Status: Active » Needs review
FileSize
18.12 KB

Here's an initial patch for this, inline with a discussion I had with @eli-t at DrupalEurope.

Status: Needs review » Needs work

The last submitted patch, 5: inconsistent-dates-2983665-5.patch, failed testing. View results

Eli-T’s picture

  1. +++ b/core/profiles/demo_umami/modules/demo_umami_content/src/InstallHelper.php
    @@ -168,6 +168,14 @@ protected function importArticles() {
    +        // Set the create and modified date offset.
    

    We're not actually setting the offset here - we're setting those dates.
    It would be nice to clarify this slightly what's going on here and why. Maybe explain that we make sure the nodes are saved at intervals rather than just the install time so that the order when sorted by time is predictable.

  2. +        if (!empty($data['date_offset'])) {
    		+          $date = new \DateTime();
    		+          $interval = new \DateInterval('P' . $data['date_offset'] . 'D');
    		+          $offset_date = $date->sub($interval);
    		+          $values['created'] = $offset_date->format('U');
    		+          $values['changed'] = $offset_date->format('U');
    		+        }
    

    This logic is duped at line 280 as well. Can we put this in a method on this class?

  3. DefaultContentDateOffsetTest.php might need running through phpcs. Also all the implemented methods are extremely similar, so could benefit from a helper method.
  4. +++ b/core/profiles/demo_umami/modules/demo_umami_content/src/InstallHelper.php
    @@ -168,6 +168,14 @@ protected function importArticles() {
    +        }
    
    +++ b/core/profiles/demo_umami/modules/demo_umami_content/tests/src/Functional/DefaultContentDateOffsetTest.php
    @@ -0,0 +1,95 @@
    +    // The umami guide to our favorite mushrooms
    
  5. I'm not sure why this comment is here.
  6. +++ b/core/profiles/demo_umami/modules/demo_umami_content/tests/src/Functional/DefaultContentDateOffsetTest.php
    @@ -0,0 +1,95 @@
    +    // Test 0 offset
    

    This is testing a 3 day offset.

cferthorney’s picture

Status: Needs work » Needs review
FileSize
18.78 KB

@eli-t I've updated the patch with your suggestions. With regards the helper function, I am not sure if we'd need 1 or 2? 1 for formatting the offset \DateTime object, and 1 for the node's created date?

edited for clarity.

Status: Needs review » Needs work

The last submitted patch, 8: inconsistent-dates-2983665-8.patch, failed testing. View results

cferthorney’s picture

Status: Needs work » Needs review
FileSize
18.83 KB

Further test fixes, and coding standard changes

cferthorney’s picture

Documentation refinements, and creation of a formatDate() helper function as discussed at DrupalEurope.

cferthorney’s picture

Removing an unused namespace.

longwave’s picture

+++ b/core/profiles/demo_umami/modules/demo_umami_content/tests/src/Functional/DefaultContentDateOffsetTest.php
@@ -0,0 +1,104 @@
+ * Known failure if content is installed one side of midnight and the test is
+ * run the other.

Can we commit with this known fragile test? How long is the window? Will this trigger a handful of random fails when patches are uploaded between 11pm and midnight?

cferthorney’s picture

I have just discussed this with @Eli-T at Drupal Europe, and we think it is OK as the window is actually quite small. The problem will occur if the test server crosses midnight between install time (When the node is created) and the time the Umami test suite runs, which is likely to be a few seconds at most.

cferthorney’s picture

Status: Needs review » Needs work

We have also now discussed this with @xjm. This could actually produce a critical elsewhere and could be hugely disruptive. We are going to look at how DateTime handles these kinds of issues, or remove the test. Reverting to "Needs work".

cferthorney’s picture

Status: Needs work » Needs review
FileSize
15.48 KB

Removing tests.

Eli-T’s picture

Status: Needs review » Needs work

The consequences of what the tests for this issue test being broken do not justify introducing tests that will either be fragile or too vague to be reliable. Therefore I support removal of the tests for this.

+++ b/core/profiles/demo_umami/modules/demo_umami_content/src/InstallHelper.php
@@ -527,4 +543,23 @@ protected function fileUnmanagedCopy($path) {
+  protected function getOffsetDate($daysToOffset) {

This function returns string, or FALSE when no $daysToOffset is passed.

When it is called, we aren't checking for a return value of FALSE, so would set the created and changed value to FALSE.

Suggest if $daysToOffset is not set, we treat that as an offset of 0, effectively returning a date string representing the current time.

This gives a consistent return type for getOffsetDate(), and reasonable fallback handling.

cferthorney’s picture

Status: Needs work » Needs review
FileSize
19.31 KB

Refactored inline with #17.

longwave’s picture

Instead of explicitly checking the dates can we just add a test that confirms the order of the items is as expected?

cferthorney’s picture

Reworking of #18 to account for new article as part of commit 089f391. Also remove incorrectly inherited patches found in 18.

cferthorney’s picture

Status: Needs review » Needs work

@longwave - I've discussed this approach with @eli-t and I'll work on it as a test, hopefully the test won't be fragile then.

cferthorney’s picture

Readded the test and ensured the CSV was correctly line ended etc

cferthorney’s picture

Status: Needs work » Needs review

Marking needs review

longwave’s picture

+++ b/core/profiles/demo_umami/modules/demo_umami_content/tests/src/Functional/DefaultContentDateOffsetTest.php
@@ -0,0 +1,74 @@
+ * Known failure if content is installed one side of midnight and the test is
+ * run the other.

This can be removed now.

cferthorney’s picture

Fixes the documentation

Eli-T’s picture

Issue tags: +Needs reroll
savkaviktor16@gmail.com’s picture

Status: Needs review » Needs work

The last submitted patch, 27: inconsistent-dates-2983665-27.patch, failed testing. View results

Eli-T’s picture

Status: Needs work » Reviewed & tested by the community

Patch looks good. All concerns raised in the issue have been addressed. Tested on SimplyTest.me.

Therefore marking RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 27: inconsistent-dates-2983665-27.patch, failed testing. View results

zuhair_ak’s picture

Status: Needs work » Needs review
FileSize
18.52 KB

Running the patch again to see if the tests fail again

longwave’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 31: inconsistent-dates-2983665-31.patch, failed testing. View results

zuhair_ak’s picture

Status: Needs work » Needs review
FileSize
16.09 KB

One more time

longwave’s picture

Status: Needs review » Reviewed & tested by the community

You don't need to reupload the patch when the bot reports a false failure; use the "Add test / retest" link below the failed patch to force another test run.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Discussed with @Eli-T who pointed out that adding nid to the sort in the views would achieve the same result - ie. the articles and recipes would be in the order they are listed in the csv. The other benefits are:

  • less code to maintain
  • easier to move to migrate_csv if that makes it into core.
Eli-T’s picture

A further benefit in addition to those pointed out by @alexpott in #36 is that it is bad practice to sort purely by date as this can lead to unstable sort orders in data returned from the database. By adding nid to the sort order, we avoid that and demonstrate better practice in the example views provided by the Umami demo.

cferthorney’s picture

In line with the discussion from #36/#37, this is a new patch stripping out everything from the previous patches. This is why there is no `interdiff.txt` as it would be very difficult to read and understand. This new patch adds the NID sort to the front page view as discussed here and in the Out of the box Slack channel.

Eli-T’s picture

Status: Needs review » Needs work

Thanks for your continued work on this @cferthorney - this patch looks really good.

In reviewing it, I've noticed we also need to add the same change to the articles_aside view. Then I think we're good to go.

longwave’s picture

+1 for this much simpler approach

cferthorney’s picture

@eli-t - would it be worth adding the `nid` to the articles and recipes views themselves too? Or should that be done as a separate issue?

cferthorney’s picture

Status: Needs work » Needs review
FileSize
3.36 KB
715 bytes

Updated to include `articles_aside` as suggested in #39

Eli-T’s picture

Just to clarify wrt #41, the views mentioned by @cferthorney are already amended to include nid in the sort order in the patch in #38

Eli-T’s picture

Status: Needs review » Reviewed & tested by the community

Have manually reviewed the patch in #42 - looks good.

Have verified the views with the following machine names have ID in the sort order

  • recipes
  • frontpage
  • articles_aside
  • featured_articles

Have spun up 5 instances of Umami on D8.7.x with the patch and verified all content in the same order on the following paths:

  • /articles/give-it-a-go-and-grow-your-own-herbs
  • /
  • /recipes
  • /articles

Therefore marking RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 374bb899a4 to 8.7.x and bae36ffc3e to 8.6.x. Thanks!

Backported to 8.6.x as this only affects configuration before install and this is not API.

  • alexpott committed 374bb89 on 8.7.x
    Issue #2983665 by cferthorney, zuhair_ak, Saviktor, John Cook, Eli-T,...

  • alexpott committed bae36ff on 8.6.x
    Issue #2983665 by cferthorney, zuhair_ak, Saviktor, John Cook, Eli-T,...

Status: Fixed » Closed (fixed)

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