-
Notifications
You must be signed in to change notification settings - Fork 919
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
Adds note tags support #5344
base: master
Are you sure you want to change the base?
Adds note tags support #5344
Changes from all commits
09bda4e
e15dfc4
a98e64e
67d150d
feb68f7
0efc5ed
16f0d6c
22c4ef9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
# == Schema Information | ||
# | ||
# Table name: note_tags | ||
# | ||
# note_id :bigint(8) not null, primary key | ||
# k :string default(""), not null, primary key | ||
# v :string default(""), not null | ||
# | ||
# Foreign Keys | ||
# | ||
# note_tags_id_fkey (note_id => notes.id) | ||
# | ||
|
||
class NoteTag < ApplicationRecord | ||
belongs_to :note | ||
|
||
validates :note, :associated => true | ||
validates :k, :v, :allow_blank => true, :length => { :maximum => 255 }, :characters => true | ||
validates :k, :uniqueness => { :scope => :note_id } | ||
Comment on lines
+17
to
+19
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To ensure stronger data integrity, we can enforce that k and v are always present, within length limits, and unique per note. validates :note, presence: true
validates :k, presence: true, length: { maximum: 255 }, uniqueness: { scope: :note_id }
validates :v, presence: true, length: { maximum: 255 } |
||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,10 @@ xml.note("lon" => note.lon, "lat" => note.lat) do | |
|
||
xml.date_closed note.closed_at if note.closed? | ||
|
||
note.tags.each do |k, v| | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code is duplicated multiple times, consider creating a shared helper method to reuse instead. |
||
xml.tag(:k => k, :v => v) | ||
end | ||
|
||
xml.comments do | ||
note.comments.each do |comment| | ||
xml.comment do | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
class CreateNoteTags < ActiveRecord::Migration[7.2] | ||
def change | ||
# Create a table and primary key | ||
create_table :note_tags, :primary_key => [:note_id, :k] do |t| | ||
t.column "note_id", :bigint, :null => false | ||
t.column "k", :string, :default => "", :null => false | ||
t.column "v", :string, :default => "", :null => false | ||
|
||
t.foreign_key :notes, :column => :note_id, :name => "note_tags_id_fkey" | ||
end | ||
end | ||
end | ||
Comment on lines
+1
to
+12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there will be the case where we would filter the notes by tags(keys and values) we can consider adding the composite index on k,v. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How is a surrogate key going to help with queries? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I misunderstood the use case, it doesn't. Adding the composite index on k and v would do.. I edited top comment for more clarity. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
FactoryBot.define do | ||
factory :note_tag do | ||
sequence(:k) { |n| "Key #{n}" } | ||
sequence(:v) { |n| "Value #{n}" } | ||
|
||
note | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Encoding all the tags in a string like this is nasty and creates all sorts of problems if people use colons or commas in tag names or values - colons in particular are historically common in tag names for other object types.
I'm not sure what the solution is as this API is unusual in using form parameters rather than an XML or JSON body to create an object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JSON just for tags?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Tom here. Also independent of implementation, validation here would be necessary in my opinion.
Two alternative approaches that could be useful here is to: