Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#42 | interdiff.txt | 715 bytes | cferthorney |
#42 | inconsistent-ordering-front-page-2983665-42.patch | 3.36 KB | cferthorney |
#38 | inconsistent-ordering-front-page-2983665-38.patch | 2.5 KB | cferthorney |
#34 | inconsistent-dates-2983665-34.patch | 16.09 KB | zuhair_ak |
#31 | inconsistent-dates-2983665-31.patch | 18.52 KB | zuhair_ak |
Comments
Comment #2
Eli-TSetting 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.
Comment #4
Eli-TAdding 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.
Comment #5
cferthorneyHere's an initial patch for this, inline with a discussion I had with @eli-t at DrupalEurope.
Comment #7
Eli-TWe'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.
This logic is duped at line 280 as well. Can we put this in a method on this class?
This is testing a 3 day offset.
Comment #8
cferthorney@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.
Comment #10
cferthorneyFurther test fixes, and coding standard changes
Comment #11
cferthorneyDocumentation refinements, and creation of a
formatDate()
helper function as discussed at DrupalEurope.Comment #12
cferthorneyRemoving an unused namespace.
Comment #13
longwaveCan 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?
Comment #14
cferthorneyI 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.
Comment #15
cferthorneyWe 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".
Comment #16
cferthorneyRemoving tests.
Comment #17
Eli-TThe 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.
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.
Comment #18
cferthorneyRefactored inline with #17.
Comment #19
longwaveInstead of explicitly checking the dates can we just add a test that confirms the order of the items is as expected?
Comment #20
cferthorneyReworking of #18 to account for new article as part of commit 089f391. Also remove incorrectly inherited patches found in 18.
Comment #21
cferthorney@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.
Comment #22
cferthorneyReadded the test and ensured the CSV was correctly line ended etc
Comment #23
cferthorneyMarking needs review
Comment #24
longwaveThis can be removed now.
Comment #25
cferthorneyFixes the documentation
Comment #26
Eli-TComment #27
savkaviktor16@gmail.com CreditAttribution: savkaviktor16@gmail.com at Skilld commentedRe-rolled
Comment #29
Eli-TPatch looks good. All concerns raised in the issue have been addressed. Tested on SimplyTest.me.
Therefore marking RTBC.
Comment #31
zuhair_akRunning the patch again to see if the tests fail again
Comment #32
longwaveBack to RTBC.
Comment #34
zuhair_akOne more time
Comment #35
longwaveYou 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.
Comment #36
alexpottDiscussed 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:
Comment #37
Eli-TA 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.
Comment #38
cferthorneyIn 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.
Comment #39
Eli-TThanks 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.
Comment #40
longwave+1 for this much simpler approach
Comment #41
cferthorney@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?
Comment #42
cferthorneyUpdated to include `articles_aside` as suggested in #39
Comment #43
Eli-TJust to clarify wrt #41, the views mentioned by @cferthorney are already amended to include nid in the sort order in the patch in #38
Comment #44
Eli-THave manually reviewed the patch in #42 - looks good.
Have verified the views with the following machine names have ID in the sort order
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:
Therefore marking RTBC.
Comment #45
alexpottCommitted 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.