Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ported over the fixes in #11858 "Check media Parent for permissions when setting correct MediaType" to target v8 #12233

Merged
merged 4 commits into from
Apr 21, 2022

Conversation

OwainJ
Copy link
Contributor

@OwainJ OwainJ commented Apr 6, 2022

Ported over the fixes in the v9 PR #11858 Check media Parent for permissions when setting correct MediaType to v8.

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 #7735

Description

I have ported over fixes from the v9 PR #11858, as the fixed issue also affected v8, the fixes have been tweaked slightly to work with v8's codebase.

Description from the v9 PR:

Ever since Umbraco version 8.14, the way media gets uploaded has changed. In the past, uploading media directly from a MediaPicker would respect the permissions set on the upload folder when not using the default mediaType of "Image". Ever since version 8.14 this has changed, and even if the folder does not allow the default "Image" item to be uploaded, it still forces the uploaded image to be that media type (and same with File).

With this pullrequest, as created and demonstrated during today's umbraCollab, we not just upload an image with the default 'Image' Media Type, but first try to determine if the folder we are trying to upload the item to has any specific permissions set that have the "umbracoFile" property. If that is the case, we want to use that Media Type instead of the default!

For Folders we are not able to distinguish between custom folders or the default Folder, so best practice for now would be to not allow the creation of the default Folder if the parent doesn't explicitly allow this, so that we can't create folders where they aren't allowed!

To test this Pullrequest, simply try out any mediapicker that points towards a media folder that has specific permissions set up, upload a new image, and it will upload it to the correct media type! If it does not find any items within it's permitted children with a property of alias umbracoFile, it will continue with the rest of the Umbraco logic and upload to the default type!

Gif of the drag & drop upload working correctly in v8:
v8 media drag and drop fix

@umbrabot
Copy link

Hi there @OwainJ, 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 🤖 🙂

Copy link
Contributor

@elit0451 elit0451 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @OwainJ for porting the fix to Umbraco 8 🙌 I reviewed and tested it and left a few comments for you. I will push the needed changes and once the build pipeline succeeds I will make sure to merge it 😉

Comment on lines 604 to 606
var notificationModel = new SimpleNotificationModel();
notificationModel.AddErrorNotification(Services.TextService.Localize("speechBubbles", "folderCreationNotAllowed"), "");
throw new HttpResponseException(Request.CreateValidationErrorResponse(notificationModel));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we can just use the CreateNotificationValidationErrorResponse() that we already have and pass in the error msg. It is doing exactly what you are trying to do here. I will fix up

var saveResult = mediaService.Save(f, Security.CurrentUser.Id);
if (saveResult == false)
{
AddCancelMessage(tempFiles, Services.TextService.Localize("speechBubbles", "operationCancelledText") + " -- " + mediaItemName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specifying the message param is needed here because otherwise, the AddCancelMessage() will interpret the passed text as the header and the message displayed will end up being just the default operationCancelledText. Will fix up!

@@ -644,6 +652,11 @@ public async Task<HttpResponseMessage> PostAddFile()
//in case we pass a path with a folder in it, we will create it and upload media to it.
if (result.FormData.ContainsKey("path"))
{
if (!IsFolderCreationAllowedHere(parentId))
{
AddCancelMessage(tempFiles, Services.TextService.Localize("speechBubbles", "folderUploadNotAllowed"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specifying the message param is needed here because otherwise, the AddCancelMessage() will interpret the passed text as the header and the message displayed will end up being just the default operationCancelledText.

Also, the syntax for v8 is a little different, here we can just pass "speechBubbles/folderUploadNotAllowed" as the method is already doing the localization

Will fix up!

@elcoinkeur
Copy link

Hi ! We updated our Umbraco site to 8.18.3 from and older version some weeks ago, and figured out about this issue just now. As 8.18.3 just released a month ago, is there any plan to release 8.18.4 anytime soon ? It's frustrating to go to medias each time we want to upload a specific image type.

@OwainJ
Copy link
Contributor Author

OwainJ commented May 13, 2022

@elcoinkeur according to the release progress tracker, 8.18.14 is scheduled for release on Monday, the 16th of May.
You can view the release progress here: https://our.umbraco.com/download/releases/8184

@stefanbesteman-weareyou

Hi @OwainJ ,

We upgraded an existing umbraco 8.17.1 to 8.18.5 where we use differtent media type images per folder.

After the upgrade we could not upload new images anymore to our specific folders.

We use for example a mediatype 'Header visual images Folder' with rights to add 'Header visual image' media type and rights to add a subfolder of the same media type 'Header visual images Folder'. If we remove the rights to add the subfolder it works again as expected and this is for now a good workaround, but our client will want to make subfolders in the future. Can you please fix this in the next version?

Thanks in advance
Stefan Besteman (IO Digital)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants