Re: [openstreetmap/openstreetmap-website] Adds note tags support (PR #5344)
@nenad-vujicic commented on this pull request. > + note.tags.each do |k, v| +xml.tag(:k => k, :v => v) + end Thanks, removed! -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5344#discussion_r1879055306 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds note tags support (PR #5344)
@nenad-vujicic pushed 8 commits. 12591bcfb671e980ed74c34f3004355e92b47bae Added NoteTag model and note_tags table b8d585a99cebb332960fde3d224d31d4ede3112b Added displaying tags on Notes sidebar 31c9a18f4dcd73afdb1ed8f3d2f70bf0ead99f1c Updated (j)builder files with note tags 8fc5d396891f0a41ba571497eb0785775bb06ac3 Added support for tags in note creation API 45375fe469e77090b94963a2db32a4a663e2264a Added note_tag factory and NoteTag model test-case 1f304e502fbf3b6c17fa3844c5c206acdc768f9a Added unit test for notes with tags bd52817e9f9b3d1da3fc117a61d07d01e4c6a5de Test note/tag output created at ActiveRecord level f2c0a069eadee057b6a184a17a7d983358ca8811 Test note/tag output created at POST/HTML level -- View it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5344/files/9950cd43e13fa816939ef1555612562a39071bda..f2c0a069eadee057b6a184a17a7d983358ca8811 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds note tags support (PR #5344)
@AntonKhorev commented on this pull request. > + note.tags.each do |k, v| +xml.tag(:k => k, :v => v) + end Or not writing the tags in rss for now. Currently nobody is looking for them there. -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5344#discussion_r1877637785 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds note tags support (PR #5344)
@nenad-vujicic commented on this pull request. > + note.tags.each do |k, v| +xml.tag(:k => k, :v => v) + end Or perhaps using one dc:subject element per note-tag pair (supported by the DCMI standard)? This was used in the most recent push. -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5344#discussion_r1877010811 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds note tags support (PR #5344)
@nenad-vujicic pushed 8 commits. 90c02cac2cfa6bef044900602e8bb420b2af167d Added NoteTag model and note_tags table 40035aed16d598b26c605ee98fa85968d8add1f1 Added displaying tags on Notes sidebar ba084cc11d5b824fba08cb704c6ced6a5175b470 Updated (j)builder files with note tags b833e400e60ba6a00e44d78ec7d2acdf53707534 Added support for tags in note creation API 98bcb6494b79607db33174eb6d5109aa75aa7dee Added note_tag factory and NoteTag model test-case faa483b9dc27f2f80f61a721fd301e2c6fbb1fa7 Added unit test for notes with tags e8a149e49beae5919a830467bd93b78ff5727ce4 Test note/tag output created at ActiveRecord level 9950cd43e13fa816939ef1555612562a39071bda Test note/tag output created at POST/HTML level -- View it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5344/files/1c14d7f434717f633957600e54acefc8e6e23bcd..9950cd43e13fa816939ef1555612562a39071bda You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds note tags support (PR #5344)
@nenad-vujicic commented on this pull request. > + note.tags.each do |k, v| +xml.tag(:k => k, :v => v) + end Thanks for catching this. Honestly, I'm not sure about what could be the best solution here. I believe we have multiple options: 1. Adding Atom + XHTML namespaces and displaying tags like in [user's history feed](https://www.openstreetmap.org/user/Nenad%20Vujicic/history/feed) (I'm a little bit afraid to add new namespaces because of client support) 2. Using e.g. for simplified, flattened representation of tags (the easiest and the most portable, but not so nice visual representation of tags) 3. Creating custom or some other namespace or some other solution? Can you suggest what could be the best approach here? -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5344#discussion_r1876232635 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds note tags support (PR #5344)
@AntonKhorev commented on this pull request. > + note.tags.each do |k, v| +xml.tag(:k => k, :v => v) + end Is it going to be understood by any rss readers? -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5344#pullrequestreview-2486497915 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds note tags support (PR #5344)
Here is the latest update with removed validation (which allows k and v to be empty strings, like they can be in e.g. [this node](https://www.openstreetmap.org/node/9842077514) (@kcne thanks for helping in catching this). I haven't added other stuffs (composite index - this will be part of separate PR, pulling out shared routine for writing tags - somehow this doesn't affect readability to me, but we could pull it out for all elements in a separate PR), but can add them if others request. @tomhughes @gravitystorm @AntonKhorev can you, please, share your thoughts about this PR and a way it solves the problem? If we want to do it in a proper way, we will need to pass XML / JSON as a payload, but the problem with this approach is we'll either break backward compatibility or would need to create additional route which will work in a new way (this looks a little bit dirty to me and more like a workaround than a proper way). What is the preferred way for you at the moment (the current one in this PR or passing XML/JSON as a payload using separate route) or can you suggest some other better solution? Thanks, Nenad. -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5344#issuecomment-2520301211 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds note tags support (PR #5344)
@nenad-vujicic pushed 8 commits. c213dc73ef45a368c950278aec84f0eaa985410c Added NoteTag model and note_tags table fe9843964667255bc6a4e02ac21a828dd7a4374d Added displaying tags on Notes sidebar 0120c6e6827985aaab80aa7e8fcb541dae49844b Updated (j)builder files with note tags 29aa9c6e4558ca3423b783183826a73403c476cc Added support for tags in note creation API 06ab3bc77d7f79232cf01042a28f2f4f9a7343db Added note_tag factory and NoteTag model test-case 1e4feff0008f0c61b8f86eb1774a80bb33f674b8 Added unit test for notes with tags dcb52420c59484c393e45748240ce39c9fc257e5 Test note/tag output created at ActiveRecord level 1c14d7f434717f633957600e54acefc8e6e23bcd Test note/tag output created at POST/HTML level -- View it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5344/files/22c4ef927ebb124868adc89755353822d68f9da0..1c14d7f434717f633957600e54acefc8e6e23bcd You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds note tags support (PR #5344)
@kcne commented on this pull request. > +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 I misunderstood the use case, you are right it doesn't. Adding the index on k and v would do.. I will edit the top comment for more clarity -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5344#discussion_r1868452118 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds note tags support (PR #5344)
@AntonKhorev commented on this pull request. > +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 How is a surrogate key going to help with queries? -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5344#discussion_r1868247899 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds note tags support (PR #5344)
@kcne commented on this pull request.
> @@ -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|
This code is duplicated multiple times, consider creating a shared helper
method to reuse instead.
> + validates :note, :associated => true
+ validates :k, :v, :allow_blank => true, :length => { :maximum => 255 },
:characters => true
+ validates :k, :uniqueness => { :scope => :note_id }
To ensure stronger data integrity, we can enforce that k and v are always
present, within length limits, and unique per note. Additionally, using
`presence: true` for note simplifies and improves performance compared to
`associated: true`
```ruby
validates :note, presence: true
validates :k, presence: true, length: { maximum: 255 }, uniqueness: { scope:
:note_id }
validates :v, presence: true, length: { maximum: 255 }
```
> +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
This migration works well for ensuring uniqueness with a composite primary key.
However, if querying by tags (e.g., `{k: "created_by", v: "openstreetmap"}`) is
expected, a surrogate primary key (`id`) with additional indexes might be more
efficient. For example:
```ruby
create_table :note_tags do |t|
t.bigint :note_id, null: false
t.string :k, null: false, default: ""
t.string :v, null: false, default: ""
t.timestamps
end
add_index :note_tags, [:note_id, :k], unique: true # Enforce uniqueness
add_index :note_tags, [:k, :v] # Optimize tag-based
queries
```
This approach aligns with Rails conventions, simplifies queries, and allows
adding indexes for tag lookups (`k`/`v`).
> @@ -83,12 +85,30 @@ def create
lat = OSM.parse_float(params[:lat], OSM::APIBadUserInput, "lat was not a
number")
comment = params[:text]
+ # Extract the tags parameter (if it exists)
+ tags = []
+ if params[:tags].present?
+# Split by commas to get individual key-value pairs
+pairs = params[:tags].split(",")
+
+# For each pair in parameters, store it in tags variable
+pairs.each do |pair|
+ key, value = pair.split(":", 2)
+ tags << { :k => sanitize(key), :v => sanitize(value) } if key &&
value
+end
+ end
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
```
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/5344#pullrequestreview-2475434346
You are receiving this because you are subscribed to this thread.
Message ID:
___
rails-dev mailing list
rails-dev@openstreetmap.org
https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds note tags support (PR #5344)
OK, here is another attempt. I made the above corrections and updated it to pass tag definitions as a nested object serialized into JSON, supporting various characters in keys and values. -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5344#issuecomment-2501865573 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds note tags support (PR #5344)
@nenad-vujicic pushed 8 commits. 09bda4e6e9cdeb46f890807b48f69c9181bff5b4 Added NoteTag model and note_tags table e15dfc4a06f2ad8109a5f35e4895ff1c9263b5d5 Added displaying tags on Notes sidebar a98e64ee5694d84c3d16608b3d076a2161ff388c Updated (j)builder files with note tags 67d150d41528e9f61df5c95eba795e5201fbd929 Added support for tags in note creation API feb68f7244d435fff836a3a6dc4fd3eef8c58048 Added note_tag factory and NoteTag model test-case 0efc5ed2a3eb18be29a34958dc195cd49672a92b Added unit test for notes with tags 16f0d6c3e3481b328e944e54ab2b19f386a78e60 Test note/tag output created at ActiveRecord level 22c4ef927ebb124868adc89755353822d68f9da0 Test note/tag output created at POST/HTML level -- View it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5344/files/0841bfcd8d86b7abb0f3c19acfc8a522a2686a5f..22c4ef927ebb124868adc89755353822d68f9da0 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds note tags support (PR #5344)
@AntonKhorev commented on this pull request.
> @@ -83,12 +85,30 @@ def create
lat = OSM.parse_float(params[:lat], OSM::APIBadUserInput, "lat was not a
number")
comment = params[:text]
+ # Extract the tags parameter (if it exists)
+ tags = []
+ if params[:tags].present?
+# Split by commas to get individual key-value pairs
+pairs = params[:tags].split(",")
+
+# For each pair in parameters, store it in tags variable
+pairs.each do |pair|
+ key, value = pair.split(":", 2)
+ tags << { :k => sanitize(key), :v => sanitize(value) } if key &&
value
+end
+ end
JSON just for tags?
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/5344#discussion_r1855499989
You are receiving this because you are subscribed to this thread.
Message ID:
___
rails-dev mailing list
rails-dev@openstreetmap.org
https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds note tags support (PR #5344)
@tomhughes requested changes on this pull request.
> @@ -9,6 +9,13 @@ SET xmloption = content;
SET client_min_messages = warning;
SET row_security = off;
+--
+-- Name: public; Type: SCHEMA; Schema: -; Owner: -
+--
+
+-- *not* creating schema, since initdb creates it
+
+
This block is noise created by the version of `pg_dump` you're using and has
nothing to do with the actual changes in this PR so should be removed.
> @@ -0,0 +1,13 @@
+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
+end
+
+# Add foreign key without validation
+add_foreign_key :note_tags, :notes, :column => :note_id, :name =>
"note_tags_id_fkey", :validate => false
Would moving this inside the `create_table` as `t.foreign_key` avoid the need
to create it with validation disabled? Not sure if the strong migrations is
clever enough to know that is safe?
If not then you can just add a second migration to enable validation as it's
safe to do on an empty table.
> @@ -50,7 +50,8 @@ OSM.NewNote = function (map) {
data: {
lat: location.lat,
lon: location.lng,
-text: $(form.text).val()
+text: $(form.text).val(),
+tags: "created_by:OpenStreetMaps-Website"
There is no `s` on the end of OpenStreetMap ;-) I'm not sure what is the best
value for the tag here but it definitely shouldn't have the trailing s.
> @@ -83,12 +85,30 @@ def create
lat = OSM.parse_float(params[:lat], OSM::APIBadUserInput, "lat was not a
number")
comment = params[:text]
+ # Extract the tags parameter (if it exists)
+ tags = []
+ if params[:tags].present?
+# Split by commas to get individual key-value pairs
+pairs = params[:tags].split(",")
+
+# For each pair in parameters, store it in tags variable
+pairs.each do |pair|
+ key, value = pair.split(":", 2)
+ tags << { :k => sanitize(key), :v => sanitize(value) } if key &&
value
+end
+ end
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.
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/5344#pullrequestreview-2456745482
You are receiving this because you are subscribed to this thread.
Message ID:
___
rails-dev mailing list
rails-dev@openstreetmap.org
https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds note tags support (PR #5344)
Many of the use cases covered by this issue/PR are currently solved with hash tags (e.g. #surveyme) and tools are currently made to take these into account. Tools would need to be adapted to support tags as well. The advantage of having tags implemented like this properly would be that there could be an efficient API with which to filter notes. (Right now, this is the _only_ advantage I can think of over hash tags and stuff.) For example: - search for notes created by StreetComplete for certain quest types, to find common issues (misunderstandings, missing answer options) with quests - search for notes created by certain versions of StreetComplete, e.g. to find notes that where perhaps created in error due to the above - search specifically for notes that need e.g. survey. (However, this use case would not be useful for StreetComplete, because StreetComplete downloads all notes, anyway) -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5344#issuecomment-2491705313 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds note tags support (PR #5344)
Here are a few more benefits of adding note tags to OSM, as implemented by this PR: 1. Improves **notes categorization** (by e.g. adding descriptive tags for better organization like "Missing place" or similar):  2. Improves **collaboration & communication** (by e.g. marking resolving some note requires outdoor survey):  3. Now we are ready for **advanced filtering** by specific tags (needs development) 4. Improves **public outreach** (by e.g. adding tags about mapathons)  5. Improves **multi-language support** (by e.g. adding language in which note description is written)  @tyrasd @westnordost @Zverik @ToeBee @angoca @talllguy what do you think about this PR? Thanks, Nenad. -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5344#issuecomment-2491555177 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
