Pro 4261 export attachments #10

ValJed · 2023-08-24T13:22:25Z

Summary

Manage attachments through stream to be generated in the zip files.
A JSON file is generated with attachments documents.
A folder with actual downloaded files, stream from http GET request is passed to archive lib for each attachment.
Requests are done 5 by 5.
Error notifications, if an error occurs we archive the file isn't generated.
If we have some attachments issues, file is generated but we warn the user.

What are the specific steps to test this change?

Run exports with images or other attachments.

What kind of change does this PR introduce?

Make sure the PR fulfills these requirements:

It includes a) the existing issue ID being resolved, b) a convincing reason for adding this feature, or c) a clear description of the bug it resolves
The changelog is updated
Related documentation has been updated
Related tests have been updated

linear · 2023-08-24T13:22:29Z

PRO-4261 As an editor, I can download a ZIP file of my exported documents

When clicking on the download button of the export modal, after I selected the related docs that I want to include in the export, the process must be triggered.

image.png

Two new routes export must exist in the modules piece-type and page. Both routes will call a method export from the module doc-type .

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 files

This method will need to use the relate method from the module schema, this method fills documents with related ones.

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 aposDocs.json and aposAttachment.json, containing arrays of documents..

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 attachment folder when actual attachments will be stored (images, files…).

Generate Zip

To generate the ZIP file we would prefer using the native node library.

Progress bar notification

This progress bar notification is triggered by wrapping your export function with the run method of the job module. Here is an example from the piece-type-importer module:

return self.apos.modules['apostrophecms/job'].run(
  req,
  (req, reporting, { notificationId }) => self.importRun(req, {
    progressNotifId: notificationId,
    filePath: file.path,
    reporting,
    totalPieces
  }),
  {}
);

The reporting object passed from the second argument function allows to set the total of the documents you want to import with the method setTotal, as well as to call the methods success or failure for every document that we tried to export or import.

Finally the ZIP file will be sent back to the client and the download triggered from the user's browser.

Permissions

For 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 adminOnly documents, therefore should not be able to export these documents. It matches the way it works currently because in this case the user can see all drafts. In core, admin should be able to export adminOnly content, except for users.

For Reference

https://github.com/apostrophecms/tech-designs/blob/main/3.x/share-documents-across-sites/design.md

Acceptance Criteria

I'm an editor or admin on with the Download modal open, and my documents selected, and I hit "Download" and when I do, a ZIP file begins downloading.
The zip file downloads and I open it and I see the the draft and live versions (only draft if there is no live) in my unarchived folder.

…hments

…d of type

…rency

…hments

ETLaurent

neat! 👏
great implementation above the existing, happy to see that it fitted well.

some questions, some comments, few changes requested

ETLaurent · 2023-08-30T09:23:22Z

i18n/en.json

+  "exportFileGenerationError": "The {{ extension }} file generation failed",
+  "exportAttachmentError": "Some attachments could not be added to the {{ extension }} file",


alphabetical order, as you like it ;)

ETLaurent · 2023-08-30T09:25:59Z

lib/formats/archiver.js

+    const position = i * max;
+    return [
+      ...chunk,
+      ...attachments.slice(position, position + max)


we're sure we don't leave a promise out in the process?
unit-test it could be good to ensure that

Yes because of this line:
const length = Math.ceil(attachments.length / max);
we create as many arrays as needed chunks. The ceil allows to be sure we have a last array to put the last promises even if the don't fill it.
Could be tested indeed.

ETLaurent · 2023-08-30T09:31:04Z

lib/formats/archiver.js

+  for (const chunk of chunkedPromises) {
+    const results = await Promise.allSettled(chunk);
+    for (const { status } of results) {
+      if (status === 'rejected' && !attachmentError) {
+        attachmentError = true;
+      }


you can use .some rather than another for..of:

Suggested change

for (const chunk of chunkedPromises) {

const results = await Promise.allSettled(chunk);

for (const { status } of results) {

if (status === 'rejected' && !attachmentError) {

attachmentError = true;

}

for (const chunk of chunkedPromises) {

const results = await Promise.allSettled(chunk);

if (results.some(result => result.status === 'rejected')) {

attachmentError = true;

}

Nice one, updated

ETLaurent · 2023-08-30T09:35:07Z

lib/formats/archiver.js


-      archive.append(content, { name: filename });
+// If one image download fail should we stop the entire process?


no I don't think so, and your attachmentError is a good way of telling that one or several attachment failed, and still being able to generate the archive

Comment removed.

ETLaurent · 2023-08-30T09:49:52Z

lib/methods/archive.js

+          });
+        }
+      } catch (err) {
+        console.log(err);


Suggested change

console.log(err);

self.apos.error('error', error.message);

(let's rename err to error to be consistent in this module 🙏 )

Destructured to get message.

ETLaurent · 2023-08-30T09:59:33Z

lib/methods/export.js


-      const docs = await self.apos.doc.db
+      const docsIdsUniq = [ ...new Set(docsIds) ];
+      const docs = await self.apos[collection].db


ETLaurent · 2023-08-30T10:01:01Z

lib/methods/export.js

+          /* ( */
+          /*   type === 'relationship' && */
+          /*   field.withType && */
+          /*   self.apos.modules[field.withType].options.relatedDocument === false */
+          /* ) */


add a TODO: handle 'export: false' option

Added (but we should choose another option name)

I am handling this in fetchActualDocs, actually:

We should use another option name since this one is already used by piece-type-exporter and could occur unclear conflicts.
Also I think it would be more efficient to perform this check before the DB request if we can.

ok I'll handle that in #13

ETLaurent · 2023-08-30T10:02:22Z

lib/methods/export.js

+    }) {
+      return schema.flatMap(field => {
+        const fieldValue = doc[field.name];
+        const shouldRecurse = recursion < MAX_RECURSION;


Suggested change

const shouldRecurse = recursion < MAX_RECURSION;

const shouldRecurse = recursion <= MAX_RECURSION;

Hum since the recursion starts at 0, if I put <= I'll have 11 call of the function if I have a MAX_RECURSION of 10.
But semantically you're right, the first call isn't a recursion 🤔
Hum the universe is so complex, I need to let my body float in its cold matter, nothing makes sense.
let me modify this line of code

yeah but a 1 recursion means that we are passing a 2nd time in the function.
to me, "recursion" = "re-called".

that's what's been done in apiRoutes

ETLaurent · 2023-08-30T10:05:47Z

ui/apos/components/AposExportModal.vue

-      const moduleOptions = window.apos.modules[this.moduleName];
-      const label = this.checked.length > 1 ? moduleOptions.pluralLabel : moduleOptions.label;
+      const moduleOptions = apos.modules[this.moduleName];
+      const label = this.checked?.length > 1


Suggested change

const label = this.checked?.length > 1

const label = this.count > 1

if possible to use the computed below 👇

ETLaurent · 2023-08-30T10:07:18Z

ui/apos/components/AposExportModal.vue

@@ -141,7 +141,11 @@ export default {
    },
    checked: {
      type: Array,
-      default: () => []
+      default: null


why do you set it to null?
having an array by default avoids us having to make loads of conditions when checking the length

Because checked is used only in batch operations, otherwise we user this.doc.
We need a way to now we are in batch operation or in a single piece operation.
I could make a check on this.doc instead maybe.

I updated to use a data key selectedDocIds to store ids from this.doc or from this.checked

…hments

ETLaurent · 2023-08-30T13:53:19Z

i18n/en.json

@@ -1,5 +1,7 @@
 {
  "export": "Download {{ type }}",
+  "exportAttachmentError": "Some attachments could not be added to the {{ extension }} file",
+  "exportFileGenerationError": "The {{ extension }} file generation failed",


below exported 🔤

ETLaurent and others added 22 commits August 17, 2023 13:35

call export route

b4d55ba

do not include related types when toggle is off

84e0b6e

count computed

240743f

wip

766e112

retrieve related ids

0889929

use map

404be10

fetch all related docs and from every mode

93ff732

extract code to a getRelatedDocsIds method

3666b0f

refactor code, maybe too much

313cc87

fetch also related docs from published docs

3727d06

add small but important comment

a6fa62b

add todos

d434984

weg3grhrthtrh4et

ef7a790

accepts apos as a global var, removes window calls in UI

e4a598f

uses doc as prop

6c1ad74

get attachments ids

ba50486

wip working with reduce

6e9f43c

gets attachments and associated urls

a673be7

stores attachment name with associated url

92d530a

wip tests node-archiver to generate zip file

b44c140

poc on zip file generation

13b74e7

setup tests

a0c5a3c

ValJed self-assigned this Aug 24, 2023

ValJed marked this pull request as draft August 24, 2023 13:22

ValJed added 2 commits August 24, 2023 15:28

Merge remote-tracking branch 'origin/main' into pro-4261-export-attac…

5ec5458

…hments

adds attachments to zip files

a5f1016

ValJed force-pushed the pro-4261-export-attachments branch from e1dc4c2 to a5f1016 Compare August 25, 2023 07:39

ValJed added 2 commits August 28, 2023 15:46

adds recursion to getRelatedIdsBySchema method, check withType instea…

26f9532

…d of type

formats properly data, removes check on relatedDocuments option

b79e0d7

ValJed added 6 commits August 28, 2023 20:06

passes apos to compress function

8717f15

append attachments to archive, split promises into chunk limit concur…

d0e3147

…rency

minor changes

65221ec

adds error management

52f3613

Merge remote-tracking branch 'origin/main' into pro-4261-export-attac…

a5df6b6

…hments

adds extension to error notifications

8a09964

ValJed marked this pull request as ready for review August 29, 2023 15:21

ValJed requested review from ETLaurent and falkodev August 29, 2023 15:22

ETLaurent requested changes Aug 30, 2023

View reviewed changes

ValJed added 2 commits August 30, 2023 14:55

feedback

4b6bf2b

Merge remote-tracking branch 'origin/main' into pro-4261-export-attac…

213dcd1

…hments

ValJed requested a review from ETLaurent August 30, 2023 13:05

ETLaurent reviewed Aug 30, 2023

View reviewed changes

ETLaurent approved these changes Aug 30, 2023

View reviewed changes

ValJed merged commit a988c32 into main Aug 30, 2023
0 of 9 checks passed

ETLaurent deleted the pro-4261-export-attachments branch August 30, 2023 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pro 4261 export attachments #10

Pro 4261 export attachments #10

ValJed commented Aug 24, 2023 •

edited

Loading

linear bot commented Aug 24, 2023

Generate files

Generate Zip

Progress bar notification

Permissions

ETLaurent left a comment

ETLaurent Aug 30, 2023

ETLaurent Aug 30, 2023

ValJed Aug 30, 2023

ETLaurent Aug 30, 2023

ValJed Aug 30, 2023

ETLaurent Aug 30, 2023

ValJed Aug 30, 2023

ETLaurent Aug 30, 2023

ValJed Aug 30, 2023

ETLaurent Aug 30, 2023

ETLaurent Aug 30, 2023

ValJed Aug 30, 2023

ETLaurent Aug 30, 2023

ValJed Aug 30, 2023

ETLaurent Aug 30, 2023

ETLaurent Aug 30, 2023

ValJed Aug 30, 2023 •

edited

Loading

ETLaurent Aug 30, 2023

ETLaurent Aug 30, 2023

ETLaurent Aug 30, 2023

ValJed Aug 30, 2023

ValJed Aug 30, 2023

ETLaurent Aug 30, 2023

		"exportFileGenerationError": "The {{ extension }} file generation failed",
		"exportAttachmentError": "Some attachments could not be added to the {{ extension }} file",


		archive.append(content, { name: filename });
		// If one image download fail should we stop the entire process?

	const shouldRecurse = recursion < MAX_RECURSION;
	const shouldRecurse = recursion <= MAX_RECURSION;

	const label = this.checked?.length > 1
	const label = this.count > 1

Pro 4261 export attachments #10

Pro 4261 export attachments #10

Conversation

ValJed commented Aug 24, 2023 • edited Loading

Summary

What are the specific steps to test this change?

What kind of change does this PR introduce?

Make sure the PR fulfills these requirements:

linear bot commented Aug 24, 2023

Generate files

Generate Zip

Progress bar notification

Permissions

ETLaurent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValJed Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValJed commented Aug 24, 2023 •

edited

Loading

ValJed Aug 30, 2023 •

edited

Loading