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

Repeatable imports #314

Merged
merged 15 commits into from
Nov 8, 2023
Merged

Repeatable imports #314

merged 15 commits into from
Nov 8, 2023

Conversation

kellnerd
Copy link
Contributor

@kellnerd kellnerd commented Nov 5, 2023

Existing imports can now be updated if they are still pending. The implementation currently only overwrites the import header to link to the freshly created entity data and leaves all the old entity data behind orphaned. (Still to be improved...)

I've also made the options parameter more flexible to already include support for creating updates for accepted entities as well (which might be added in a future version.) In addition to the import ID, the createImport function now also returns its status (which is used by the consumer).

Also contains a few more fixes for type errors (in currently unused code) which I somehow had not seen so far.

Without them, the return type of `bookshelf.transaction()` is unusable.
Alternatively we could make `@metabrainz/bookshelf` depend on them, but
I don't want to put effort into that fork again.
Also drop a (harmless but redundant) duplicate conversion to snake case.
Only accepting a boolean flag to overwrite pending imports is no good
long term solution because a future flag to update already accepted
imports only makes sense if pending imports are also updated.
To account for all the possible outcomes of an import, the function
should return the import's status in addition to the ID.
Not all possible values will be supported by the initial version.
This feature currently only updates the import metadata and points the
import header to the freshly created data rows, but leaves the old data
of previous import attempts behind orphaned.
Knex does not seem to implement `.upsert()` for postgres.
@kellnerd kellnerd marked this pull request as ready for review November 7, 2023 12:34
Copy link
Member

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

Looking good !
And I look forward to seeing the continuation of this, you were right it's now getting more interesting !

src/func/imports/create-import.ts Show resolved Hide resolved
Copy link
Member

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

Thank you for the clarification !
🚀

@MonkeyDo MonkeyDo merged commit 67276af into metabrainz:master Nov 8, 2023
2 checks passed
@kellnerd kellnerd deleted the repeatable-imports branch November 8, 2023 13:51
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.

2 participants