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

Adds note tags support #5344

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion app/assets/javascripts/index/new_note.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,10 @@ OSM.NewNote = function (map) {
data: {
lat: location.lat,
lon: location.lng,
text: $(form.text).val()
text: $(form.text).val(),
tags: JSON.stringify({
created_by: "OpenStreetMap-Website"
})
},
success: function (feature) {
noteCreated(feature, marker);
Expand Down
14 changes: 14 additions & 0 deletions app/controllers/api/notes_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -83,12 +83,26 @@ def create
lat = OSM.parse_float(params[:lat], OSM::APIBadUserInput, "lat was not a number")
comment = params[:text]

# Extract the tags, if present
tags = []
if params[:tags].present?
parsed_tags = JSON.parse(params[:tags])
parsed_tags.each do |key, value|
tags << { :k => key, :v => value } if key.present? && value.present?
end
end
Copy link
Member

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

JSON just for tags?

Copy link
Contributor

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:

  • use nested form parameters:
tags[created_by]=OpenStreetMap-Website&tags[editor]=JOSM
  • using a prefix for tag related properties:
tag_created_by=OpenStreetMap-Website&tag_editor=JOSM


# Include in a transaction to ensure that there is always a note_comment for every note
Note.transaction do
# Create the note
@note = Note.create(:lat => lat, :lon => lon)
raise OSM::APIBadUserInput, "The note is outside this world" unless @note.in_world?

# Create a NoteTag for each tag in the tags array
tags.each do |tag|
@note.note_tags.create(:k => tag[:k], :v => tag[:v])
end

# Save the note
@note.save!

Expand Down
12 changes: 12 additions & 0 deletions app/models/note.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ class Note < ApplicationRecord
has_many :subscriptions, :class_name => "NoteSubscription"
has_many :subscribers, :through => :subscriptions, :source => :user

has_many :note_tags

validates :id, :uniqueness => true, :presence => { :on => :update },
:numericality => { :on => :update, :only_integer => true }
validates :latitude, :longitude, :numericality => { :only_integer => true }
Expand Down Expand Up @@ -92,6 +94,16 @@ def author_ip
comments.first.author_ip
end

def tags
unless @tags
@tags = {}
note_tags.each do |tag|
@tags[tag.k] = tag.v
end
end
@tags
end

private

# Fill in default values for new notes
Expand Down
20 changes: 20 additions & 0 deletions app/models/note_tag.rb
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
Copy link
Contributor

@kcne kcne Dec 3, 2024

Choose a reason for hiding this comment

The 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
4 changes: 4 additions & 0 deletions app/views/api/notes/_note.gpx.builder
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,9 @@ xml.wpt("lon" => note.lon, "lat" => note.lat) do
xml.status note.status

xml.date_closed note.closed_at if note.closed?

note.tags.each do |k, v|
xml.tag(:k => k, :v => v)
end
end
end
2 changes: 2 additions & 0 deletions app/views/api/notes/_note.json.jbuilder
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ json.properties do
json.status note.status
json.closed_at note.closed_at.to_s if note.closed?

json.tags note.tags unless note.tags.empty?

json.comments(note.comments) do |comment|
json.date comment.created_at.to_s

Expand Down
4 changes: 4 additions & 0 deletions app/views/api/notes/_note.rss.builder
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,8 @@ xml.item do
xml.geo :lat, note.lat
xml.geo :long, note.lon
xml.georss :point, "#{note.lat} #{note.lon}"

note.tags.each do |k, v|
xml.tag(:k => k, :v => v)
end
end
4 changes: 4 additions & 0 deletions app/views/api/notes/_note.xml.builder
Original file line number Diff line number Diff line change
Expand Up @@ -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|
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Expand Down
4 changes: 4 additions & 0 deletions app/views/api/notes/feed.rss.builder
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ xml.rss("version" => "2.0",
xml.geo :lat, comment.note.lat
xml.geo :long, comment.note.lon
xml.georss :point, "#{comment.note.lat} #{comment.note.lon}"

comment.note.tags.each do |k, v|
xml.tag(:k => k, :v => v)
end
end
end
end
Expand Down
2 changes: 2 additions & 0 deletions app/views/notes/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
</ul>
</div>

<%= render :partial => "browse/tag_details", :object => @note.tags %>
<% if @note_comments.find { |comment| comment.author.nil? } -%>
<p class='alert alert-warning'><%= t ".anonymous_warning" %></p>
<% end -%>
Expand Down
12 changes: 12 additions & 0 deletions db/migrate/20241030122707_create_note_tags.rb
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
Copy link
Contributor

@kcne kcne Dec 3, 2024

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

How is a surrogate key going to help with queries?

Copy link
Contributor

@kcne kcne Dec 3, 2024

Choose a reason for hiding this comment

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

28 changes: 28 additions & 0 deletions db/structure.sql
Original file line number Diff line number Diff line change
Expand Up @@ -1061,6 +1061,17 @@ CREATE TABLE public.note_subscriptions (
);


--
-- Name: note_tags; Type: TABLE; Schema: public; Owner: -
--

CREATE TABLE public.note_tags (
note_id bigint NOT NULL,
k character varying DEFAULT ''::character varying NOT NULL,
v character varying DEFAULT ''::character varying NOT NULL
);


--
-- Name: notes; Type: TABLE; Schema: public; Owner: -
--
Expand Down Expand Up @@ -2028,6 +2039,14 @@ ALTER TABLE ONLY public.note_subscriptions
ADD CONSTRAINT note_subscriptions_pkey PRIMARY KEY (user_id, note_id);


--
-- Name: note_tags note_tags_pkey; Type: CONSTRAINT; Schema: public; Owner: -
--

ALTER TABLE ONLY public.note_tags
ADD CONSTRAINT note_tags_pkey PRIMARY KEY (note_id, k);


--
-- Name: notes notes_pkey; Type: CONSTRAINT; Schema: public; Owner: -
--
Expand Down Expand Up @@ -3210,6 +3229,14 @@ ALTER TABLE ONLY public.note_comments
ADD CONSTRAINT note_comments_note_id_fkey FOREIGN KEY (note_id) REFERENCES public.notes(id);


--
-- Name: note_tags note_tags_id_fkey; Type: FK CONSTRAINT; Schema: public; Owner: -
--

ALTER TABLE ONLY public.note_tags
ADD CONSTRAINT note_tags_id_fkey FOREIGN KEY (note_id) REFERENCES public.notes(id);


--
-- Name: redactions redactions_user_id_fkey; Type: FK CONSTRAINT; Schema: public; Owner: -
--
Expand Down Expand Up @@ -3397,6 +3424,7 @@ INSERT INTO "schema_migrations" (version) VALUES
('23'),
('22'),
('21'),
('20241030122707'),
('20241023004427'),
('20241022141247'),
('20240913171951'),
Expand Down
52 changes: 46 additions & 6 deletions test/controllers/api/notes_controller_test.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
require "test_helper"
require "json"

module Api
class NotesControllerTest < ActionDispatch::IntegrationTest
Expand Down Expand Up @@ -105,7 +106,18 @@ def test_create_anonymous_success
assert_difference "Note.count", 1 do
assert_difference "NoteComment.count", 1 do
assert_no_difference "NoteSubscription.count" do
post api_notes_path(:lat => -1.0, :lon => -1.0, :text => "This is a comment", :format => "json")
assert_difference "NoteTag.count", 2 do
post api_notes_path(
:lat => -1.0,
:lon => -1.0,
:tags => {
"created_by" => "OSM_TEST",
"삭ÒX~`!@#$%^&*()-=_+,<.>/?;:'\"[{}]\\|傥4ր" => "Ƭ߯ĸá~`!@#$%^&*()-=_+,<.>/?;:'\"[{}]\\|؇Őϋ"
}.to_json,
:text => "This is a comment",
:format => "json"
)
end
end
end
end
Expand All @@ -116,6 +128,9 @@ def test_create_anonymous_success
assert_equal "Point", js["geometry"]["type"]
assert_equal [-1.0, -1.0], js["geometry"]["coordinates"]
assert_equal "open", js["properties"]["status"]
assert_equal 2, js["properties"]["tags"].count
assert_equal "OSM_TEST", js["properties"]["tags"]["created_by"]
assert_equal "Ƭ߯ĸá~`!@#$%^&*()-=_+,<.>/?;:'\"[{}]\\|؇Őϋ", js["properties"]["tags"]["삭ÒX~`!@#$%^&*()-=_+,<.>/?;:'\"[{}]\\|傥4ր"]
assert_equal 1, js["properties"]["comments"].count
assert_equal "opened", js["properties"]["comments"].last["action"]
assert_equal "This is a comment", js["properties"]["comments"].last["text"]
Expand All @@ -131,6 +146,9 @@ def test_create_anonymous_success
assert_equal [-1.0, -1.0], js["geometry"]["coordinates"]
assert_equal id, js["properties"]["id"]
assert_equal "open", js["properties"]["status"]
assert_equal 2, js["properties"]["tags"].count
assert_equal "OSM_TEST", js["properties"]["tags"]["created_by"]
assert_equal "Ƭ߯ĸá~`!@#$%^&*()-=_+,<.>/?;:'\"[{}]\\|؇Őϋ", js["properties"]["tags"]["삭ÒX~`!@#$%^&*()-=_+,<.>/?;:'\"[{}]\\|傥4ր"]
assert_equal 1, js["properties"]["comments"].count
assert_equal "opened", js["properties"]["comments"].last["action"]
assert_equal "This is a comment", js["properties"]["comments"].last["text"]
Expand Down Expand Up @@ -594,6 +612,8 @@ def test_reopen_fail

def test_show_success
open_note = create(:note_with_comments)
create(:note_tag, :note => open_note, :k => "created_by", :v => "OSM_TEST")
create(:note_tag, :note => open_note, :k => "삭ÒX~`!@#$%^&*()-=_+,<.>/?;:'\"[{}]\\|傥4ր", :v => "Ƭ߯ĸá~`!@#$%^&*()-=_+,<.>/?;:'\"[{}]\\|؇Őϋ")

get api_note_path(open_note, :format => "xml")
assert_response :success
Expand All @@ -610,6 +630,8 @@ def test_show_success
assert_select "comment", :count => 1
end
end
assert_select "tag[k='created_by'][v='OSM_TEST']", :count => 1
assert_select "tag[k='삭ÒX~`!@#$%^&*()-=_+,<.>/?;:\\'\"[{}]\\\\|傥4ր'][v='Ƭ߯ĸá~`!@#$%^&*()-=_+,<.>/?;:\\'\"[{}]\\\\|؇Őϋ']", :count => 1
end

get api_note_path(open_note, :format => "rss")
Expand All @@ -624,6 +646,8 @@ def test_show_success
assert_select "geo|lat", open_note.lat.to_s
assert_select "geo|long", open_note.lon.to_s
assert_select "georss|point", "#{open_note.lon} #{open_note.lon}"
assert_select "tag[k='created_by'][v='OSM_TEST']", :count => 1
assert_select "tag[k='삭ÒX~`!@#$%^&*()-=_+,<.>/?;:\\'\"[{}]\\\\|傥4ր'][v='Ƭ߯ĸá~`!@#$%^&*()-=_+,<.>/?;:\\'\"[{}]\\\\|؇Őϋ']", :count => 1
end
end
end
Expand All @@ -643,6 +667,8 @@ def test_show_success
assert_equal close_api_note_url(open_note, :format => "json"), js["properties"]["close_url"]
assert_equal open_note.created_at.to_s, js["properties"]["date_created"]
assert_equal open_note.status, js["properties"]["status"]
assert_equal "OSM_TEST", js["properties"]["tags"]["created_by"]
assert_equal "Ƭ߯ĸá~`!@#$%^&*()-=_+,<.>/?;:'\"[{}]\\|؇Őϋ", js["properties"]["tags"]["삭ÒX~`!@#$%^&*()-=_+,<.>/?;:'\"[{}]\\|傥4ր"]

get api_note_path(open_note, :format => "gpx")
assert_response :success
Expand All @@ -658,6 +684,8 @@ def test_show_success
assert_select "url", api_note_url(open_note, :format => "gpx")
assert_select "comment_url", comment_api_note_url(open_note, :format => "gpx")
assert_select "close_url", close_api_note_url(open_note, :format => "gpx")
assert_select "tag[k='created_by'][v='OSM_TEST']", :count => 1
assert_select "tag[k='삭ÒX~`!@#$%^&*()-=_+,<.>/?;:\\'\"[{}]\\\\|傥4ր'][v='Ƭ߯ĸá~`!@#$%^&*()-=_+,<.>/?;:\\'\"[{}]\\\\|؇Őϋ']", :count => 1
end
end
end
Expand Down Expand Up @@ -1171,18 +1199,30 @@ def test_search_bad_params

def test_feed_success
position = (1.1 * GeoRecord::SCALE).to_i
create(:note_with_comments, :latitude => position, :longitude => position)
create(:note_with_comments, :latitude => position, :longitude => position)
a_note = create(:note_with_comments, :latitude => position, :longitude => position)
create(:note_tag, :note => a_note, :k => "created_by", :v => "OSM_TEST")
create(:note_tag, :note => a_note, :k => "삭ÒX~`!@#$%^&*()-=_+,<.>/?;:'\"[{}]\\|傥4ր", :v => "Ƭ߯ĸá~`!@#$%^&*()-=_+,<.>/?;:'\"[{}]\\|؇Őϋ")
b_note = create(:note_with_comments, :latitude => position, :longitude => position)
create(:note_tag, :note => b_note, :k => "created_by", :v => "OSM_TEST")
create(:note_tag, :note => b_note, :k => "삭ÒX~`!@#$%^&*()-=_+,<.>/?;:'\"[{}]\\|傥4ր", :v => "Ƭ߯ĸá~`!@#$%^&*()-=_+,<.>/?;:'\"[{}]\\|؇Őϋ")
position = (1.5 * GeoRecord::SCALE).to_i
create(:note_with_comments, :latitude => position, :longitude => position)
create(:note_with_comments, :latitude => position, :longitude => position)
c_note = create(:note_with_comments, :latitude => position, :longitude => position)
create(:note_tag, :note => c_note, :k => "created_by", :v => "OSM_TEST")
create(:note_tag, :note => c_note, :k => "삭ÒX~`!@#$%^&*()-=_+,<.>/?;:'\"[{}]\\|傥4ր", :v => "Ƭ߯ĸá~`!@#$%^&*()-=_+,<.>/?;:'\"[{}]\\|؇Őϋ")
d_note = create(:note_with_comments, :latitude => position, :longitude => position)
create(:note_tag, :note => d_note, :k => "created_by", :v => "OSM_TEST")
create(:note_tag, :note => d_note, :k => "삭ÒX~`!@#$%^&*()-=_+,<.>/?;:'\"[{}]\\|傥4ր", :v => "Ƭ߯ĸá~`!@#$%^&*()-=_+,<.>/?;:'\"[{}]\\|؇Őϋ")

get feed_api_notes_path(:format => "rss")
assert_response :success
assert_equal "application/rss+xml", @response.media_type
assert_select "rss", :count => 1 do
assert_select "channel", :count => 1 do
assert_select "item", :count => 4
assert_select "item", :count => 4 do |items|
items.each do |item|
assert_select item, "tag", :count => 2
end
end
end
end

Expand Down
21 changes: 21 additions & 0 deletions test/controllers/notes_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,27 @@ def test_index_success
assert_response :not_found
end

def test_displaying_note_with_tags
user = create(:user)

note = create(:note)
create(:note_comment, :note => note, :author => user, :body => "Note description")
create(:note_tag, :note => note, :k => "created_by", :v => "OSM_TEST")
create(:note_tag, :note => note, :k => "삭ÒX~`!@#$%^&*()-=_+,<.>/?;:'\"[{}]\\|傥4ր", :v => "Ƭ߯ĸá~`!@#$%^&*()-=_+,<.>/?;:'\"[{}]\\|؇Őϋ")

sidebar_browse_check :note_path, note.id, "notes/show"
assert_dom "h2", :text => "Unresolved note ##{note.id}"
assert_dom "p", :text => "Note description"
assert_dom "tr" do
assert_dom "th", :text => "created_by"
assert_dom "td", :text => "OSM_TEST"
end
assert_dom "tr" do
assert_dom "th", :text => "삭ÒX~`!@#$%^&*()-=_+,<.>/?;:'\"[{}]\\|傥4ր"
assert_dom "td", :text => "Ƭ߯ĸá~`!@#$%^&*()-=_+,<.>/?;:'\"[{}]\\|؇Őϋ"
end
end

def test_index_paged
user = create(:user)

Expand Down
8 changes: 8 additions & 0 deletions test/factories/note_tags.rb
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
Loading