Add support for file upload property editor within the block list and grid

[PeterKvayt]

Update 26/5, @AndyButland: I've renamed the title of this PR from "Fixes #18872 currentValue is not current in Umbraco.BlockList element properties" as it's more than the fix that this describes, and now supports making available the file upload property editor within block list and grid elements."

Prerequisites

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

Description

Fixes #18872

Here, I am adding additional logic to include in processing the current value.

In the original implementation umbraco set the edited and current value in the following way:

As you can see, the current value was always the same and equal to edited value. This leads to unexpected behavior for property editors, that rely on current value.

In this PR I am parsing currentValue:

and then creating a mapping for edited and current data. Mapping allows us to determine whether the value was cleared or removed (it is if edited was null), or changed (it is if edited and current are not null), or just created (it is if current is null).
In any case, it would be processed in the same way.

Also I have changed the following piece of code:

So now it is now "fake" content property data object, it is a "real".

Also there is an additional fix for FileUploadPropertyValueEditor. The fix covers the case when the current value is not just a string, but presented as json string with src value (I have faced with this case during testing).

---

This item has been added to our backlog AB#51895

[github-actions]

Hi there @PeterKvayt, thank you for this contribution! 👍

While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

• It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
• The automated tests all pass (see "Checks" tab on this PR)
• The level of security for this contribution is the same or improved
• The level of performance for this contribution is the same or improved
• Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
• If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
• 💡 The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

[PeterKvayt]

I can't understand what the issue is with the failed check.

[AndyButland]

Hi @PeterKvayt - I'll try to give this a review next week. Before I do though, are you able to update here or on the issue with a practical example of the problem the code concern you've found please? I do agree it looks incorrect, and it seems we should expect a property to behave the same with a block list or when used directly on a content item, but it would be good to understand what "real world" problem it is causing. Partly for prioritisation, and partly to be able to verify that the PR does resolve the issue. Thanks.

I can't understand what the issue is with the failed check.

This is due to the change you have made in to the signature of this method: Umbraco.Cms.Core.PropertyEditors.BlockValuePropertyValueEditorBase<TValue, TLayout>.MapBlockValueFromEditor(TValue) - as it's protected it's part of a public API, so we can't change this other than perhaps in a major version bump. So likely we would need to keep it, introduce a new method and obsolete the original.

[PeterKvayt]

Hi @AndyButland!
Thanks for the reply.
I will provide a practical example a little bit later (hopefully this week or at the beginning of next week).

[PeterKvayt]

Okay, so I have a few issues related to this PR:

• 15.3.0-rc2: FileUploadPropertyValueEditor throws "Invalid content key." #18718
• Why Umbraco.FilleUpload is not supported for blocks? #18755
• "currentValue" is not current in Umbraco.BlockList element properties #18872
• Error 404 on file link click. #18407 (comment) (this issue a little bit related to our PR, but the comment describes why I can't use the media picker).

I encountered the following situation: Initially, I was using the standard document type with the default Umbraco.FileUpload property (which, at the time, was not part of a Block List). Later, I needed to create a custom FileUpload property editor with an additional setting to manage a prefix for the uploaded file path. At that point, I started using my custom FileUpload property editor.

Eventually, I needed to use this custom editor within a Block List element property. Since it was a custom property editor, it was allowed to be used within Block List elements — unlike the default Umbraco.FileUpload, which isn't supported in that context. I resolved the issues related to using Umbraco.FileUpload in Block Lists (see: #18755).

As a result, I now have a custom FileUpload property editor that can be used inside Block List elements. It also handles file deletion from storage properly, as we're addressing the issue related to invalid current values that previously caused problems.

[AndyButland]

Hi @PeterKvayt - I've been reviewing this today and think it's looking good. Working as expected when I step through and I've resolved the breaking change and added some unit tests now to verify the behaviour.

I've also enabled the file upload property to be used in the block list - which, from what I read above, has been your aim with the issues you've raised and this PR. Having asked around it seems that this was a restriction in Umbraco 13 but doesn't need to be in 15+, so in theory it should work as expected (particularly with the change you've proposed here).

Would you mind taking a look over my changes and see if you can find any issue with the use of the file upload in the block list (or in the normal use as a property directly on a document type) please? Then if all looks good to you I think we can bring this in.

[AndyButland]

Just wondering if you'd chance to look over my comments and changes please @PeterKvayt , and been able to confirm that with these updates the file upload now functions in the block list as you'd expected?

[PeterKvayt]

@AndyButland
I have already started a review and left some notes.
I thought you had already seen them.

[AndyButland]

I'd don't see anything I'm afraid @PeterKvayt - can you point me to them please? Or if they've got lost, re-apply to the conversation on this PR? I'm away for a few days but can look to pick up again next week.

[PeterKvayt]

@AndyButland
I have fixed all the cases, so now it is woking correctly.

But not all the tests have been passed.
I have not so deeply dived to solve them.
Could you help?

[AndyButland]

I think this is looking good @PeterKvayt - I've replicated all your test cases and can confirm that we now see the expected results. And I've resolved the failing tests and backward compatibility checks you were seeing.

My only concern was the knowledge that we are putting into the file upload property editor about blocks, and was wondering if these should really be separated some way. But on reflection I think it's OK. We already have an abstraction here with the notification handlers, as it's only in their implementation we need to look into the block property data. And whilst we perhaps should have a separate class for the notification handlers rather than combining them into FileUploadPropertyEditor, it's not unreasonable to have it this way, isn't added in this PR and in any case couldn't be split out in a non-breaking way.

[PeterKvayt]

@AndyButland I agree that the file upload property editor knows a lot about other property editors.
I think it is better to move logic related to notification handlers into a separate place.
I have added all the logic into the file upload property editor because similar logic has been implemented in the same way.
I think we can add "TODO" here to make this refactoring in the future.

I was just wondering why you removed my "TODO" about the temporary file?

Anyway, I think it is ready to merge.

[kraftvaerk-chth]

Looking forward for this to be merged! :D

[AndyButland]

Just updating that I've found a few issues with blocks in the RTE that we haven't accounted for (checked items are now fixed):

• Deleting the content when you have file upload blocks in the RTE doesn't remove the file from disk
• Deleting the block from the RTE when you have previously populated a file upload block doesn't remove the file from disk
• Copy of content isn't handling the copy of the file when the block is in the RTE.
• For deletes and copies, check and handle nested RTEs (i.e. blocks with RTE fields that contain blocks).

And regarding earlier review points, think it makes sense that we do the following clean-up:

• Refactoring of the notification handlers into separate classes.

[AndyButland]

I believe I've handled the extra cases relating to blocks within the rich text editor now, as detailed in the comment above.

@PeterKvayt - would you be up for doing a final review and test of my changes to see that in your opinion it's working as expected, and in line with your intentions when creating the PR please? Thanks.

@Migaroez - similarly, if you are up to continue the internal review you started and then found I was still working, please do. I'm finished with testing and amending myself now. Thanks.