diff --git a/app/assets/javascripts/index/new_note.js b/app/assets/javascripts/index/new_note.js index 887ba043b1..da4cfe19cd 100644 --- a/app/assets/javascripts/index/new_note.js +++ b/app/assets/javascripts/index/new_note.js @@ -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); diff --git a/app/controllers/api/notes_controller.rb b/app/controllers/api/notes_controller.rb index 7e2e7fb793..3bd5e585b2 100644 --- a/app/controllers/api/notes_controller.rb +++ b/app/controllers/api/notes_controller.rb @@ -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.presence || "", :v => value.presence || "" } + end + end + # 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! diff --git a/app/models/note.rb b/app/models/note.rb index 6d8ca078fa..f053b4cfd2 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -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 } @@ -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 diff --git a/app/models/note_tag.rb b/app/models/note_tag.rb new file mode 100644 index 0000000000..36d3b2c552 --- /dev/null +++ b/app/models/note_tag.rb @@ -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 } +end diff --git a/app/views/api/notes/_note.gpx.builder b/app/views/api/notes/_note.gpx.builder index bb5cac1c9f..b0d886e7d6 100644 --- a/app/views/api/notes/_note.gpx.builder +++ b/app/views/api/notes/_note.gpx.builder @@ -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 diff --git a/app/views/api/notes/_note.json.jbuilder b/app/views/api/notes/_note.json.jbuilder index 0884c4426d..cd432921fd 100644 --- a/app/views/api/notes/_note.json.jbuilder +++ b/app/views/api/notes/_note.json.jbuilder @@ -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 diff --git a/app/views/api/notes/_note.xml.builder b/app/views/api/notes/_note.xml.builder index 0e5ad0007e..ad0b65f4c6 100644 --- a/app/views/api/notes/_note.xml.builder +++ b/app/views/api/notes/_note.xml.builder @@ -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| + xml.tag(:k => k, :v => v) + end + xml.comments do note.comments.each do |comment| xml.comment do diff --git a/app/views/notes/show.html.erb b/app/views/notes/show.html.erb index d17612e292..9073695094 100644 --- a/app/views/notes/show.html.erb +++ b/app/views/notes/show.html.erb @@ -22,6 +22,8 @@

+ <%= render :partial => "browse/tag_details", :object => @note.tags %> + <% if @note_comments.find { |comment| comment.author.nil? } -%>

<%= t ".anonymous_warning" %>

<% end -%> diff --git a/db/migrate/20241030122707_create_note_tags.rb b/db/migrate/20241030122707_create_note_tags.rb new file mode 100644 index 0000000000..46f7cb2055 --- /dev/null +++ b/db/migrate/20241030122707_create_note_tags.rb @@ -0,0 +1,12 @@ +class CreateNoteTags < ActiveRecord::Migration[7.2] + def change + # Create a table, primary and foreign keys + 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 diff --git a/db/structure.sql b/db/structure.sql index 9679e0b925..280afa536b 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -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: - -- @@ -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: - -- @@ -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: - -- @@ -3397,6 +3424,7 @@ INSERT INTO "schema_migrations" (version) VALUES ('23'), ('22'), ('21'), +('20241030122707'), ('20241023004427'), ('20241022141247'), ('20240913171951'), diff --git a/test/controllers/api/notes_controller_test.rb b/test/controllers/api/notes_controller_test.rb index 5f69e6a2ac..8602f549b6 100644 --- a/test/controllers/api/notes_controller_test.rb +++ b/test/controllers/api/notes_controller_test.rb @@ -1,4 +1,5 @@ require "test_helper" +require "json" module Api class NotesControllerTest < ActionDispatch::IntegrationTest @@ -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 @@ -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"] @@ -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"] @@ -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 @@ -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") @@ -643,6 +665,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 @@ -658,6 +682,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 diff --git a/test/controllers/notes_controller_test.rb b/test/controllers/notes_controller_test.rb index 71dfa42bae..3a39f7e91b 100644 --- a/test/controllers/notes_controller_test.rb +++ b/test/controllers/notes_controller_test.rb @@ -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) diff --git a/test/factories/note_tags.rb b/test/factories/note_tags.rb new file mode 100644 index 0000000000..f7d90e002d --- /dev/null +++ b/test/factories/note_tags.rb @@ -0,0 +1,8 @@ +FactoryBot.define do + factory :note_tag do + sequence(:k) { |n| "Key #{n}" } + sequence(:v) { |n| "Value #{n}" } + + note + end +end diff --git a/test/models/note_tag_test.rb b/test/models/note_tag_test.rb new file mode 100644 index 0000000000..058d68eb87 --- /dev/null +++ b/test/models/note_tag_test.rb @@ -0,0 +1,49 @@ +require "test_helper" + +class NoteTagTest < ActiveSupport::TestCase + def test_length_key_valid + tag = create(:note_tag) + [0, 255].each do |i| + tag.k = "k" * i + assert_predicate tag, :valid? + end + end + + def test_length_value_valid + tag = create(:note_tag) + [0, 255].each do |i| + tag.v = "v" * i + assert_predicate tag, :valid? + end + end + + def test_length_key_invalid + tag = create(:note_tag) + tag.k = "k" * 256 + assert_not_predicate tag, :valid?, "Key should be too long" + assert_predicate tag.errors[:k], :any? + end + + def test_length_value_invalid + tag = create(:note_tag) + tag.v = "v" * 256 + assert_not_predicate tag, :valid?, "Value should be too long" + assert_predicate tag.errors[:v], :any? + end + + def test_orphaned_tag_invalid + tag = create(:note_tag) + tag.note = nil + assert_not_predicate tag, :valid?, "Orphaned tag should be invalid" + assert_predicate tag.errors[:note], :any? + end + + def test_uniqueness + existing = create(:note_tag) + tag = build(:note_tag, :note => existing.note, :k => existing.k, :v => existing.v) + assert_predicate tag, :new_record? + assert_not_predicate tag, :valid? + assert_raise(ActiveRecord::RecordInvalid) { tag.save! } + assert_predicate tag, :new_record? + end +end