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

Give pending entities BBIDs and treat them the same way as regular entities #1136

Open
wants to merge 32 commits into
base: import-entities
Choose a base branch
from

Conversation

kellnerd
Copy link
Contributor

@kellnerd kellnerd commented Nov 4, 2024

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

  1. Schema change to give pending imports a BBID already
    • Merge the import entry table into the entity table (since they both have the same columns now)
    • Add a boolean column to distinguish pending imports and accepted entities and use it for all entity views
    • Give import tables and columns more descriptive names (2c5b3ef)
    • Also integrate import views into the entity views which are used to load and update entities (66aae22)
  2. Adapt ORM (Adapt to pending entities with BBIDs / Introducing Kysely bookbrainz-data-js#323) and website to these changes
  3. Display imports via the regular entity pages
    • Thanks to the combined entity views this requires only a bit of fine-tuning! (e107640)
    • Hide irrelevant buttons and actions which don't make sense for pending imports
    • Show the proper footer to approve or discard the import instead of the regular entity actions
    • Move and adapt the approve/discard routes to the regular entity routers
  4. Completely rewrite the approval function to preserve the BBID which was already assigned (see Adapt to pending entities with BBIDs / Introducing Kysely bookbrainz-data-js#323 and 3541746)

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:

  • Approve the import for an initial revision before creating a new "Edit" revision as usual (b2c0577)
  • Exit early if the entity is approved without additional changes and avoid creating an empty revision (9dd8b51)

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 apparently yarn link does not take changed dependencies in your linked local bb-data version into account.

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.
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.
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