-
Notifications
You must be signed in to change notification settings - Fork 592
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
make changes for import/export module to work properly #4270
Conversation
PRO-4261 As an editor, I can download a ZIP file of my exported documents
When clicking on the Two new routes This method should take the IDs of the documents the user want to export, as well as the related types that he wants to get. Only the types, not the IDs, since we decided that it would be too heavy for the export modal to get the amount of related docs when we batch a large amount of documents. Generate filesThis method will need to use the Since we need to store documents in a flat EJSON array, it might be a good idea to add an option to this method in order to get a flat array of related documents instead of filling requested documents with their related docs, see how it could potentially be used with query builders. We want one EJSON file for each db collection, for example We want, for each document to export the draft and live versions (only draft if there is no live). The draft version must appear first to avoid situation where a live exists without draft which is forbidden. Same thing for related documents, they must be written first in the EJSON file, otherwise the doc manager will yell when we'll import it. We will create an Generate ZipTo generate the ZIP file we would prefer using the native node library. Progress bar notificationThis progress bar notification is triggered by wrapping your export function with the
The Finally the ZIP file will be sent back to the client and the download triggered from the user's browser. PermissionsFor exporting documents, a user that has at least one per-type permission should be able to export anything. He won’t be able to see For Reference https://github.com/apostrophecms/tech-designs/blob/main/3.x/share-documents-across-sites/design.md Acceptance Criteria
|
d5171df
to
30ab5da
Compare
@@ -432,7 +437,11 @@ module.exports = { | |||
} | |||
if (self.isSized(extension)) { | |||
// For images we correct automatically for common file extension mistakes | |||
const result = await Promise.promisify(self.uploadfs.copyImageIn)(file.path, '/attachments/' + info._id + '-' + info.name, { sizes: self.imageSizes }); | |||
const result = await Promise.promisify(self.uploadfs.copyImageIn)( |
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.
only indentation.
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.
Small things hopefully
@@ -402,16 +401,21 @@ module.exports = { | |||
extensions: accepted.join(req.t('apostrophe:listJoiner')) | |||
})); | |||
} | |||
|
|||
if (options.attachmentId && await self.apos.attachment.db.findOne({ _id: options.attachmentId })) { | |||
throw self.apos.error('invalid', 'duplicate'); |
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 could hit a race condition and imposes an unnecessary delay, try/catch the insert and check for self.apos.doc.isUniqueError(err)
to see if mongodb threw a unique key error or something else.
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 I kept the behavior as it is for now, we might create a ticket to update slightly the insert method in order to insert the doc before to copy to file to uploadfs.
At least here if a user use the option attachmentId
(I don't see a lot of use case) they are protected from a mess (file created while the document has not been inserted).
await self.db.deleteOne({ _id: existing._id }); | ||
await self.insert(req, file, { | ||
attachmentId: attachment._id, | ||
docIds: attachment.docIds, |
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.
Merge the old and new docIds and archivedDocIds so you have a complete list. Conversely, use $in
queries with an { _id: 1 }
projection to confirm they all actually exist on the receiving site and filter out those that don't. (I assume you're doing all of the attachments last after the docs so that this would give a complete result.)
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 know we are pressed for time. There is also a much simpler alternative re: tracking docIds and archivedDocIds. Just use the recomputeAllDocReferences
task programmatically at the end of the entire import process. It takes a little time, but and it will definitely get the right result without any tricky bookkeeping. It is 100% OK to do that for this first release at least, if you find it difficult to get the lists right per the technique above.
You can invoke a task internally but I'd just refactor its logic to a method for convenience (with no other changes).
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.
Thanks for having spotted that. I now only fill these arrays with existing docIds
and archivedDocIds
and the new ones that exist in the current site based on if the document is archived or not.
@@ -56,6 +56,7 @@ | |||
:labels="moduleLabels" | |||
:disable="relationshipErrors === 'min'" | |||
:displayed-items="items.length" | |||
:checked="checked" |
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.
Huh, was this just an unrelated bug?
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.
Not a bug, it has been added to be consistent with the other vue changes.
Basically the modal for exporting documents needed the checked IDs so we pass it to AposDocsManagerToolbar
when used.
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.
You also need to merge in the docIds and archivedDocIds from existing
because the attachment could be present in other documents on the receiving site.
CHANGELOG.md
Outdated
### Add | ||
|
||
* Allows to insert attachments with a given ID, as well as with `docIds` and `atchiveDocIds` to preserve related docs. | ||
* Adds an `update` method to the attachment module, that update the mongoDB doc and the associated file. |
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.
* Adds an `update` method to the attachment module, that update the mongoDB doc and the associated file. | |
* Adds an `update` method to the attachment module, that updates the mongoDB doc and the associated file. |
CHANGELOG.md
Outdated
|
||
### Add | ||
|
||
* Allows to insert attachments with a given ID, as well as with `docIds` and `atchiveDocIds` to preserve related docs. |
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.
* Allows to insert attachments with a given ID, as well as with `docIds` and `atchiveDocIds` to preserve related docs. | |
* Allows to insert attachments with a given ID, as well as with `docIds` and `archiveDocIds` to preserve related docs. |
...existing.docIds, | ||
...existing.archivedDocIds, | ||
...attachment.docIds, | ||
...attachment.archivedDocIds |
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.
@boutell is this what you meant?
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.
Yes sorry
PRO-4271
Summary
Brings some changes in core that are necessary for the new Import/Export module.
Must be tested with this PR.
Allows to insert attachment with a given ID (to import attachments with same IDs). As well as with
docIds
andarchiveDocIds
to preserve related docs.Adds an update method to the attachment module in order to allow a user that choose to override a piece with attachment to delete the existing one and reinsert it (the document in DB and the associated file). It deletes and recreates to reuse existing code and avoid big changes here, since the insert method does a lot of things.
Adds an option to the http remote method to allow receiving the original response from
node-fetch
, it allows to manipulate the response as a stream before it has been consumed in the method byres.text()
we need it to download images through streams and push them in the compressed file.What are the specific steps to test this change?
See steps in this PR.
What kind of change does this PR introduce?
Make sure the PR fulfills these requirements: