feat(comment-file): file upload delete read #550

ptamarit · 2026-01-22T15:16:57Z

Partially fixes #489

zzacharo · 2026-01-23T08:33:17Z

invenio_requests/alembic/1763728177_create_request_files_table.py

+        ["object_version_id"],
+        unique=False,
+    )
+    # TODO: Is ix_request_files_record_id needed? (We do not have it in Zenodo prod for communities)


Oh, I removed it in the 2nd commit instead of the first one, my bad.
And indeed, I checked and I don't think that these indexes make sense.

We decided that commits will be squashed.

Same for comment below.

zzacharo · 2026-01-23T08:34:54Z

invenio_requests/alembic/1763728177_create_request_files_table.py

+    # op.create_index(
+    #     "uidx_request_files_id_key", "request_files", ["id", "key"], unique=True
+    # )
+    # TODO: Is uidx_request_files_record_id_key needed? (We do not have it in Zenodo prod for communities, but instead have uidx_request_files_id_key)


invenio_requests/records/jsonschemas/requests/request-v1.0.0.json

invenio_requests/records/api.py

zzacharo · 2026-01-23T09:04:44Z

invenio_requests/services/files/config.py

+            yield projection
+
+
+class RequestFileLink(Link):


This will have to change after @fenekku changes regarding links. @fenekku would there be any guide on how to move to the new way of defining links?

Guidance on the change would be to:

replace Link by EndpointLink imported with from invenio_records_resources.services import EndpointLink

replace the "{api}/..." by the name of the endpoint

add params=[...] argument with string name of the route variables

define vars=lambda obj, vars: ... to update vars with those route variables

You can see: https://github.com/inveniosoftware/invenio-requests/pull/522/changes#diff-1dc8e4a7059ecea92c9ede417ea4f8e769f1963c9d94a7f37d8e80d3e4d8222eR131 for the Request work. (and invenio-records-resources and so on for various takes)

@ptamarit I think we need to note down the above so that we upgrade the links after the next major release including the above changes. @fenekku we plan to release this feature first as part of the current major release so that we avoid having to bump to the major including your work on links refactoring among others.

Can you clarify that part:

we plan to release this feature first as part of the current major release so that we avoid having to bump to the major including your work on links refactoring among others.

given the below: ?

The links change in this repo will necessitate a major bump of invenio-requests. The UTC change that Christoph will apply: #487 would also necessitate a major bump of this repo. The current plan, was to get the Links and UTC changes as part of the same major bump.

Either this PR can be part of that same major bump or it can be part of its own separate major bump (ordering to your liking).

We will release this as part of the major bump inveniosoftware/invenio-app-rdm@c540fa1 which includes already the commenting features. Of course, after you bump the major with the Links and UTC we will need to include the feature applying the changes as you pointed out in https://github.com/inveniosoftware/invenio-requests/pull/522/changes#diff-1dc8e4a7059ecea92c9ede417ea4f8e769f1963c9d94a7f37d8e80d3e4d8222eR131 .

We want to do this because we dont want to block you but also Zenodo wants to deploy all the commenting features rather soon without having to bump to the changes you introduce with Christoph. @ptamarit correct me if I am wrong :)

Got it. If you want to get commenting and files sooner, it makes sense to do your own major bump. That major bump will have to be incremented again on invenio-app-rdm once my work and @utnapischtim's work is merged and released.

We had done our own last December so we will continue in that one as a maint release if you are faster than us :)

invenio_requests/services/files/config.py

invenio_requests/services/files/service.py

zzacharo · 2026-01-23T09:38:29Z

invenio_requests/config.py

 Additional replies can be loaded via pagination.
 """
+
+REQUESTS_FILES_DEFAULT_QUOTA_SIZE = 100 * 10**6  # 100MB


Make sure there is some nice error when we reach that in the UI

I just tested, and we get an HTTP status code 400 and the following response:
{'message': 'Bucket quota exceeded.', 'status': 400}

(This makes me think that my explicit check and custom exception RequestFileSizeLimitError are not that useful, since the underlying bucket already takes care of returning errors which are properly handled.)

Unless, we want to have a specific error for comments when this bubbles up

invenio_requests/resources/files/resource.py

zzacharo · 2026-01-23T15:29:00Z

invenio_requests/services/files/service.py

+            raise RequestFileNotFoundError()

-        return deleted_file.file.dumps()
+        return request_file.file


Return it via result item?

Using file_result_item in the last commit (different than the 2 other results, so that we can rely on the send_file implemented by FileService).

ptamarit · 2026-01-23T17:03:08Z

FYI, I disabled assertions in the Alembic test which are failing since SQLAlchemy-Continuum 1.6.0 (not related to changes on this PR, failing in master too).

ptamarit · 2026-01-26T09:55:41Z

Just squashed commits before merging.

ptamarit requested a review from zzacharo January 22, 2026 15:16

ptamarit force-pushed the 489-comment-file-attachment branch 3 times, most recently from 54697d7 to 0d5843d Compare January 22, 2026 17:37

zzacharo approved these changes Jan 23, 2026

View reviewed changes

zzacharo reviewed Jan 23, 2026

View reviewed changes

ptamarit commented Jan 23, 2026

View reviewed changes

invenio_requests/resources/files/resource.py Outdated Show resolved Hide resolved

zzacharo reviewed Jan 23, 2026

View reviewed changes

ptamarit changed the title ~~feat(comment-file): file upload and delete and get content~~ feat(comment-file): file upload delete read Jan 23, 2026

ptamarit force-pushed the 489-comment-file-attachment branch from d4a0708 to 76a06fd Compare January 23, 2026 17:02

ptamarit added 2 commits January 26, 2026 10:54

feat(comment-file): file upload delete read

ba40408

fix: tests: disable failing Alembic assertions

5e51ffb

ptamarit force-pushed the 489-comment-file-attachment branch from 76a06fd to 5e51ffb Compare January 26, 2026 09:55

ptamarit merged commit 1969df2 into inveniosoftware:feature/comments Jan 26, 2026
4 checks passed

feat(comment-file): file upload delete read #550

feat(comment-file): file upload delete read #550

Uh oh!

Conversation

ptamarit commented Jan 22, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ptamarit Jan 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fenekku Jan 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ptamarit commented Jan 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ptamarit commented Jan 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ptamarit Jan 23, 2026 •

edited

Loading

fenekku Jan 23, 2026 •

edited

Loading

ptamarit commented Jan 23, 2026 •

edited

Loading