SPIKE Update UI to account for a more nuance understanding of unpublished

maxime-rainville commented 1 year ago

As a content author, I want to see the state of a DataObject and all its relations at a glance so I can know if I need to publish them.

Acceptance criteria

"Stage label" on the Gridfield accounts for the record's owned relations.
Initial "Publish" button on gridfield accounts for the record's owned relations
SiteTree label accounts for the record's owned relations
"Publish" button on CMSMain accounts for the record's owned relations
"Cancel draft changes" button state looks up the recursive stage differ.
Those changes do not lead to substantial performance regression even on large record list.
- If there is a substantial performance regression, the new behaviour can be opt-in for the SiteTree and GridField List
Projects can still use Version Snap Shot if they want to even with this change merged in.

Excluded

Campaign admin publication labels
Asset admin publication labels

Parent EPIC

https://github.com/silverstripe/silverstripe-versioned/issues/419

PRs

sabina-talipova commented 10 months ago

While working on this task, I came across several questions regarding the behavior and use of badges. if the parent object does not have the Versioned extension, but the child objects have this extension:

What should the badge be if several child objects have different states. (e.g. "Draft" and "Archived"). Probably, we need some additional badge type for changes in sub objects.
"Save" button is currently highlighted and if the user clicks on it, all child objects will be recursively published. But if the user leaves the page without clicking on the “Save” button, he receives a warning about unsaved data. This situation seems to force the user to publish the changes, but perhaps he does not currently plan to publish some of the child objects.
This moment is more of a clarification: At the moment, if the user clicks on the “cancel draft changes” button, then all changes in the object itself and its child objects are recursively canceled. Is this correct and expected behaviour? I think this requires further discussion.

emteknetnz commented 10 months ago

@sabina-talipova

I've done an initial peer review on the PRs only looking at code changes, I haven't tested locally. I've also bumped the size on this card from medium to large

Did you chat with anyone regarding https://github.com/silverstripe/silverstripe-admin/issues/1593#issuecomment-1867078306? Or do we still need to have a discussion about these questions?

There's a couple of large AC's that need to be looked at too

Those changes do not lead to substantial performance regression even on large record list.

Can you provide some benchmarks for the performance difference of including this. Using response times in browser developer tools and include the average of 3 runs. Test should cover all of the components listed in the ACs. As well as performance numbers, could you please also include the PHP files used to perform the benchmark

Projects can still use Version Snap Shot if they want to even with this change merged in.

You'll need to confirm this as well. If it doesn't work then possibly this is OK since versioned snapshots includes some functionality that does the same thing as this PR. Versioned snapshot is here. Recommend you do this indenpendently of your performance testing because there's going to be a bunch of things to setup.

sabina-talipova commented 9 months ago

Performance tests:

DB: MySQL 5.7 Browser: Chrome

Relations: Page (1) has_many -> Company (5 / overall 5) has_many -> Department (5 / overall 25) has_many -> Employee (50 / overall 1 250).

Process	Time Without PR (sec)	Time With PR (sec)
Load page with ~~1250~~ 1280 related objects	~ 0.6 - 1.5	~ 0.5 - 9.0
Load Company record with 250 related objects	~ 0.5 - 1.2	~ 0.9 - 3.0
Load Department record with 50 related objects	~ 0.7	~ 0.7 - 0.9
Load Employee record	~ 0.7	~ 0.7 - 0.9
Load page with ~~1 250~~ 1280 related objects if there is any Modification in related objects	~ 0.6	~ 6 - 11
Load Modified Company record with 250 related objects	~ 1	~ 2.5
Load Company record with 250 related objects with at least one modified related object	~ 0.7	~ 2.5
Load Department record record with 50 related objects with at least one modified related object	~ 0.8	~ 0.9

Relations: Page (1) has_many -> Company (10 / overall 10) has_many -> Department (10 / overall 100) has_many -> Employee (100 / overall 10 000).

Process	Time Without PR (sec)	Time With PR (sec)
Load page with ~~10 000~~ 10 110 related objects	~ 0.6 - 1.5	~ 11 - 13
Load Company record with 1 000 related objects	~ 0.6 - 1.5	~ 11 - 13

Steps to reproduce tests:

Copy files from https://github.com/sabina-talipova/recipe-kitchen-sink/tree/test-versioned-performance/app to the app directory in the local project. frameworktest module should be installed.
See instruction how to create records here

Update core public / index.php to set timer with the following code:

$before = microtime(true);
// main code here ...
$after = microtime(true);
echo ($after-$before) . " sec";

Open CMS and test new created Pages and objects.

GuySartorelli commented 9 months ago

That performance looks really bad - we might want to drop this. At most if we do implement it, it's got to be opt-in with that sort of performance loss.

sabina-talipova commented 9 months ago

This PRs works relatively quickly with multiple relationships when the main parent is another DateObject (not Page). On average 0.5 - 0.7 sec to load record. The main problem is that changes can exist at the very last level and at the very last object. Then it takes a significant amount of time to find changes. There are still some questions here that I would like to clarify and discuss with one of the designers. See. @emteknetnz, https://github.com/silverstripe/silverstripe-versioned-snapshots doesn't support CMS 5 and these changes for CMS 5.2.

emteknetnz commented 9 months ago

Good investigation

Yeah seems like we should not merge this just now given potential performance regressions on every page edit load just to make more accurate save/publish buttons. I always amazed to hear just how bulky of the larger and older websites get, so 1250 records probably isn't outside the realms of possibilities

I'm thinking we should retroactively call this one a SPIKE and I'll move this card back to the backlog and discuss in a couple of weeks during a refinement session what we want to do with this.

GuySartorelli commented 9 months ago

That sounds like a good idea. My current thinking is we'd probably want to provide some way for projects to have this behaviour - but that doesn't necessarily have to be an opt-in feature flag, it could just mean enabling developers to implement this themselves via extensions etc.

maxime-rainville commented 8 months ago

I've switched this to the 5.3 milestone.

Two random thoughts:

Could we get around the performance issue with caching? Basically, have a background process that detects when a children of something needs to be published and remembers that this is needed until the parent is actually published.
Could we detect the need for publication bottom-up instead? Basically, when a child DataObject gets a new draft version, notify the parent that you probably should be in a dirty state now.

GuySartorelli commented 6 months ago

A wise person has just pointed out to me that we do have an abstraction for recursive database queries now (see CTE) which could be used to improve the performance somewhat. We'd probably have to provide a suitable fallback logic (most likely just don't perform a recursive check) for DB drivers that don't support recursive CTEs. Or hold off till CMS 6.

silverstripe / silverstripe-admin