Action trigger feature #25

mfendeksilverstripe · 2019-11-26T22:07:36Z

Action trigger workflow feature

New workflow added which is based on user actions as opposed to model writes.

Features

out of the box action handling of basic user actions (page edit form, grid field item edit form)
static configuration to alter existing actions and adding new ones
runtime overrides which allow altering how snapshots are created
snapshots are created only after all model writes are done so snapshot rollback is consistent
detailed documentation is in readme

unclecheese

Awesome work. As I stated in the demo, I really think this puts the module much closer to where it needs to be in order to become a viable long term solution to versioning headaches. I have two primary concerns, other than what's detailed in the comments:

Documentation -- I know you wrote a ton of new content, which is great, but I feel like it still requires a lot of context to understand what's going on. I had to read it three times to wrap my head around it, and I'm more familiar with the use cases than most will be. So I think just backing up and thinking really high level about what problems this solves, and working by example is really key. In fact, I would probably prefer that you worked off an imaginary project to show the real business case for it and to connect all the examples into one coherent theme.
Coupling -- I'm sure this wasn't your preferred architecture, but it does seem like what used to be a fairly self-contained module is now has all kinds of awareness about different CMS features. You've added a bunch of public APIs that are named specifically after external features, and things like GraphQL Schema are now imported into Snapshot. It may very well have to be that way just because our framework isn't very well decoupled to begin with, but I'd just share that if I were building this from the ground up, the way I would envision that working is with something like a registry that accepts any number of classes of a certain interface that can declare their identifiers. So something like:

Injector:
  Snapshot:
    properties:
      listenerRegistry:
         graphql: %My\GraphQL\Listener

Then, your implementation is $snapshot->fireAction('graphql', array $params), which fails gracefully if nothing in the registry matches graphql, and you destructure the argument signature of those methods so it just takes arbitrary params -- more like an event system. You lose the typing, but I think that's a fair trade off for the value of decoupling.

Does that make sense? Right now it feels like you're really overloading the purpose of extensions. Like I said, you've probably got good reasons for that, but just a gut reaction tells me there's a cleaner approach somewhere.

unclecheese · 2019-12-16T03:13:30Z

README.md

+'`save`': '`Save page`'
+
+This means each time a user saves a page via page edit form a snapshot will be created with a context message `Save page`.
+


This still isn't clear to me what save is. I think I need to see the code that is providing this identifier back to the config. I assume that's something like:

class CMSPageSaveAction ... { public function getActionName(): string { return 'save' } }

???

I think it's important here to show why a snapshot is created whenever someone saves a page, so it doesn't look so much like magic.

unclecheese · 2019-12-16T03:14:47Z

README.md

+
+'`save`': '`Save page`'
+
+This means each time a user saves a page via page edit form a snapshot will be created with a context message `Save page`.


Can you define "context message" here, or just show what the end result looks like in the IUI?

unclecheese · 2019-12-16T03:18:16Z

README.md

+
+`CustomActionListenerAbstract` is the parent class because this action is a custom mutation
+
+Returning `false` inside `processAction` makes the module fallback to default behaviour.


What is "default" behaviour at this point?

unclecheese · 2019-12-16T03:19:23Z

README.md

+
+There are two main cases here:
+
+`1` - snapshot module doesn't have enough information to find origin and is using `event` as a placeholder to fill the gap.


An example of this would be really helpful, as you've done below.

unclecheese · 2019-12-16T03:22:07Z

src/Listener/CurrentPage.php

+     * @param mixed $controller
+     * @return Page|null
+     */
+    private function getCurrentPageFromController($controller): ?Page


I feel like I've written this code so many times. Would be worth considering if it should be a core API. Nothing in here is unique to the concerns of snapshots.

unclecheese · 2019-12-16T03:31:48Z

src/Listener/GraphQL/CustomAction.php

+     */
+    private function getActionType(string $query): ?string
+    {
+        $action = explode('(', $query);


Does this assume the query has arguments?

unclecheese · 2019-12-16T03:35:57Z

src/Listener/GraphQL/GenericAction.php

+        $url = $request->getHeader('referer');
+        $url = parse_url($url, PHP_URL_PATH);
+        $url = ltrim($url, '/');
+        $page = $this->getCurrentPageFromRequestUrl($url);


Second time we've done this. Does it belong in a parent class, or maybe in your trait?

unclecheese · 2019-12-16T03:39:41Z

src/Listener/GraphQL/GenericAction.php

+
+        if ($owner instanceof Create) {
+            return static::TYPE_CREATE;
+        }


This feels like really awkward coupling between the extension and its owner. It feels like something that should be the concern of the CRUD operation. Since it's static anyway, what if this was just return $owner->config()->snapshot_action_type, and this class defined a default value in private static $snapshot_action_type to ensure the property always existed?

unclecheese · 2019-12-16T03:45:26Z

src/Snapshot.php

+        }
+
+        return $actions[$identifier];
+    }


If this is singleton access only, should it just be static? There's nothing in here that requires an instance.

unclecheese · 2019-12-16T03:46:27Z

src/Snapshot.php

+                // no need to add the origin to the list of objects as it's already there
+                $origin = $owner;
+            }
+        } elseif ($origin->ClassName === $owner->ClassName && (int) $origin->ID === (int) $owner->ID) {


I think you want to compare hashes with SnapshotHasher here.

unclecheese · 2019-12-16T22:29:29Z

_config/config.yml

+    'graphql_crud_create': 'GraphQL create'
+    'graphql_crud_delete': 'GraphQL delete'
+    'graphql_crud_update': 'GraphQL update'


Do you have plans on how to make these i18n compatible?

mfendeksilverstripe · 2020-01-06T23:50:29Z

_config/config.yml

+---
+SilverStripe\Snapshots\Snapshot:
+  actions:


It's worth considering grouping the actions by listeners. Action names are probably not unique in general.

For example:
Page\CMSMainAction - save
Form\Submission - save

Should the configuration be moved to individual listeners then?

…ave-snapshots SS-8748: Avoid recording useless save actions

Some snapshot items can be information-only so don't require a publish at the owner. These items will be excluded from status flag checks

The lister mode was not marking items as WasPublished. This meant the status flags were always showing as 'modified'

…cation-snapshots Bugfix/no modification snapshots

This was referenced Nov 27, 2019

CMS action related extension points silverstripe/silverstripe-framework#9340

Merged

Operation extension points silverstripe/silverstripe-graphql#243

Merged

Snapshot message silverstripe/silverstripe-versioned-snapshot-admin#24

Closed

mfendeksilverstripe force-pushed the feature/rework branch from 8205243 to bde6769 Compare November 28, 2019 21:14

Action based snapshot work flow

ce82d2b

mfendeksilverstripe force-pushed the feature/rework branch from bde6769 to ce82d2b Compare November 28, 2019 22:14

unclecheese requested changes Dec 16, 2019

View reviewed changes

unclecheese reviewed Dec 16, 2019

View reviewed changes

mfendeksilverstripe commented Jan 6, 2020

View reviewed changes

blueo and others added 5 commits January 8, 2020 09:09

SS-8748: Avoid recording useless save actions

d2b8e02

Merge pull request #1 from silverstripe-terraformers/bugfix/useless-s…

b94d4f8

…ave-snapshots SS-8748: Avoid recording useless save actions

Allow snapshot items to be hidden

245bc30

Some snapshot items can be information-only so don't require a publish at the owner. These items will be excluded from status flag checks

Mark publish form submissions as WasPublished

e2d2ebd

The lister mode was not marking items as WasPublished. This meant the status flags were always showing as 'modified'

Merge pull request #2 from silverstripe-terraformers/bugfix/no-modifi…

2f2988c

…cation-snapshots Bugfix/no modification snapshots

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Action trigger feature #25

Action trigger feature #25

mfendeksilverstripe commented Nov 26, 2019 •

edited

Loading

unclecheese left a comment

unclecheese Dec 16, 2019

unclecheese Dec 16, 2019

unclecheese Dec 16, 2019

unclecheese Dec 16, 2019

unclecheese Dec 16, 2019

unclecheese Dec 16, 2019

unclecheese Dec 16, 2019

unclecheese Dec 16, 2019

unclecheese Dec 16, 2019

unclecheese Dec 16, 2019

unclecheese Dec 16, 2019

mfendeksilverstripe Jan 6, 2020 •

edited

Loading

		'`save`': '`Save page`'

		This means each time a user saves a page via page edit form a snapshot will be created with a context message `Save page`.


		`CustomActionListenerAbstract` is the parent class because this action is a custom mutation

		Returning `false` inside `processAction` makes the module fallback to default behaviour.


		There are two main cases here:

		`1` - snapshot module doesn't have enough information to find origin and is using `event` as a placeholder to fill the gap.

Action trigger feature #25

Are you sure you want to change the base?

Action trigger feature #25

Conversation

mfendeksilverstripe commented Nov 26, 2019 • edited Loading

Action trigger workflow feature

Features

unclecheese left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfendeksilverstripe Jan 6, 2020 • edited Loading

Choose a reason for hiding this comment

mfendeksilverstripe commented Nov 26, 2019 •

edited

Loading

mfendeksilverstripe Jan 6, 2020 •

edited

Loading