Laravel-Backpack / CRUD

Build custom admin panels. Fast!
https://backpackforlaravel.com
MIT License
3k stars 880 forks source link

Uploaders - Refactor and fixes #5478

Open pxpm opened 3 months ago

pxpm commented 3 months ago

This PR aims to fix and at the same time refactor uploaders and it's related validation classes.

Since launched some issues were identified for example the lack of validation support inside repeatables, or the tight coupling between an "ajax uploader" and a "dropzone uploader", that would prevent us from adding other types of ajax uploads like EasyMDE. Issues in dropzone validation .. etc.

There is a companion PR to this one in: https://github.com/Laravel-Backpack/PRO/pull/232

tabacitu commented 3 months ago

Please please PLEASE start writing a title and description for every PR you create Pedro. It takes 2-3 minutes, man. And it helps YOU and the rest of the team understand what's happening there.

I swear I will start closing PRs that are not descriptive. You've been warned.

pxpm commented 3 months ago

Check https://github.com/Laravel-Backpack/CRUD/issues/5484 before merging this.

karandatwani92 commented 3 months ago

Hey @pxpm

I tested fields of uploaders-test-branch. I'm facing many issues with it:

image

I'm considering testing subfields after the Green Flag on the above direct-use fields.

pxpm commented 3 months ago

I am sorry @karandatwani92

But I am not able to reproduce any of the issue you mentioned: image

I think I made the mistake of giving the test branch I used, I should have just let you guys create your own tests. It was intended to be used aa a guide, the branch it's not working AFAIK, it's missing routes for example.

I think in 1) it's clearly missing ->withFiles(). 🤷

I couldn't reproduce any of the other bugs you mentioned. The image of the easy mde in the screenshot is in:

CRUD::field('easymde')->type('easymde')->withFiles([
            'path' => 'test',
        ]);

image

Let me know if I can help you with something.

karandatwani92 commented 3 months ago

Hey @pxpm, I'm facing the same issues even with a fresh installation. PLUS, I found one more bug:

pxpm commented 3 months ago

@karandatwani92 test issues:

karandatwani92 commented 3 months ago

Hey @pxpm

The above bugs are fixed✅. Now I started testing subfields under repeatable and there is an issue:

pxpm commented 2 months ago

Hey @pxpm

The above bugs are fixed✅. Now I started testing subfields under repeatable and there is an issue:

  • Suppose we created two rows and saved them. Go to Edit and Reorder - (It breaks saving: causes validation error)❌

Thanks @karandatwani92 haven't spotted that. 🙏 nice catch!

I've just pushed fixes both in this PR and in PRO PR to address that issue. Please note that I rebased both branches, so if you have any troubles with a git pull, please just delete the local branch and download a new copy.

Thanks again 🙏

karandatwani92 commented 2 months ago

Hey @pxpm

New Issue: Create new, edit, and save without changes(validation error even when the file is already attached): Screenshot 2024-04-18 at 2 47 54 PM

karandatwani92 commented 1 month ago

New Bugs:

  1. Message for Default Validation Error is missing. (See the difference.) Screenshot 2024-05-24 at 9 09 27 PM
  2. If I switch the repeatable row position, the subfield upload value stays, but the file gets deleted. Screenshot 2024-05-24 at 9 19 08 PM
pxpm commented 1 month ago

1 is not a bug, in the demo app mimetypes is not added to translations, it comes by default with Laravel since Laravel 9 if I am not mistaken, we never bothered adding it to the demo because we don't use it.

2 might be a bug, I am not sure if I have a test case covering that, if not I will add one and evaluate that scenario.

Thanks.

pxpm commented 1 month ago

@karandatwani92 I've identified the issue you reported in 2 and added a test alongside with the fix for it. 🙏

karandatwani92 commented 1 month ago

Hey @pxpm

I tested it, and it is working completely fine. Please continue with the media uploader and we'll re-test again.