Skip to content

Conversation

@jethro-r
Copy link
Collaborator

@jethro-r jethro-r commented Jun 2, 2025

@jethro-r jethro-r requested a review from ReganRyanNZ June 2, 2025 11:04
@jethro-r
Copy link
Collaborator Author

jethro-r commented Jun 2, 2025

Still in development, but pull and test if you want

@@ -0,0 +1,4 @@
class MakeBookOwnerNotNull < ActiveRecord::Migration[7.1]
def change
end
Copy link
Owner

Choose a reason for hiding this comment

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

This migration is empty 🙃

@@ -0,0 +1,5 @@
class AddOwnerToBooks < ActiveRecord::Migration[7.1]
def change
add_reference :books, :owner, foreign_key: { to_table: :users }
Copy link
Owner

Choose a reason for hiding this comment

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

This is normal rails way to tie 2 tables together, but we don't want admin users to own books, we just want books to have a list of editors. An array of strings would be good, or maybe a jsonb field to have e.g. {"regan" => "regan.ryan.nz@gmail.com"} yea that's better because then we can list the names of the owners, since some people have weird emails

# become the chorus for this tune"
end

def song_diff(old_lyrics, new_lyrics, context_lines: 2)
Copy link
Owner

Choose a reason for hiding this comment

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

It would be good to do git rebase -i master and drop the email commit, since it's not part of this feature

songs: songs,
languages: languages
languages: languages,
admin: owner_id || [71],
Copy link
Owner

Choose a reason for hiding this comment

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

User.system_user.id would be much better than "71"

Also you have "integer OR array" which might lead to problems.

But yea I think would be good to shift to jsonb as per another comment somewhere

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.

3 participants