-
-
Notifications
You must be signed in to change notification settings - Fork 286
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
Give pending entities BBIDs and treat them the same way as regular entities #1136
Open
kellnerd
wants to merge
32
commits into
metabrainz:import-entities
Choose a base branch
from
kellnerd:entity-import-bbid
base: import-entities
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add a boolean column to distinguish pending imports and accepted entities and use it for all entity views. Stop using a separate import table and adapt foreign key constraints. Now that imported entities have BBIDs instead of numeric IDs, they can finally have relationships (either with other pending imports or with already accepted entities).
As the last commit is a breaking change already, we may as well do that. - The whole `link_import` table stores the import's metadata. - In contrast the JSONB column only stores additional freeform data. - Origin and source are somewhat redundant terms, the table contains the names of external data sources, such as OpenLibrary - `origin_id` (for example an OpenLibrary ID) contains the corresponding external ID, prefer the expanded term "identifier" as `*_id` is usually used for foreign keys
Meaningful values for (pending_entity_bbid, accepted_entity_bbid): 1. (A, null) = new pending import 2. (A, A) = accepted import 3. (null, null) = discarded import 4. (B, A) = update is pending for accepted entity Theoretically (null, A) could be used to seed the table with manually added entities which already have the given external identifier.
If the existing DB already has legacy import tables with testdata, simply run `down.sql` before `up.sql` to start from scratch.
Make column names and order of the import views compatible.
The latter is more precise and has the advantage not to to be a keyword.
We want to display more of these props, especially the external ID.
Again using the author router for testing
Approving and discarding an import via the unified entity routes and display pages is now working and the import specific routes/pages will be removed in one of the next steps.
Previously a `FormSubmissionError('No Updated Field')` could crash the entire server instead of just showing the message to the user. Now we can also remove the catch-log-rethrow anti-pattern which only caused a duplicate log message with Express already handling this.
- Approve the import for an initial revision before creating a new "Edit" revision as usual - Exit early if the entity is approved without additional changes
In case we want to create a new annotation, we initially have to create it without an associated revision and can later update `latestRevision`. Without this patch, the entity editors can not be used to cleanly "approve" pending entities without creating an empty revision. An alternative idea which has been considered was to rollback the transaction in that case, but this has annoying side effects: - The ID of the rolled back revision is still lost and will be unused. - It seems impossible to return the main entity for indexing.
Although a lot of work has been put into multiple iterations of these, this import-specific stuff is now no longer needed (and partly broken). All of the deleted features (and even more) have been integrated into the regular entity routes and display pages.
This was referenced Nov 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Pending entities are identified by a numeric ID instead of a BBID as regular entities.
Besides making it impossible for pending entities to be used in relationships, this requires separate logic to load data (for display or editing) and to store it (after approval).
Having this separate logic which is almost identical but shares almost no code with regular entities is very annoying as every change for an entity type has to be done in two places.
Solution
import
entry table into theentity
table (since they both have the same columns now)Now that imported entities have BBIDs instead of numeric IDs, they can finally have relationships (either with other pending imports or with already accepted entities).
Since we are using the combined entity views, we can also load imports into the entity editor without any changes! In order to make editing of pending entities (Edit & Approve) work, we only have to do minimal changes in the edit handler:
Finally I had the enormous pleasure to delete a ton of unused code (d9830e5).
Areas of Impact
Literally everything since I have tampered with the core schema and entity loading.
P.S. You might have to temporarily
yarn add kysely
in bb-site because apparentlyyarn link
does not take changed dependencies in your linked local bb-data version into account.