Skip to content

🧹 Journal: Entries store their Keywords #1663

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

Merged
merged 2 commits into from
Jul 15, 2023

Conversation

zspencer
Copy link
Member

This gets us a little bit closer to being able to browse Entry by Keywords, since each Entry will know it's keywords

@zspencer zspencer requested review from a team July 13, 2023 23:13
@zspencer zspencer changed the title Journal: Entries may be queried by Keywords 🧹 Journal: Entries may be queried by Keywords Jul 13, 2023
@zspencer zspencer added the 🧹 refactor Includes non-behavioral changes label Jul 13, 2023
@zspencer zspencer changed the title 🧹 Journal: Entries may be queried by Keywords 🧹 Journal: Entries store their Keywords Jul 13, 2023
Copy link
Member

@anaulin anaulin left a comment

Choose a reason for hiding this comment

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

Having keywords at both the Journal and the Entry level feels a little bit overkill to me, but I'm sure you have some larger plan for what that's necessary.

.or(where(canonical_keyword: keyword)).exists?
body.scan(/#(\w+)/)&.flatten&.map do |keyword|
existing_keyword = where(":aliases = ANY (aliases)", aliases: keyword)
.or(where(canonical_keyword: keyword)).first
Copy link
Member

Choose a reason for hiding this comment

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

I realize this query is from before your PR, but reading it over right now it struck me that it feels like it should be a nice little scope, which would help tighten up this entire loop a bit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree; I would love for this to be a scope!

@@ -0,0 +1,5 @@
class JournalAddKeywordsToEntries < ActiveRecord::Migration[7.0]
def change
add_column :journal_entries, :keywords, :string, array: true
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised these are not tied in any way to the ids of the Journal keywords -- I would have expected you'd want that to show "all entries with keyword X in Journal".

Copy link
Member Author

Choose a reason for hiding this comment

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

You know, I was thinking a join table would be "too much" but I suppose it's not?

The way I was planning to find the Entries was indexing this keywords field and using the scope you suggested above; but I do worry that that would make the relationy bits a bit less railsy

Copy link
Contributor

@daltonrpruitt daltonrpruitt left a comment

Choose a reason for hiding this comment

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

I spent too long trying to figure out where the keywords were shown in the UI....
I should read more deeply. 😬

@zspencer
Copy link
Member Author

Having keywords at both the Journal and the Entry level feels a little bit overkill to me, but I'm sure you have some larger plan for what that's necessary.

I was thinking that an Entry >=< Keyword relationship is a many-to-many, and the Journal =< Keyword is one-to-many.

I spent too long trying to figure out where the keywords were shown in the UI....
I should read more deeply. 😬

I am trying to take very very tiny steps; instead of just YOLO-Shipping the whole thing lol. It does make it a bit harder to figure out the trajectory tho.

@zspencer zspencer force-pushed the journal/entries-know-their-keywords branch from 6f9a625 to 6ffd6bd Compare July 15, 2023 01:01
Copy link
Member Author

@zspencer zspencer left a comment

Choose a reason for hiding this comment

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

OK, I've extracted the scope; and I am wanting to see how far I can get before sprouting the join table

The reason I'm avoiding the join table is because I work in another code base with hundreds of thousands of "taggings" in a join table; and the performance characteristics are pretty bad; even with fully indexing the taggings table across the tag_id and entry_id in both directions!

My gut says that storing the tags an Entry uses and indexing that will be far more performant because using the canonical keyword as a kind of "natural join" when traversing from the keyword to the entries; and from the entries to the keywords.

But I'm willing to drop that concern.

@@ -6,10 +6,16 @@ class Keyword < ApplicationRecord
validates :canonical_keyword, presence: true, uniqueness: {case_sensitive: false, scope: :journal_id}
belongs_to :journal, inverse_of: :keywords

scope(:search, lambda do |*keywords|
where("lower(aliases::text)::text[] && ARRAY[?]::text[]", keywords.map(&:downcase))
.or(where("lower(canonical_keyword) IN (?)", keywords.map(&:downcase)))
Copy link
Member Author

Choose a reason for hiding this comment

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

There is probably a way less awkward way of doing this; but this is what I was able to come up with 🙃

@zspencer zspencer merged commit d413ff8 into main Jul 15, 2023
@zspencer zspencer deleted the journal/entries-know-their-keywords branch July 15, 2023 01:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧹 refactor Includes non-behavioral changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants