-
Notifications
You must be signed in to change notification settings - Fork 5
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.
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
Rework SIP Package generation and download #5837
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions/comments, 👍 apart from that!
package = SIPPackage(self) | ||
zip_file = self.create_zipfile(package) | ||
zip_file.seek(0) | ||
return NamedBlobFile(zip_file.read(), contentType='application/zip') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could pass a reference to the file instead of reading the file into memory yourself. That triggers an plone.namedfile.interfaces.IStorage
adapter, which should be able to handle the file instance. If we are lucky it can be moved on the filesystem without reading the whole thing into memory. Not 100% sure but worth a try.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not found a way to pass in a file in to a NamedBlobFile, we would need to register a IStorage Adapter for __builtin__.file
what I'm not sure is the right way. For now i did no change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as discussed today, if it does not work by default i'm ok with not changing the current implementation 👍.
""" | ||
|
||
def __call__(self): | ||
self.context.store_sip_package() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This view will respond to a GET
request. do you see a way to create a form which only responds to an i.e. POST
, as it will modify data in the database.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do the maintenance-actions as discussed in a separate PR, I added a checkbox to the Main Issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have added a checkbox to the main issue in #5167.
opengever/disposition/locales/de/LC_MESSAGES/opengever.disposition.po
Outdated
Show resolved
Hide resolved
06f8153
to
d31a58f
Compare
Store SIP package as a blob locally instead of re generating them for each download. This solves time and ressources and makes it possible to provide customer specific scripts and SIP package handling.
The export view stays still available, but is no longer available trough the UI, can be useful for debugging purposes.
d31a58f
to
1da11eb
Compare
@deiferni updated according to your feedback. The maintenance-action I'll do in a separate PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
This is the first part of the work we do for #5167, a short summary of the changes this PR does.
Checkliste
Zutreffendes soll angehakt stehengelassen werden.