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

Conversation

nenad-vujicic
Copy link
Contributor

Description

PR adds support for note tags as described in #5294 Following changes are made:

  1. Created note_tags table, connected it with note table (using foreign key and associations), created NoteTag model file and NoteTagTest class with basic tests (PR Adding map note tags - part 1 - added migration script and model files #5323 does this)
  2. Added preparing and rendering of note tags in left sidebar using browse/tag_details partial, also, created test with multiple tags added rendering and checking if rendered HTML contains tags
  3. Updated (j)builder files to write note tags in XML, JSON, GPX, RSS and feed files and updated test to check existence of tags in each of above files (creation of tag is performed using ActiveLayer)
  4. Added support for creating tags at note creation time using POST HTTP request and improved tests to check existence of tags in each of generated files (creation of tag is performed using POST HTTP request). Also, added creating tag created_by, OpenStreetMaps-Website for notes created using OSM website.

Displayed resolved note:
image

Displayed opened note:
image

XML output for "opened note":
image

Displayed note without tags (all current):
image

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)

@nenad-vujicic
Copy link
Contributor Author

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):

image

  1. Improves collaboration & communication (by e.g. marking resolving some note requires outdoor survey):

image

  1. Now we are ready for advanced filtering by specific tags (needs development)

  2. Improves public outreach (by e.g. adding tags about mapathons)

image

  1. Improves multi-language support (by e.g. adding language in which note description is written)

image

@tyrasd @westnordost @Zverik @ToeBee @angoca @talllguy what do you think about this PR?

Thanks,
Nenad.

@westnordost
Copy link

westnordost commented Nov 21, 2024

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:

  • 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)

db/structure.sql Outdated Show resolved Hide resolved
db/migrate/20241030122707_create_note_tags.rb Outdated Show resolved Hide resolved
app/assets/javascripts/index/new_note.js Outdated Show resolved Hide resolved
app/controllers/api/notes_controller.rb Show resolved Hide resolved
@nenad-vujicic nenad-vujicic marked this pull request as draft November 25, 2024 09:52
@nenad-vujicic
Copy link
Contributor Author

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.

@nenad-vujicic nenad-vujicic marked this pull request as ready for review November 26, 2024 20:32
app/controllers/api/notes_controller.rb Show resolved Hide resolved
app/views/api/notes/_note.xml.builder Show resolved Hide resolved
app/models/note_tag.rb Show resolved Hide resolved
db/migrate/20241030122707_create_note_tags.rb Outdated Show resolved Hide resolved
@nenad-vujicic
Copy link
Contributor Author

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,
Nenad.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal for adding support for map notes tags
5 participants