Don't save duplicate templates on save

[bjarnef]

Prerequisites

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

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

Description

This PR removes the logic to save template on blur and since we really don't use the behaviour anywhere else in backoffice, where it require a button click or use the shortcut key. You could easily by mistake remove the focus on the header, which trigger a save.

The CreateView() method was set to overwrite which make sense it always overwrite any existing template on disk. Changing this to false will instead use the existing view on disk if any exists otherwise create a new view.

Alternatively it could be a configuration option, but not sure we want that 🤷‍♂️

[bjarnef]

I noticed another minor thing, when saving an template with empty or invalid alias we get an infinite spinner.

It should always change save button state independent of "suppressNotification".

Fixed in e0fcfbb

[mikecp]

Hi @bjarnef ,

Thanks for your PR!

I've read the conversation in the issue and I'd like to come back on one suggestion of @nathanwoulfe about offering to load the file if it exists.

My concern goes to the fact that with the "overwrite = false" solution, the template editor might ignore that the physical file already exist and then spend some time creating the template in the backoffice, then save it, and in fact lose all his/her work because we will just ignore what was entered and just load what was saved on disk.

I guess the "ideal" scenario might be that the "onblur" on the alias would trigger a check on disk to verify if there is a file with that name, and if yes load it (maybe with a warning), and if not, do nothing. And then the Save would just create it with overwrite=true.

Also, maybe another consideration might be to have an homogeneous behavior with stylesheets and scripts files, which seem to just overwrite whatever was on disk before...

@bjarnef @nathanwoulfe what is your opinion?

[mikecp]

Update:
I just made some quick tests without this fix, and it seems to be working as expected and not creating duplicates :-S I'll test further tomorrow, and/or maybe you have a scenario where duplicates are created?

[bjarnef]

@mikecp with the current behaviour it creates a template on blur and another from the button save. Furthermore I don't think you neccesary want to save if placing focus outside the name field. In fact I think this is a bit inconsistent since we don't use this behaviour anywhere else in backoffice: datatypes, partial views, scripts, styles, content, etc. instead it makes the form dirty and trigger the "Unsaved changes" dialog.

Maybe there should be an additional warning instead when saving the template in case a psysical view already exists on disk, where you can choose "Yes, overwrite" og "No, keep original"?

Currently we two options:

• Overwrite view on disk (you'll loose the content in the original view)
• Don't overwrite view on disk (it will load the original content in the view, but you'll loose any content added to template view from backoffice)

I think no matter which one of these we choose, developers might see this as an issue. Original og was reported, because it was expected to use view on disk, while other may prefer to always use backoffice template view content no matter what exists in disk.

Furthermore I wonder how this currently works on scripts, styles, partial views? E.g. creating a "Breadcrumb" either from empty or snippet and you already have an "Breadcrumb" view on disk. I think this should work in a consistent way.

I think it may require some feedback from HQ as well how we want this. @nul800sebastiaan

Honestly I like this approach more, overwrite or not overwrite view on disk, but without the in blur as I think this is more consistent with the redt og the backoffice UI/UX.

[mikecp]

@bjarnef Thanks for your feedback!

I did not have time yet to check further why it seems to behave differently on my local PC without your changes (I don't get duplicate templates created), but I already wanted to react on the fact that I agree with you that all different sections should ideally work in a consistant way. And also with the fact that creating a new template file on disk during an onblur event should be removed.

In fact all scripts, styles, partial views and partial views maco files sections work the same; we just overwrite whatever is on disk. BUT those section also auto-sync with the situation on disk. And if you add a new file on disk while the section is already opened in the backoffice, doing a "reload" on the section root will refresh the tree and show that newly created file.
So, basically, you have a more or less easy way to move back-forth between backoffice situation and disk situation and to synchronize them.

This is not the case with the Templates section, and I guess this is due to the fact that the tree under there depends on the "master layout" of the template and is therefore not directly dependent of the disk situation. So I guess that we don't have much choice but try to get as close as possible from the behavior of the other section.

As you mention, choosing one of the two options "overwrite/no overwrite" will be seen, as an issue by the people who expected the other behavior 😕

There might be other scenario's that would maybe decrease the risk of "frustrations":

• onblur of the name would just check if there is a matching file on disk and then display a warning and offer to load it
• next to the "Create.." menu, have something like a "Create from disk file..." menu. In that case, you could for example open the "create" form with only the alias enabled and then a "load from disk" button instead of the "save" button. And then, once the content is loaded, the template zone would be enabled , the "load from disk" button would disappear and the "save" button would come back (just thinking out loud 😁)
• Keep the menu as is for consistency with the rest and then have both "load from disk" and "save button" together on the create form.

In any case, like you said, we need some feedback from HQ to know in which direction to go from here... @nul800sebastiaan ?

[bjarnef]

Regarding templates is a special case as these are stored in the database and rendered in tree, where tree nodes in scripts, styles and partial views are based on physical files on the disk.

Maybe it should do an additional check on submit/save to check if the physical file/view exists on disk. If it does, then show an overlay/alert which require a confirmation, otherwise it won't continue the save.

Alternatively this could always overwrite - in most cases I guess developers create the templates from backoffice first, but they could of course have copied some base templates from another project, where they just want to create templates for these in backoffice. So this could maybe be another menu action like "Create template from file".

The overwrite or not overwrite could both cause some frustrating, but I think the current behaviour cause more frustrating as it create duplicate templates each time you create a new template :)

[bjarnef]

Okay, actually I can't reproduce easily currently without this change when creating a new template at root or under another template.
It may be something when only happen when on blur and save both trigger a save in a specific scenario.
However I noticed when creating an template eg. "Base" it generate an alias "base", but on save this is changed to "Base".

[mikecp]

Yeah it's a tricky one... I actually accidentally could reproduce the duplicate creations while debugging, because by going back/forth from backoffice to the debugger, I triggered multiple onblur events before the file could actually be created on disk, and I ended up with 3 new templates in the backoffice linking to 2 different physical files 😂

I could not reproduce this without breakpoint, but who knows, if for some reason the save operation takes time, maybe some people get that "multi-trigger of onblur event" situation in normal usage of the backoffice. But my feeling is that the duplication does no come from the actual click on Save.

Anyway, we agree that the "save onblur" should go away 😀

My proposition for a solution now would be to remove the onblur behavior like you did, but I would keep the overwrite parameter to true when creating the view:

_viewHelper.CreateView(template, true)

This way, we remove the risk of duplicate creation and we keep a consistent behavior with the other sections (scripts, styles etc.) of overwriting whatever exists on disk.

In the meantime I'll try to check with @nul800sebastiaan or @emma-hq what extra behavior might be interesting to have (check for existence and display warning, have extra menu "create from file", ...).

What do you think? Could you update the overwrite parameter and then I merge?

[bjarnef]

@mikecp I have changed it back to always overwrite the view on disk.

[umbrabot]

Hi there @bjarnef!

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 🤖 🙂