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

Media dropzone improvements (Batch upload, Upload sorting, Folder upload, Progress notification) #2258

Closed
wants to merge 6 commits into from

Conversation

nikcio
Copy link
Contributor

@nikcio nikcio commented Sep 6, 2024

Description

  • Fixes #16559
    • This implements batching of the file uploads which creates this issue and then fixes it by sorting the uploaded files after a successful upload. This makes sure the order remains the correct order.
  • Fixes #16484
    • This adds the missing implementation for folder uploads
  • Fixes #16508
    • This changes from using the MIME type to using the file extension in the file name and will therefore mark the file as audio if the file extension on a file is .mp3

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Motivation and context

This aims to solve some of the problems and shortcomings of the drag-and-drop functionality in the Media library

How to test?

Usage of the media drag and drop feature. As well as other usages of the umb-dropzone component. This can also be found in the media type import and document type import but see the "Still present bugs" section first.

Screenshots (if appropriate)

Multiple media types available for uploaded files:
image

Info view of files for selection (Can be shown by pressing the info icon)
image

Progress notification (Auto updates with progress)
image

Completed progress notification
image

Videos

Notice that in these videos I have a custom Media type that applies to the images I upload which means I get the popup every time.

To create a media type for this follow these steps:

  1. Create a media type
  2. Add a Upload field with an alias of umbracoFile
  3. Add jpg or any other required image file type to Accepted file extensions

Video 1 - Normal uploads

Umbraco.Google.Chrome.2024-09-06.22-47-36.mp4

Video 2 - Folder uploads

Umbraco.Google.Chrome.2024-09-06.22-48-02.mp4

Video 3 - Upload as Custom media type

Umbraco.Google.Chrome.2024-09-06.22-49-58.mp4

Checklist

  • If my change requires a change to the documentation, I have updated the documentation in this pull request.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
    • I would like some guidance on what would make sense here.

Still present bugs

  • Unable to drag and drop files produced by Umbraco due to missing MIME type

  • In the Import media type / document type feature there is no error handling if you drag and drop for example a picture in the dropzone but that is outside the scope of this PR to fix.

  • You are unable to dismiss any notification when having a folder open.

  • SQL database table locked

    • I'm pretty sure that none of these changes have created this bug but I've had a lot of issues with the database being locked when uploading many images at a time.
Stacktrace from this:

[22:10:20 ERR] Failed executing DbCommand (30,100ms) [Parameters=[@__identifier_0='?' (Size = 44)], CommandType='Text', CommandTimeout='30']
SELECT "u"."Id", "u"."ApplicationId", "u"."AuthorizationId", "u"."ConcurrencyToken", "u"."CreationDate", "u"."ExpirationDate", "u"."Payload", "u"."Properties", "u"."RedemptionDate", "u"."ReferenceId", "u"."Status", "u"."Subject", "u"."Type", "u0"."Id", "u0"."ApplicationType", "u0"."ClientId", "u0"."ClientSecret", "u0"."ClientType", "u0"."ConcurrencyToken", "u0"."ConsentType", "u0"."DisplayName", "u0"."DisplayNames", "u0"."JsonWebKeySet", "u0"."Permissions", "u0"."PostLogoutRedirectUris", "u0"."Properties", "u0"."RedirectUris", "u0"."Requirements", "u0"."Settings", "u1"."Id", "u1"."ApplicationId", "u1"."ConcurrencyToken", "u1"."CreationDate", "u1"."Properties", "u1"."Scopes", "u1"."Status", "u1"."Subject", "u1"."Type"
FROM "umbracoOpenIddictTokens" AS "u"
LEFT JOIN "umbracoOpenIddictApplications" AS "u0" ON "u"."ApplicationId" = "u0"."Id"
LEFT JOIN "umbracoOpenIddictAuthorizations" AS "u1" ON "u"."AuthorizationId" = "u1"."Id"
WHERE "u"."ReferenceId" = @__identifier_0
LIMIT 1
[22:10:20 ERR] An exception occurred while iterating over the results of a query for context type 'Umbraco.Cms.Persistence.EFCore.UmbracoDbContext'.
Microsoft.Data.Sqlite.SqliteException (0x80004005): SQLite Error 6: 'database table is locked'.
   at Microsoft.Data.Sqlite.SqliteException.ThrowExceptionForRC(Int32 rc, sqlite3 db)
   at Microsoft.Data.Sqlite.SqliteDataReader.NextResult()
   at Microsoft.Data.Sqlite.SqliteCommand.ExecuteReader(CommandBehavior behavior)
   at Microsoft.Data.Sqlite.SqliteCommand.ExecuteDbDataReaderAsync(CommandBehavior behavior, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.Storage.RelationalCommand.ExecuteReaderAsync(RelationalCommandParameterObject parameterObject, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.Storage.RelationalCommand.ExecuteReaderAsync(RelationalCommandParameterObject parameterObject, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.Query.Internal.SingleQueryingEnumerable`1.AsyncEnumerator.InitializeReaderAsync(AsyncEnumerator enumerator, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.Query.Internal.SingleQueryingEnumerable`1.AsyncEnumerator.MoveNextAsync()
Microsoft.Data.Sqlite.SqliteException (0x80004005): SQLite Error 6: 'database table is locked'.
   at Microsoft.Data.Sqlite.SqliteException.ThrowExceptionForRC(Int32 rc, sqlite3 db)
   at Microsoft.Data.Sqlite.SqliteDataReader.NextResult()
   at Microsoft.Data.Sqlite.SqliteCommand.ExecuteReader(CommandBehavior behavior)
   at Microsoft.Data.Sqlite.SqliteCommand.ExecuteDbDataReaderAsync(CommandBehavior behavior, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.Storage.RelationalCommand.ExecuteReaderAsync(RelationalCommandParameterObject parameterObject, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.Storage.RelationalCommand.ExecuteReaderAsync(RelationalCommandParameterObject parameterObject, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.Query.Internal.SingleQueryingEnumerable`1.AsyncEnumerator.InitializeReaderAsync(AsyncEnumerator enumerator, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.Query.Internal.SingleQueryingEnumerable`1.AsyncEnumerator.MoveNextAsync()
   at Microsoft.EntityFrameworkCore.Query.ShapedQueryCompilingExpressionVisitor.SingleOrDefaultAsync[TSource](IAsyncEnumerable`1 asyncEnumerable, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.Query.ShapedQueryCompilingExpressionVisitor.SingleOrDefaultAsync[TSource](IAsyncEnumerable`1 asyncEnumerable, CancellationToken cancellationToken)
   at OpenIddict.Core.OpenIddictTokenCache`1.<>c__DisplayClass12_0.<<FindByReferenceIdAsync>g__ExecuteAsync|0>d.MoveNext()
--- End of stack trace from previous location ---
   at OpenIddict.Core.OpenIddictTokenManager`1.FindByReferenceIdAsync(String identifier, CancellationToken cancellationToken)
   at OpenIddict.Core.OpenIddictTokenManager`1.OpenIddict.Abstractions.IOpenIddictTokenManager.FindByReferenceIdAsync(String identifier, CancellationToken cancellationToken)
   at OpenIddict.Validation.OpenIddictValidationHandlers.Protection.ValidateReferenceTokenIdentifier.HandleAsync(ValidateTokenContext context)
   at OpenIddict.Validation.OpenIddictValidationDispatcher.DispatchAsync[TContext](TContext context)
   at OpenIddict.Validation.OpenIddictValidationDispatcher.DispatchAsync[TContext](TContext context)
   at OpenIddict.Validation.OpenIddictValidationHandlers.ValidateAccessToken.HandleAsync(ProcessAuthenticationContext context)
   at OpenIddict.Validation.OpenIddictValidationDispatcher.DispatchAsync[TContext](TContext context)
   at OpenIddict.Validation.OpenIddictValidationDispatcher.DispatchAsync[TContext](TContext context)
   at OpenIddict.Validation.AspNetCore.OpenIddictValidationAspNetCoreHandler.HandleAuthenticateAsync()
   at Microsoft.AspNetCore.Authentication.AuthenticationHandler`1.AuthenticateAsync()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)

const notification = { data: { message: `Created` } };
this.#notificationContext!.peek('positive', notification);
// const notification = { data: { message: `Created` } };
// this.#notificationContext!.peek('positive', notification);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would like some sparing on what HQ finds would be the best way to handle this. I've disabled the notification here because when uploading many files the right hand side of the screen just becomes a green wall of Created messages. I think the best way for the drag and drop feature is to not have any Created messages at all and only the progress and failure notification messages show up but is it feasible to move the notification from here closer to the places we would like to show it I can see this method has many references.

@@ -48,12 +49,29 @@ export class UmbMediaCollectionElement extends UmbCollectionDefaultElement {
this._progress = event.progress;
}

#refreshCollection() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To avoid over updating the UI this has been added to we don't update the UI more than every 350ms


@customElement('umb-dropzone')
export class UmbDropzoneElement extends UmbLitElement {
@property({ attribute: false })
parentUnique: string | null = null;

@property({ type: Boolean })
multiple: boolean = true;
multiple: boolean = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my testing it is not possible to make a boolean property false if it's by default true therefore the default for multiple has to be false which also makes more sense when looking at the usage: <umb-dropzone multiple></umb-dropzone>

I've updated the current usages accordingly

@nikcio nikcio marked this pull request as ready for review September 6, 2024 21:14
Copy link

@iOvergaard
Copy link
Collaborator

Hi @nikcio

Thanks for what seems like a well-thought-out pull request. It looks like you were trying to fix some issues which then turned into more bug fixes and even new features, great!

I would like to highlight that our contribution guidelines state that we prefer new features to be discussed first to optimize the chances of them getting merged and also to divide multiple bug fixes up into smaller pull requests to make them easier to review.

All that being said, we really like the proposed changes here and we would like to consider this pull request for the upcoming V15 release of Umbraco. The first RC of V15 will be released at the beginning of October, and we need to get 14.3 out of the way first, but following that, we'll start the review of your contributions in this PR. I will let this PR stay open until then.

Thanks again and #h5yr

@loivsen
Copy link
Contributor

loivsen commented Oct 8, 2024

Hey @nikcio

We have been reviewing your PR and we have some comments:

Progress notification:
We would like to discuss this part further internally as we are planning to make some bigger changes to notifications. This means we are holding back merging in this feature for now, despite we know the current solution (sending a "Created" message out every time a media item gets created) is really not ideal. Your solution will be kept in mind when we discuss this further.

Fixed issues:
Issue #16559 is posted as an issue for v13 and since this PR touches a later version it does not fix the linked issue. ❤️
Issue #16484 has the sprint-candidate label and therefore been part of our sprint planning here at HQ, which means we have been working on some of the same features.

This PR:
Due to overlapping work from HQ, this PR have ended up with some major merge conflicts. There are still really good parts of the PR we would like to take into our codebase, but due to the complexity of the conflicts we are going to make new PR(s) and copy-paste parts of your code in, and put you as co-authored on the commits so that you also get the credit you deserve. I hope this is all okay with you.

Thank you for your contribution! #H5YR

@nikcio
Copy link
Contributor Author

nikcio commented Oct 8, 2024

Hi @loivsen

No worries. Let me know if I can do anything to help out.

Just a note about 16559: The changes in this PR both reintroduce and fix this issue. It implements batching which reintroduces this issue into v14 and then subsequently fixes the issue by sorting the items after upload. 😉

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