V8 - Add Media Tracking of items added via Macro Parameters in RTE + Grid - issue #8131

[marcemarc]

Prerequisites

• [ Y] I have added steps to test this contribution in the description below

If there's an existing issue for this PR then this fixes #8131

Description

Currently, the new Media Tracking functionality that tracks when a Media item is in use on a particular page, and therefore 'whether it can be safely removed' does not take into consideration Media that might have been picked in a Macro Parameter and inserted into the Rich Text Editor or Grid.

This is quite a common scenario where developers may use a Macro to create an insertable Image Gallery / Responsive Image + Caption or neatly formatted List of PDF files.

It is the responsibility of the property editor to 'get references' that include Udis, but the 'Macro Picker' in the grid and RTE isn't really a property editor in its own right, and can have in-turn Macro Parameters with different property editors as the source, in theory, when a Macro is inserted in we should loop through the Macro Parameters and call the GetReferences implementation for each type of Parameter... however this presents a problem in that from the embedded markup of the macro in the rich text area or JSON of the grid we cannot easily infer the type of parameter without further 'look ups', which in this context might be expensive?

But pragmatically we 'DO' have the Macro Parameter Values, and the Single Media Picker Macro Parameter stores Udis, and so if a Macro Parameter Value contains a Media Udi, or a CSV of Media Udis, then this is all we need for Media Tracking purposes... (it doesn't matter how it is picked)

This PR adds a new HtmlMacroParameterParser, that is injected into RichTextPropertyEditor + GridPropertyEditor - and used in their GetReferences methods to parse either the HTML (or the GridControls) for Macros, and their Macro Parameter Values.

If the Macro Parameter Values are in media Udi format, they are then returned as references for the page, and the Media item is 'tracked' as being used on the page.

Have added some 'tests' around some of the edge cases.

This will work for any Macro Parameter Editor that stores it's values as Udis or a CSV of Udis...

... fun fact... the Multiple Media Picker Macro Parameter, although it uses the same implementation as the Single Media Picker Macro Parameter - still stores it's picked items as integer ids - have created a separate PR here to address that: #8388

How to test this PR?

In the starter kit, create a Macro called Insert Image, that has a Macro Parameter of type Single Media Picker - and implement a Macro Partial View to write out the image, and allow this macro to be used in Rich Text Area and Grid.

Now in a Rich Text Area on a page, insert the Macro, and pick an image as the Macro Parameter.
Save and Publish
View the Media Item in the backoffice, and you should see this is now 'tracked' as being in use on this page.
In a different page that uses the grid, insert the Macro again, and pick the same or different image, and publish the page, visit the media item and you should find that is also being 'tracked' as being used on the grid page.

Thanks for considering this contribution to Umbraco CMS!

[Shazwazza]

Have left a comment inline. We should be using property editors to resolve references always. But perhaps I'm overlooking something, let me know.

[Shazwazza]

Yes but eventually we want to track all sorts of things... basically anything that can have a UDI reference. Could be content -> content, etc... Macros do have a property editor, they are the same. They are just property editors flagged with also being a macro parameter editor. This is exposed by IDataEditor.EditorType enum. But you can 'just' resolve all macro parameter editors from the collection ParameterEditorCollection. Don't think that would be expensive get the editor based on the editor alias ... if you have it. IIRC that is stored as part of the embedded macro parameters?

[marcemarc]

What I'm trying to say is that the Grid, has a property editor - the class is called GridPropertyEditor, it is responsible for the references to udis in the Grid - The Rich Text Editor has a property editor - the class is called RichTextPropertyEditor - but the Macro isn't a 'property editor' in the 'same way' it doesn't have a MacroPropertyEditor class (or maybe I can't find it! :-)) so it doesn't have it's own GetReferences implementation (which following the pattern would 'call' all the 'GetReferences' for each of the Macro Parameters, which of course are tied to their own PropertyEditor implementations...

Maybe we should have a 'Macro Picker' property editor that people can use for Doc Type properties? (like the old Macro Container)

So what I'm trying to say, is a Macro is a bit different, as it isn't an editor in its own right - and I'm not sure how to make it follow the same pattern of responsibility (a property editor is responsible for finding the references!) - which is why I created the HtmlMacroParameterParser to take on this task...

Having created that, and with the warnings about performance:

it occurred to me that pragmatically having the values in udi format means you 'know' what type they are - even if you want to add content item tracking at a later date - it will have a udi//document format ... you will know it's type without needing to know how it was picked... (the argument against this approach is what about complex custom macro property editors that store Json... but ignore that for now)

... but I'm aware this doesn't follow the pattern and the notion of property editor responsibility (which is good, but I don't think Macros follow this pattern which is the problem I'm encountering or misunderstanding).

The embedded Macro markup looks like this:

<?UMBRACO_MACRO macroAlias=""InsertImageCarousel"" carouselImages=""umb://media/eee91c05b2e84031a056dcd7f28eff89,umb://media/fa763e0d0ceb408c8720365d57e06e32,umb://media/bb763e0d0ceb408c8720365d57e06444"" />

and in the case of the grid:

{ ""macroAlias"": ""InsertImageCarousel"", ""macroParamsDictionary"": { ""carouselImages"": ""umb://media/eee91c05b2e84031a056dcd7f28eff89,umb://media/fa763e0d0ceb408c8720365d57e06e32,umb://media/bb763e0d0ceb408c8720365d57e06444"" }}

So you DO not have the alias of the property editor stored as part of the embedded macro parameters - just the alias of the Macro, and the name given by the developer to the parameter...

... so 'including' the property editor alias in the embedded markup would be another way of making this much easier!! :-)

but in the absence of this, my thinking here is we'd need to lookup the configured Macro, by Macro Alias, read the parameter configuration, establish the editor type for each parameter, get the editor from the ParameterEditorCollection, call it's GetReferences method - passing in the value of that parameter from the embedded macro...

... I don't think the ParameterEditorCollection look up is the expensive bit... it's the querying to get the Macro configuration? which will happen for every macro - even when it doesn't have a media picker, just to find out, it doesn't have a media picker - unless there is a better way?

whereas the pragmatic thing here is we know from the Macro Parameter Value what 'type' of udi it is, and it is all we need to add the tracking! Macros are special!

But can rework the PR to try and lookup the macro, establish parameter editor type, and call getreferences if you confirm that is 'just' the preferred approach for the longer term, but if there is a better way to do the lookup?

[marcemarc]

This is the gist of what it would be looking up the macro each time:

 private IEnumerable<UmbracoEntityReference> GetMacroParameterUmbracoEntityReferences(string macroAlias, Dictionary<string,string> macroParameters)
        { 
                //lookup macro configuration to find type of parameter editor
                // could we store/cache this somewhere
                // is there a better way of getting this info?
                IMacro macroConfig = _macroService.GetByAlias(macroAlias);
                foreach (var parameter in macroConfig.Properties)
                {
                    var parameterValue = macroParameters[parameter.Alias];
                    var parameterEditorAlias = parameter.EditorAlias;
                    //lookup propertyEditor from core current ParameterEditorCollection
                    var parameterEditor = _parameterEditors.FirstOrDefault(f => f.Alias == parameterEditorAlias);
                    if (parameterEditor != null)
                    {
                        //get the ParameterValueEditor for this PropertyEditor (where the GetReferences method is implemented) - cast As IDataValueReference to determine if it is implemented
                        IDataValueReference parameterValueEditor = parameterEditor.GetValueEditor() as IDataValueReference;
                        if (parameterValueEditor != null)
                        {
                            foreach (var entityReference in parameterValueEditor.GetReferences(parameterValue))
                            {
                                yield return entityReference;
                            }                            
                        }
                    }
                }
            }

it's certainly neater, but does that seem ok, with using the MacroService?

I can update along these lines if it make sense (and I can work out how to inject the ParameterEditorCollection properly).

[Shazwazza]

Ah yes the grid is it's own beast as well since 'grid editors' aren't property editors either. I think there's probably another issue with the grid, references and macros too since the GridPropertyEditor.GetReferences is not taking into account the macro picker grid editor either.

I do much prefer your 2nd approach above using the macro service. We are most likely going to have to do this in a few places moving forward like for the grid + rte, the RTE itself (there's already a TODO there: //TODO: Detect Macros too ... but we can save that for a later date, right now need to do media refs) , nested content + rte, block editor + rte (perhaps the block editor has a macro picker too, can't remember) so doing this the right way and having a central bit of code to do this all will be good.

Regarding performance, yes that will cause quite a few queries to execute since we are doing N+1 all over the place. But we can deal with this, there's a couple options and perhaps they are both things we might want to do:

1. Adding a macro service method IEnumerable<IMacro> GetAll(params string[] aliases) which will query for all macros by those aliases. This wouldn't require any changes to the macro repository because we can construct a query to pass to the repo for that. Then instead of doing N+1 queries, we can just get all macros by all aliases at once. This would greatly reduce the queries but still means one query to this new macro service per RTE and macro editor. I think that would still be ok though.
2. Add caching by alias to the macro repository and change the macro service to call this new repository method. I just did a similar thing to member's by username in this branch, see the diff here v8/8.6...v8/bugfix/8433-member-login-sql-locks , specifically this is how to do custom caching v8/8.6...v8/bugfix/8433-member-login-sql-locks#diff-713b853430e7cca3f31d799ccdcc9671R689 with a custom cache policy but that also means we need to update the macro cache refresher too :) ... I'd be happy to help out or point you in the right direction for this but there's a lot of examples in that branch

1 ) above is pretty easy. I'd start with that. 2 ) is probably good to do anyways because we use the GetByAlias call in quite a few places in the core and we already have hacked in a cache for it inside of MacroRenderer.Render and it's definitely less than ideal to have this hacked cache in there when it should definitely be part of the macro repository.

[marcemarc]

Great, thanks @Shazwazza for the direction, I'll rework the PR to take this approach! I was just wary of the n+1, but as you say, there are ways around reducing the impact of that - and this is much neater, the responsibility of finding the Udis is with the property editor implementation which feels right.

The PR also handled the Grid, and the Macro Grid Controls and their parameters, which were parsed in the same 'pragmatic' way, this can remain but instead call this new common method to do the entity reference finding.

Let's see how far I get!

[marcemarc]

@Shazwazza ok... this is as far as I got :-)!

It works!

It's refactored with the neater approach, of obtaining the Macro Configuration and using that to determine the Macro Parameter PropertyEditor types, to then just call each parameter editor's GetReferences if it's implemented.

I've added the GetAll endpoint to the MacroService, and added methods to the MacroRepository to use a custom cache policy, to cache by alias + teamed with an update to the MacroCacheRefresher to break the cache when a particular Macro is saved.

It's working for Macros inserted into the Rich Text Editor and as a Macro Grid Control.

But basically I think it's ready for a review, and more generally are all the bits in the right place and called the right things? and have I done anything terrible? (I am sorry in advance)

Where I got stuck, was how best to get a reference to the 'ParameterEditors' collection?

// can we inject this instead of using Current??
            var parameterEditors = Current.ParameterEditors;
            foreach (var macro in macros)
            {

(I know this isn't right, but injecting into the parser itself...

        private readonly ParameterEditorCollection _parameterEditors;
        public HtmlMacroParameterParser(IMacroService macroService, ILogger logger, ParameterEditorCollection parameterEditorCollection)
        {         
            _parameterEditors = parameterEditorCollection;

Gave an Unresolved dependency error:

-> System.InvalidOperationException: Unresolved dependency [Target Type: Umbraco.Web.Templates.HtmlMacroParameterParser], [Parameter: parameterEditorCollection(Umbraco.Core.PropertyEditors.ParameterEditorCollection)], [Requested dependency: ServiceType:Umbraco.Core.PropertyEditors.ParameterEditorCollection, ServiceName:]

So it would be great to get an insight into what I'm doing wrong there!

In terms of tracking Macro usages, it's kind of inadvertently doing this, as we are getting all the configs of all the Macros on the page in order to get their parameters, so I am adding these Macro Udis to a separate List - but I wasn't sure if these would need their own RelationType setup (and relations don't seem to Allow Macros as a Parent/Child type) - but anyway maybe that should be for a different PR - I'm just flagging that the bit that finds the Macros seems to have fallen into place!

Finally, there is an issue with the MultiMediaPicker Parameter editor (it never got converted to Udis, there is a separate PR for that - #8388) - but I've added its own DataValueEditor that will try for a csv of udis first, but fall back to use the integer ids - so it will cope in both scenarios.

Thanks for the pointers on the Macro Caching!

Marc

[Shazwazza]

I know this is still outstanding 😢 It's been starred in my inbox for ages to try to get around to but my time keeps slipping away. I've pinged some folks on the team to see if someone can assist with having a look.

[marcemarc]

Cool, thanks @Shazwazza

[marcemarc]

@Shazwazza any luck with passing this one on?

[umbrabot]

Hi there @marcemarc!

Thanks for the contribution here and apologies if it has been a while since you heard from us. We have been in the very fortunate position of having lots of work to do. With this in mind, we are writing to let you know that with the release of the Long Term Support (LTS) version, 8.18, we have now moved into the support phase of Umbraco 8. You can read all about that here but to surmise, we will be keeping Umbraco 8 safe and well by releasing patching for security or regression issues if they arise but no longer will we do that for bug fixes. The same is still true for features, although we stopped merging those some time ago.

We'd love for you to keep contributing and while we are not able to merge this to Umbraco 8, if this is still something you'd like to see in Umbraco 9, please take a look and either create an issue to say so or find an issue that already exists. We'll be happy to give you some input around how you can adjust your pull request to target Umbraco 9. Even better, it might be something that Umbraco 9 already does or has. In which case, enjoy!

Once again, a huge thank you for the time you have spent working with us.

#H5YR
Your friendly Umbraco GitHub bot 🤖 🙂