add option to import documents only as drafts #95

ETLaurent · 2024-10-18T13:34:53Z

Summary

What are the specific steps to test this change?

Check the option to import everything in draft only.
Verify that new documents should only be imported as draft, and existing documents should only have their draft version updated with the published imported version.

What kind of change does this PR introduce?

(Check at least one)

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

If adding a new feature without an already open issue, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

linear · 2024-10-18T13:34:57Z

PRO-6696 [P1] Export-Import: As a user, I should have the option of importing all documents in draft mode

lib/methods/import.js

ETLaurent · 2024-10-25T12:38:25Z

test/overrideDuplicates.js

+  this.timeout(t.timeout);
+


for slow computers...

ETLaurent · 2024-10-26T12:49:34Z

cypress no regression test: https://github.com/apostrophecms/testbed/actions/runs/11531890614/

ValJed

Globally good, few style feedback.
One tiny issue noticed, when importing draft that has no changes with the existing document, it's marked pending update but it cannot be updated (can discard draft but maybe unclear to users).

Two solutions:

Running detectDocChange to each imported document with existing one (if existing one). Might be heavy to add this step.
Updating detectDocChange to consider there are difference between draft and live (even if there is not) when modified: true to allow users to review and update to remove the modified flag and the associated label.

ValJed · 2024-10-28T15:48:03Z

lib/methods/import.js

+        // i.e "is draft without a published version"
+        const isDraftAlone =
+          doc.aposMode === 'draft' &&
+          !docs.find(item => item.aposDocId === doc.aposDocId && item.aposMode === 'published');


You could use some here that return a boolean instead of a document like find.

ValJed · 2024-10-28T15:51:13Z

lib/methods/import.js

@@ -491,6 +533,10 @@ module.exports = self => {
          fetchRelationships: false
        });

+        if (importDraftsOnly) {
+          delete docToInsert.lastPublishedAt;
+        }


Maybe this could be done in the function getPublishedDocsAsDraft to avoid spreading the logic?

function convertToDraft(doc) { if (doc.aposMode === 'published') { doc._id = doc._id?.replace('published', 'draft'); doc.aposLocale = doc.aposLocale?.replace('published', 'draft'); doc.aposMode = 'draft'; } delete doc.lastPublishedAt; return doc; }

If you want to stay "functional":

function convertToDraft({ lastPublishedAt, ...doc }) { if (doc.aposMode === 'published') { return { ...doc, _id: doc._id?.replace(':published', ':draft'), aposLocale: doc.aposLocale?.replace(':published', ':draft'), aposMode: 'draft' }; } return doc; }

I think these lines should exist in the function convertToDraft.

ValJed · 2024-10-28T15:58:44Z

lib/methods/import.js

+          await self.editDocMongoFields(matchingDraft, {
+            $set: {
+              modified: Boolean(importDraftsOnly)
+            }


Would have put the $set part into the function. Or just have renamed setDocModified to take a boolean.

I removed the function, as it was not really necessary.
Using the mongo connector directly is way clearer I think.

ValJed · 2024-10-28T16:00:29Z

lib/methods/import.js

@@ -647,6 +710,10 @@ module.exports = self => {
          fetchRelationships: false
        });

+        if (importDraftsOnly) {
+          delete docToInsert.lastPublishedAt;
+        }


Here too, could be done inside getPublishedDocsAsDraft.

? See my comment above to make it functional inside convertToDraft

ValJed · 2024-10-28T16:03:37Z

lib/methods/import.js

@@ -792,7 +861,7 @@ module.exports = self => {

      const { docs, attachmentsInfo = [] } = await format.input(exportPath);

-      const filterDocs = docs.filter(doc => docIds.includes(doc.aposDocId));
+      let filterDocs = docs.filter(doc => docIds.includes(doc.aposDocId));


Could avoid reassigning:

const filterDocs = importDraftsOnly ? self.getPublishedDocsAsDraft( docs.filter(doc => docIds.includes(doc.aposDocId)) ) : docs.filter(doc => docIds.includes(doc.aposDocId));

done, and filtering now happens before locale re-writing

boutell · 2024-10-29T12:32:01Z

detectDocChange is pretty fast. I think we do need to do it for the reason you mentioned. We can't leave this in a weird state or expect the user to disentangle it. It must "just work" in the most sensible way.

…

On Mon, Oct 28, 2024 at 12:46 PM Jed ***@***.***> wrote: ***@***.**** requested changes on this pull request. Globally good, few style feedback. One tiny issue noticed, when importing draft that has no changes with the existing document, it's marked pending update but it cannot be updated (can discard draft but maybe unclear to users). Two solutions: - Running detectDocChange to each imported document with existing one (if existing one). Might be heavy to add this step. - Updating detectDocChange to consider there are difference between draft and live (even if there is not) and allow user to review and update to remove the modified flag and the associated label. ------------------------------ In lib/methods/import.js <#95 (comment)> : > @@ -248,6 +256,37 @@ module.exports = self => { } }, + // Get only the published docs and convert them into draft. + // If a doc is a draft and has no published version, we keep it. + // If a doc has no aposMode, we keep it. + getPublishedDocsAsDraft(docs) { + return docs + .filter(isPublishedOrDraftAlone) + .map(convertToDraft); + + function isPublishedOrDraftAlone(doc) { + // i.e "is draft without a published version" + const isDraftAlone = + doc.aposMode === 'draft' && + !docs.find(item => item.aposDocId === doc.aposDocId && item.aposMode === 'published'); You could use some here that return a boolean instead of a document like find. ------------------------------ In lib/methods/import.js <#95 (comment)> : > @@ -491,6 +533,10 @@ module.exports = self => { fetchRelationships: false }); + if (importDraftsOnly) { + delete docToInsert.lastPublishedAt; + } Maybe this could be done in the function getPublishedDocsAsDraft to avoid spreading the logic? function convertToDraft(doc) { if (doc.aposMode === 'published') { doc._id = doc._id?.replace('published', 'draft'); doc.aposLocale = doc.aposLocale?.replace('published', 'draft'); doc.aposMode = 'draft'; } delete doc.lastPublishedAt; return doc; } ------------------------------ In lib/methods/import.js <#95 (comment)> : > @@ -547,7 +603,13 @@ module.exports = self => { : await manager.update(_req, docToUpdate); } - await self.setDocAsNotModified(matchingDraft); + if (matchingDraft) { + await self.editDocMongoFields(matchingDraft, { + $set: { + modified: Boolean(importDraftsOnly) + } Would have put the $set part into the function. Or just have renamed setDocModified to take a boolean. ------------------------------ In lib/methods/import.js <#95 (comment)> : > @@ -647,6 +710,10 @@ module.exports = self => { fetchRelationships: false }); + if (importDraftsOnly) { + delete docToInsert.lastPublishedAt; + } Here too, could be done inside getPublishedDocsAsDraft. ------------------------------ In lib/methods/import.js <#95 (comment)> : > @@ -792,7 +861,7 @@ module.exports = self => { const { docs, attachmentsInfo = [] } = await format.input(exportPath); - const filterDocs = docs.filter(doc => docIds.includes(doc.aposDocId)); + let filterDocs = docs.filter(doc => docIds.includes(doc.aposDocId)); Could avoid reassigning: const filterDocs = importDraftsOnly ? self.getPublishedDocsAsDraft( docs.filter(doc => docIds.includes(doc.aposDocId)) ) : docs.filter(doc => docIds.includes(doc.aposDocId)); — Reply to this email directly, view it on GitHub <#95 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAH27O46ZIRPOGRJSEG7S3Z5ZL65AVCNFSM6AAAAABQF7FBYGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGOJZGQ4TKNZZHA> . You are receiving this because your review was requested.Message ID: ***@***.***>

-- THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his

ETLaurent · 2024-10-30T11:29:12Z

cypress tests: https://github.com/apostrophecms/testbed/pull/304

ValJed

Tested, seems working, just a tiny comment about moving the logic that removes lastPublishedAt in the function that converts to draft.
Otherwise 👍🏼

ValJed · 2024-11-04T09:05:20Z

lib/methods/import.js

+          await self.editDocMongoFields(matchingDraft, {
+            $set: {
+              modified: Boolean(importDraftsOnly)
+            }


ValJed · 2024-11-04T09:05:40Z

lib/methods/import.js

@@ -491,6 +533,10 @@ module.exports = self => {
          fetchRelationships: false
        });

+        if (importDraftsOnly) {
+          delete docToInsert.lastPublishedAt;
+        }


ValJed · 2024-11-04T09:05:45Z

lib/methods/import.js

@@ -647,6 +710,10 @@ module.exports = self => {
          fetchRelationships: false
        });

+        if (importDraftsOnly) {
+          delete docToInsert.lastPublishedAt;
+        }


? See my comment above to make it functional inside convertToDraft

ValJed · 2024-11-04T09:08:15Z

lib/methods/import.js

@@ -491,6 +533,10 @@ module.exports = self => {
          fetchRelationships: false
        });

+        if (importDraftsOnly) {
+          delete docToInsert.lastPublishedAt;
+        }


If you want to stay "functional":

function convertToDraft({ lastPublishedAt, ...doc }) { if (doc.aposMode === 'published') { return { ...doc, _id: doc._id?.replace(':published', ':draft'), aposLocale: doc.aposLocale?.replace(':published', ':draft'), aposMode: 'draft' }; } return doc; }

I think these lines should exist in the function convertToDraft.

ui

5c571ca

ETLaurent added 3 commits October 18, 2024 17:21

oui

93c0e7e

wip

b2d029b

typo

5a27d3a

ETLaurent commented Oct 18, 2024

View reviewed changes

lib/methods/import.js Outdated Show resolved Hide resolved

ETLaurent added 3 commits October 21, 2024 15:16

add missing parts

c60341e

oui

81668eb

insert a doc with key only in draft

f0d4099

ETLaurent changed the title ui add option to import documents only as drafts Oct 22, 2024

ETLaurent added 14 commits October 22, 2024 15:32

simplify conditions using "published"

cd662fe

re-work

5a25ea0

make it work with man-made exports

c146506

use splice

728102b

use filter

09cb7e3

align indicator

b868628

set modified only on drafts

9caff0b

add timeouts

6c0e096

test wip[

b2f139b

add tests

c640a1e

test lastPublishedAt

31c71ed

one last test to do

a280ffa

test update

d716aba

changelog

b104d4e

ETLaurent commented Oct 25, 2024

View reviewed changes

test/overrideDuplicates.js

Comment on lines +9 to +10

this.timeout(t.timeout);

Copy link

Contributor Author

ETLaurent Oct 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for slow computers...

ETLaurent added 2 commits October 25, 2024 14:38

align

b2f02f8

readme

8f0f546

ETLaurent marked this pull request as ready for review October 25, 2024 12:53

stylelint

48bc9e4

ETLaurent requested a review from boutell October 25, 2024 12:54

ETLaurent requested a review from ValJed October 25, 2024 13:40

ValJed requested changes Oct 28, 2024

View reviewed changes

ETLaurent mentioned this pull request Oct 29, 2024

detectDocChange now returns true when one of the docs contains modified: true apostrophecms/apostrophe#4783

Closed

10 tasks

ETLaurent added 3 commits October 30, 2024 16:47

find -> some

99ca648

remove editDocMongoFields

5105866

filter docs before rewriting the locale

3491452

boutell approved these changes Oct 30, 2024

View reviewed changes

ETLaurent added 4 commits October 31, 2024 11:50

wip isEqual

95c83d6

set draft as modified only if it differs from published

5dfcd5c

fix eslint

e97ffdd

test modified false

9665fc2

ETLaurent requested review from ValJed and boutell October 31, 2024 15:23

ValJed approved these changes Nov 4, 2024

View reviewed changes

ETLaurent merged commit 34a2f5e into main Nov 4, 2024
9 checks passed

ETLaurent deleted the pro-6696-import-as-draft branch November 4, 2024 10:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add option to import documents only as drafts #95

add option to import documents only as drafts #95

ETLaurent commented Oct 18, 2024 •

edited

Loading

linear bot commented Oct 18, 2024

ETLaurent Oct 25, 2024

ETLaurent commented Oct 26, 2024

ValJed left a comment •

edited

Loading

ValJed Oct 28, 2024

ValJed Oct 28, 2024

ValJed Nov 4, 2024

ValJed Nov 4, 2024

ValJed Oct 28, 2024

ETLaurent Oct 31, 2024

ValJed Nov 4, 2024

ValJed Oct 28, 2024

ValJed Nov 4, 2024

ValJed Oct 28, 2024

ETLaurent Oct 31, 2024

boutell commented Oct 29, 2024 via email

ETLaurent commented Oct 30, 2024

ValJed left a comment

ValJed Nov 4, 2024

ValJed Nov 4, 2024

ValJed Nov 4, 2024

ValJed Nov 4, 2024

add option to import documents only as drafts #95

add option to import documents only as drafts #95

Conversation

ETLaurent commented Oct 18, 2024 • 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 Oct 18, 2024

Choose a reason for hiding this comment

ETLaurent commented Oct 26, 2024

ValJed left a comment • 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

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

boutell commented Oct 29, 2024 via email

ETLaurent commented Oct 30, 2024

ValJed 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

ETLaurent commented Oct 18, 2024 •

edited

Loading

ValJed left a comment •

edited

Loading