Fix registering uploads in repeating groups with Objects API (v2) #5059
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sergei-maertens
added
the
needs-backport
sergei-maertens
commented
Jan 29, 2025
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5059 +/- ##
==========================================
- Coverage 96.72% 96.72% -0.01%
==========================================
Files 770 770
Lines 26491 26542 +51
Branches 3454 3460 +6
==========================================
+ Hits 25623 25672 +49
- Misses 606 607 +1
- Partials 262 263 +1 ☔ View full report in Codecov by Sentry. |
|
We decided to backport this only to 3.0.x - the code on 2.8.x is too different and this bugfix is already unreasonably large. |
sergei-maertens
changed the title
sergei-maertens
force-pushed
the
issue/4689-upload-in-repeating-groups
branch
from
e42436b to
2ae1a36
Compare
robinmolen
approved these changes
Jan 30, 2025
vaszig
reviewed
Jan 30, 2025
Moved the file component key/value processing to the mapped variable processing instead of special casing this in the registration handler, since we will need access to the component types to handle editgrids which have file uploads inside, as these have different data paths *and* will require recursion as well since there can be editgrids inside editgrids that have this problem. The alternative is special casing repeating groups too, which breaks the mechanism to do component-specific post-processing in the dedicated function. This also updates the query for the submission variables so that if we have a more exact data path for an upload (inside a repeating group) that we use that instead of messing with the container editgrid which incorrectly gets replaced now. For uploads *not* in repeating groups, this data path is empty because it's identical to the variable key, so we can coalesce there at the DB level to calculate this in a unified way. See also #2713 that highlights the difficulties with how file uploads are now processed, which requires some proper re-structuring.
Fixed/added some type definitions to make it easier to reason about the data being operated on.
We must look up the uploads in the url map and replace only the file component nodes in the item values rather than the whole repeating group. This also needs to recurse, since in theory the repeating group can have another repeating group inside it.
sergei-maertens
force-pushed
the
issue/4689-upload-in-repeating-groups
branch
from
2ae1a36 to
c1f3989
Compare
sergei-maertens
added a commit
that referenced
this pull request
Jan 31, 2025
Moved the file component key/value processing to the mapped variable processing instead of special casing this in the registration handler, since we will need access to the component types to handle editgrids which have file uploads inside, as these have different data paths *and* will require recursion as well since there can be editgrids inside editgrids that have this problem. The alternative is special casing repeating groups too, which breaks the mechanism to do component-specific post-processing in the dedicated function. This also updates the query for the submission variables so that if we have a more exact data path for an upload (inside a repeating group) that we use that instead of messing with the container editgrid which incorrectly gets replaced now. For uploads *not* in repeating groups, this data path is empty because it's identical to the variable key, so we can coalesce there at the DB level to calculate this in a unified way. See also #2713 that highlights the difficulties with how file uploads are now processed, which requires some proper re-structuring. We must look up the uploads in the url map and replace only the file component nodes in the item values rather than the whole repeating group. This also needs to recurse, since in theory the repeating group can have another repeating group inside it. Backport-of: #5059
|
Backported to 3.0.x in 3f96489 The coverage failure can be ignored:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #4689
🤠 🔫 #2713 is so relevant...
Changes
Backporting this will be... fun.
Checklist
Check off the items that are completed or not relevant.
Impact on features
Release management
I have updated the translations assets (you do NOT need to provide translations)
./bin/makemessages_js.sh./bin/compilemessages_js.shDockerfile/scripts
./binfolderCommit hygiene