Skip to content

Flatten attachments #6608

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.

Sign up for GitHub

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

Jump to bottom
Merged
ThiefMaster merged 7 commits into from
Nov 20, 2024
Merged

Flatten attachments #6608

ThiefMaster merged 7 commits into from
Nov 20, 2024

Conversation

@moliholy
Copy link
Contributor

@moliholy moliholy commented Nov 9, 2024 •
edited
Loading

Closes #6536.

This PR flattens the event attachments as described in #6536.

Implementation notes

The endpoint accepts a flatten boolean field. If set to True, it flattens the files as described bellow, while if False it maintains the old behaviour.

Please note that a new Download Attachments (flat) entry has been added.

Screenshot 2024-11-12 at 09 05 56

Previously

./
├── Abensarz_Krzysztof_33/
│  └── File_upload_1220987_KHAbensarz.pdf
├── Camacho_Ninho_Victor_30/
│  └── File_upload_1220987_Abstract-PaE-_Electrodynamics.pdf
├── Sabenso_Gabriel_23/
│  └── File_upload_1220987_Abstract_G.Sabenso.pdf
└── Piotr_Edgar_26/
   └── File_upload_1220987_abstract_PE.pdf

After the changes in this PR

./
├── Abensarz_Krzysztof_33_File_upload_1220987_KHAbensarz.pdf
├── Camacho_Ninho_Victor_30_File_upload_1220987_Abstract-PaE-_Electrodynamics.pdf
├── Sabenso_Gabriel_23_File_upload_1220987_Abstract_G.Sabenso.pdf
└── Piotr_Edgar_26_File_upload_1220987_abstract_PE.pdf

@moliholy
Copy link
Contributor Author

moliholy commented Nov 9, 2024 •
edited
Loading

@ThiefMaster this is introducing a breaking change, so I'll wait for your input on this.

@tomasr8
Copy link
Member

tomasr8 commented Nov 11, 2024

Is the user supposed to be able to control whether the list is flat or not? I think out of the three options proposed in the linked issue, option 3 sounds the best to me

@moliholy
Copy link
Contributor Author

moliholy commented Nov 12, 2024 •
edited
Loading

I think out of the three options proposed in the linked issue, option 3 sounds the best to me

@tomasr8 I just implemented that option. I edited the description accordingly.

EDIT: never mind, it actually didn't always work. I've been struggling with form submission and JQuery. This is a best-effort, but I'm not sure the cleanest approach. Mind to take a look?

@tomasr8
Copy link
Member

tomasr8 commented Nov 13, 2024

I don't know if this is the best way, but you could pass the flatten param directly in url_for as a query string. That way, you don't need to change the js callback at all:

diff --git a/indico/modules/events/registration/controllers/management/reglists.py b/indico/modules/events/registration/controllers/management/reglists.py
index 5a64156dda..e843c07620 100644
--- a/indico/modules/events/registration/controllers/management/reglists.py
+++ b/indico/modules/events/registration/controllers/management/reglists.py
@@ -119,7 +119,6 @@ class RHRegistrationsListManage(RHManageRegFormBase):
                 'attachment',
                 type='href-custom',
                 url=url_for('.registrations_attachments_export', regform),
-                params={'flatten': False},
                 weight=60,
                 extra_classes='js-submit-list-form regform-download-attachments',
             ),
@@ -127,9 +126,8 @@ class RHRegistrationsListManage(RHManageRegFormBase):
                 _('Download Attachments (flat)'),
                 'attachment',
                 type='href-custom',
-                url=url_for('.registrations_attachments_export', regform),
-                params={'flatten': True},
-                weight=60,
+                url=url_for('.registrations_attachments_export', regform, flatten=True),
+                weight=59,
                 extra_classes='js-submit-list-form regform-download-attachments',
             ),
             ActionMenuEntry(
@@ -909,7 +907,7 @@ class RHRegistrationsExportAttachments(ZipGeneratorMixin, RHRegistrationsExportB
 
     @use_kwargs({
         'flatten': fields.Boolean(load_default=False),
-    })
+    }, location='query')
     def _process_args(self, flatten):
         RHRegistrationsExportBase._process_args(self)
         self.flatten = flatten

@moliholy
Copy link
Contributor Author

moliholy commented Nov 13, 2024

I don't know if this is the best way, but you could pass the flatten param directly in url_for as a query string. That way, you don't need to change the js callback at all:

This is a POST. AFAIK it's not a good practice to mix both query parameters and body parameters. I don't have an strong opinion against it, and It's fine for me if it solves the issue. I simply thought that wouldn't be accepted.

@ThiefMaster
Copy link
Member

ThiefMaster commented Nov 13, 2024 •
edited
Loading

I don't see a reason against mixing them, especially if it means not adding custom JS. The only reason we use POST there is because the number of IDs may not fit in a query string.

@moliholy
Copy link
Contributor Author

moliholy commented Nov 13, 2024

@ThiefMaster OK, all done. Ready for review.

tomasr8
tomasr8 reviewed Nov 13, 2024
View reviewed changes
@ThiefMaster ThiefMaster force-pushed the feat/flatten-attachments branch from 26a1031 to 18738fb Compare November 20, 2024 11:12
@ThiefMaster ThiefMaster force-pushed the feat/flatten-attachments branch from 18738fb to 33ae7e6 Compare November 20, 2024 11:14
@ThiefMaster ThiefMaster added this to the v3.3 milestone Nov 20, 2024
@ThiefMaster ThiefMaster enabled auto-merge (squash) November 20, 2024 11:15
@ThiefMaster ThiefMaster merged commit 122c378 into indico:master Nov 20, 2024
9 checks passed
AjobK pushed a commit to AjobK/indico that referenced this pull request Dec 19, 2024
@moliholy @AjobK
Add option to flatten reg attachment archive (indico#6608)
8e8f725
AjobK pushed a commit to AjobK/indico that referenced this pull request Jan 7, 2025
@moliholy @AjobK
Add option to flatten reg attachment archive (indico#6608)
175fba5
AjobK pushed a commit to AjobK/indico that referenced this pull request Jan 7, 2025
@moliholy @AjobK
Add option to flatten reg attachment archive (indico#6608)
51f55f7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tomasr8 tomasr8 tomasr8 left review comments

@ThiefMaster ThiefMaster ThiefMaster approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v3.3

Development

Successfully merging this pull request may close these issues.

Flatten file structure in registration attachment download

3 participants

@moliholy @tomasr8 @ThiefMaster