-
Notifications
You must be signed in to change notification settings - Fork 920
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
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:
|
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. |
22c4ef9
to
1c14d7f
Compare
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 (@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, |
1c14d7f
to
9950cd4
Compare
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 and GPX files.
Improved API::NotesController#create to support creating tags (if passed). Also, added adding "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 and GPX outputs (with tags) when note (and tags) are created on FactoryBot / ActiveRecord level.
Added testing of note JSON outputs (with tags) when note (and tags) are created by sending POST/HTTP request.
9950cd4
to
f2c0a06
Compare
Description
PR adds support for note tags as described in #5294 Following changes are made:
note_tags
table, connected it with note table (using foreign key and associations), createdNoteTag
model file andNoteTagTest
class with basic tests (PR Adding map note tags - part 1 - added migration script and model files #5323 does this)browse/tag_details
partial, also, created test with multiple tags added rendering and checking if rendered HTML contains tagscreated_by, OpenStreetMaps-Website
for 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)