Adds note tags support #5344
Adds note tags support #5344
Conversation
|
Here are a few more benefits of adding note tags to OSM, as implemented by this PR:
@tyrasd @westnordost @Zverik @ToeBee @angoca @talllguy what do you think about this PR? Thanks, |
|
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. (Hashtags are not going away anytime soon) 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:
|
db/structure.sql
Outdated
|
|
||
| -- *not* creating schema, since initdb creates it | ||
|
|
||
|
|
There was a problem hiding this comment.
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.
| end | ||
|
|
||
| # Add foreign key without validation | ||
| add_foreign_key :note_tags, :notes, :column => :note_id, :name => "note_tags_id_fkey", :validate => false |
There was a problem hiding this comment.
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 was a problem hiding this comment.
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.
| key, value = pair.split(":", 2) | ||
| tags << { :k => sanitize(key), :v => sanitize(value) } if key && value | ||
| end | ||
| end |
There was a problem hiding this comment.
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.
Added NoteTag model class, note_tags DB table, associations between Note and NoteTag and private / foreign keys.
Added Note#tags routine for preparing note tags for rendering using "browse/tag_details" partial. Also, added displaying of previously prepared note tags between note's "Description" and comments in app/views/notes/show.html.erb.
Added writing note tags in (j)builder files for generating XML, JSON, GPX, RSS and feed files.
Improved API::NotesController#create to support tags (if passed). Also, added adding of "created_by:OpenStreetMap-Website" tag to note when created from OSM website.
Added registering new factory bot for note_tag and added new unit tests to NoteTagTest for checking if key length is valid, value length is valid, key length is invalid, value length is invalid, orphaned tag is invalid and note_tags are unique.
Added NotesControllerTest#test_displaying_note_with_tags unit test for testing cases when note has tags and comment (description). Internally, new note is created with comment (description) and several tags and then rendered in sidebar. Existence of appropriate HTML tags for notes, comment (description) and tags is checked.
Added testing of note XML, JSON, GPX, RSS and feed outputs (with tags) when note (and tags) are created on FactoryBot / ActiveRecord level.
Added testing of note XML, JSON, GPX, RSS and feed outputs (with tags) when note (and tags) are created by sending POST/HTTP request.
nenad-vujicic
force-pushed
the
issue_5294_poc
branch
from
0841bfc
to
22c4ef9
Compare
|
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. |
Description
PR adds support for note tags as described in #5294 Following changes are made:
note_tagstable, connected it with note table (using foreign key and associations), createdNoteTagmodel file andNoteTagTestclass with basic tests (PR Adding map note tags - part 1 - added migration script and model files #5323 does this)browse/tag_detailspartial, also, created test with multiple tags added rendering and checking if rendered HTML contains tagscreated_by, OpenStreetMaps-Websitefor notes created using OSM website.Displayed resolved note:
Displayed opened note:
XML output for "opened note":
Displayed note without tags (all current):
Fixes #5294
How has this been tested?
Run tests from workflows, tested creating and displaying notes manually as administrator / moderator / regular user, tested displaying already existing notes (they will be displayed as earlier - without tags)