Skip to content
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

add the key column feature to update existing documents #64

Merged
merged 15 commits into from
Jun 5, 2024

Conversation

ETLaurent
Copy link
Contributor

@ETLaurent ETLaurent commented May 20, 2024

Summary

Adds

  • Add the possibility to set a key column in your import CSV file in order to update existing pieces and pages.
    Thanks to this, this module reaches parity with the deprecated @apostrophecms/piece-type-importer module.

Fixes

  • We can now import pieces or pages with an import file that contains just the required fields. Before that, CSV import files had to contain all the document attributes, which was only the case when importing a file previously exported using this module.

See PR comments.

  • TODO: fix mocha tests
  • TODO: add cypress tests for the column key feature and the fix mentioned in the changelog

What kind of change does this PR introduce?

(Check at least one)

  • Bug fix
  • New feature
  • Refactor
  • Documentation
  • Build-related changes
  • Other

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:

@ETLaurent ETLaurent marked this pull request as ready for review May 22, 2024 09:54
@ETLaurent ETLaurent added the wip label May 22, 2024
@ETLaurent ETLaurent changed the title update doc thanks to an update key add the key column feature to update existing documents May 22, 2024
// If an update key is found, we try to update the document.
// It also means that we are in a "simple" import (CSV or Excel),
// not in a full import process like Gzip with JSON formats.
if (updateKey) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if an update key is found, we call self.insertOrUpdateDocWithKey rather than self.insertOrUpdateDoc because it has its own logic.


if (!matchingDraft && !matchingPublished) {
return insert();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if neither the draft nor the published version matched the column key value, then we insert the doc.

aposDocId: matchingDraft.aposDocId
})
.toObject();
}
Copy link
Contributor Author

@ETLaurent ETLaurent May 23, 2024

Choose a reason for hiding this comment

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

update all versions, even if one does not match the column key value.

Comment on lines 435 to 436
// Manually set `modified: false` because `setModified` option is not taken into account
// in the `update` method.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes...

}
}

self.handleRichTextFields(manager, doc);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

was not taken into account when importing a CSV (or Excel file)

return method === 'update'
? manager[method](_req, doc, { setModified: false })
: manager[method](_req, '_home', 'lastChild', doc, { setModified: false });
await manager.convert(_req, doc, docToImport);
Copy link
Contributor Author

@ETLaurent ETLaurent May 23, 2024

Choose a reason for hiding this comment

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

same as above, we need to convert the doc for "not-fully-exported" CSV entries like:

type,title,main,openGraphTitle
default-page,d2,<p><em>rich</em> <strong>text</strong> 2</p>,some text 2

i.e "man-made" import files, not created from our import-export module

@ETLaurent ETLaurent requested a review from boutell May 23, 2024 16:28
@ETLaurent
Copy link
Contributor Author

@boutell The PR just need some adaptation in the mocha tests to pass, but the general code is reviewable

@boutell
Copy link
Member

boutell commented May 28, 2024

Strategy feels good, like the attention to detail in various scenarios

@ETLaurent ETLaurent removed the wip label Jun 4, 2024
@ETLaurent ETLaurent merged commit 661d6d5 into main Jun 5, 2024
9 checks passed
@ETLaurent ETLaurent deleted the pro-5941-update-key branch June 5, 2024 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants