fix(upload-files): do file upload on components too #14724
Conversation
Codecov ReportBase: 59.61% // Head: 59.69% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #14724 +/- ##
==========================================
+ Coverage 59.61% 59.69% +0.07%
==========================================
Files 1340 1340
Lines 32639 32641 +2
Branches 6225 6225
==========================================
+ Hits 19459 19486 +27
+ Misses 11316 11297 -19
+ Partials 1864 1858 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
nathan-pichon
added
source: core:upload
nathan-pichon
force-pushed
the
fix/file-upload-components
branch
from
589564c
to
1d45c81
Compare
nathan-pichon
assigned petersg83, Marc-Roig and jhoward1994 and unassigned petersg83, Marc-Roig and jhoward1994
| if (files && Object.keys(files).length > 0) { | ||
| await this.uploadFiles(uid, entity, files); | ||
| await this.uploadFiles(uid, Object.assign(entityData, entity), files); |
There was a problem hiding this comment.
Nice, this works well for me. I'm just wondering if we could use entityData directly in the uploadFiles function? To me it seems to already contain the properties of entity + the component fields we need for this fix. There may be something I'm missing though, WDYT?
There was a problem hiding this comment.
I don't really know.
I have done this way to change the fewer things I could in the function and keep the current behavior.
Do we want the function to return the entity + component ?
Edit: Sorry, morning 😅 , we didn't have the ID of the entity in entityData. Also, if anything is in entity and not in entityData, I prefer to ensure that uploadFiles(...) got all the data by merging those two. What do you think?
Was on it, but only unit tests. |
nathan-pichon
force-pushed
the
fix/file-upload-components
branch
from
64ab431
to
96e87ab
Compare
There was a problem hiding this comment.
It looks good to me!
I tested with :
- single component
- repeatable components
- dynamic-zones
One case that didn't work is when, for a single component, I put nothing in data for the component and only upload a file. It can happen if the component only has one field which is a media or when we just don't put values for the other fields.
But I think we can ignore that for the moment.
I added a few questions
| qs: { | ||
| populate: ['todo', 'todo.docs'], | ||
| }, | ||
| }); |
There was a problem hiding this comment.
If you want you can also put the populate param on the POST request to have the response populated so you don't have to use newlyCreatedTodolist.
But if it was intended (to test that the changes were correctly made up to the GET I guess) it's perfect too 👌
There was a problem hiding this comment.
I didn't know that you could add the populate parameter in the entity creation. I guess that it's also a way to achieve the test.
I don't know if it's better to have only the POST call in the test or if adding the GET adds unwanted complexity to the test.
There was a problem hiding this comment.
I don't have a strong opinion on that. I just wanted to let you know. As you wish :)
packages/core/strapi/lib/services/entity-service/__tests__/entity-service.test.js
Show resolved
Hide resolved
nathan-pichon
force-pushed
the
fix/file-upload-components
branch
2 times, most recently
from
03db1b5
to
506c8f7
Compare
nathan-pichon
force-pushed
the
fix/file-upload-components
branch
from
795c248
to
3c1de75
Compare
What does it do?
The files related to the components were never linked to them. This fix allows to manage the files sent to be linked to the components.
Giving the full entity with components allow the uploadFiles to works on components level too.
Why is it needed?
Because we need to be able to attach files to components
How to test it?
See issue #14126
Related issue(s)/PR(s)
#14126