Support trigger migrations #3134
Merged
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
dato
force-pushed
the
trigger_migrations
branch
from
fdca981
to
222c3a6
Compare
As metadata changes, search continues to work.
dato
force-pushed
the
trigger_migrations
branch
from
222c3a6
to
0299f2e
Compare
dato
force-pushed
the
trigger_migrations
branch
3 times, most recently
from
1fc5431
to
be64c37
Compare
dato
force-pushed
the
trigger_migrations
branch
from
be93903
to
44ef928
Compare
Previously, triggers lived only in a particular migration file. With this change, code for the triggers resides in the model, and their lifecycle is managed through normal Django migrations.
dato
force-pushed
the
trigger_migrations
branch
2 times, most recently
from
4594de3
to
7472383
Compare
dato
force-pushed
the
trigger_migrations
branch
from
7472383
to
8df408e
Compare
- do not COALESCE columns that cannot be NULL - do not bring bookwyrm_book to author names JOIN - add comments documenting the four steps
dato
commented
Dec 5, 2023
|
fwiw, I think it might be a good idea to delay merging this until after having tagged the next release. I’m completely fine with that if you agree it makes sense. |
|
I agree! I think I'm going to make a release tomorrow morning with just what's already merged unless you think there's anything that should get in under the wire |
|
Only #3096 comes to mind (very optional, just to get it out there?; but I didn’t quite understand what it fixes, so I have no technical opinion). Current main sounds great! |
|
I considered that one and decided to wait because I have zero recollection of why I had it set to the current way, but I know I did it on purpose, and I don't want to have to re-debug it tomorrow. But another day, I will have the willpower to do so 😆 |
|
💯
Thank you for preparing this release.
|
Minnozz
approved these changes
Mar 13, 2024
Minnozz
reviewed
Mar 13, 2024
7 tasks
Conflicts: requirements.txt
dato
force-pushed
the
trigger_migrations
branch
from
8bdb0df
to
cceccd1
Compare
These are tests I missed when first writing trigger tests in test_book_search.py.
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.
author.search_vector, which is never usedauthor_search_vector_triggervia Author.Meta.triggersauthor_search_vector_trigger, add missing WHERE clausesearch_vector_triggervia Book.Meta.triggersbook_authors_search_vector_triggerto executeThis PR uses django-pgtrigger to define triggers as part of model metadata. With this, Django's migration mechanism can be used to keep track of changes.
Done:
In a later PR:
migrate trigger on bookwyrm_book_authors table
(pgtriggers docs say the latter probably requires giving
Book.authorsits own "through" table, which we need anyway for things like Multiple Types of Authors #1120. But that's a more pervasive change, and I don't believe it belongs in this PR.)Also, about this trigger, see last commit.)