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

Enhanced the notes search endpoint #1955

Merged
merged 9 commits into from Oct 11, 2018

Conversation

Projects
None yet
5 participants
@ENT8R
Contributor

ENT8R commented Aug 25, 2018

Follow-up of #1953 and #1954:
The search πŸ” endpoint was enhanced to add the following features πŸŽ‰:

  • get notes of a specific user πŸ‘¨ (either by the display_name πŸ“› or the id πŸ†”)
  • get notes of a specific time ⌚️ range (if no to (end time) parameter was given, it is automatically set to the current time)
  • get the latest notes πŸ“ if no parameter at all was specified

I still could not figure out why the query filter does not work for the notes of a specific user πŸ€·β€β™‚οΈ... (see the code πŸ’» for more information ℹ️) Maybe someone else knows the issue just by looking at the code πŸ˜ƒ...
This PR would fix #1194 and it is at least a step towards #832...

ENT8R added some commits Aug 25, 2018

@Nakaner

This comment has been minimized.

Nakaner commented Aug 27, 2018

Did you take privacy in account and limit searching by user ID and display name to authenticated users?

@ENT8R

This comment has been minimized.

Contributor

ENT8R commented Aug 27, 2018

No, not yet... Should I do this?

@tomhughes

This comment has been minimized.

Member

tomhughes commented Aug 27, 2018

I don't see why - we don't do it for any other APIs currently.

That may change as a result of GDPR but that can be handled as and when we have a final decision on what needs to be done for that.

@@ -876,6 +876,45 @@ def test_search_success
end
end
def test_search_by_user_success

This comment has been minimized.

@GaspardPO

GaspardPO Aug 27, 2018

I see the unit test for display name, but is there also a unit test for filter by id?

This comment has been minimized.

@ENT8R

ENT8R Aug 27, 2018

Contributor

Nope. Also not yet πŸ˜„ This might be the reason why the coverage decreased... πŸ”½
I will add another test.

This comment has been minimized.

@ENT8R

ENT8R Aug 27, 2018

Contributor

I've added some more tests with the last commit

@ENT8R ENT8R referenced this pull request Aug 27, 2018

Closed

Combine list and map view #25

ENT8R added some commits Oct 9, 2018

@ENT8R

This comment has been minimized.

Contributor

ENT8R commented Oct 9, 2018

I fixed now all remaining issues, all checks are finishing successful, so is there anything which prevents it from being merged or is everything done?

@notes = @notes.joins(:comments).where("to_tsvector('english', note_comments.body) @@ plainto_tsquery('english', ?)", params[:q])
if @user
@notes = @user.notes
@notes = closed_condition(@notes)

This comment has been minimized.

@tomhughes

tomhughes Oct 9, 2018

Member

I'd merge these into one line so that it follows the same pattern as the all notes case.

@notes = closed_condition(@notes)
elsif params[:display_name] || params[:id]
# Return an error message because obviously the user could not be found
raise OSM::APIBadUserInput, "The user could not be found"

This comment has been minimized.

@tomhughes

tomhughes Oct 9, 2018

Member

I think we should split this in two and make the text User XXX not known where XXX is the id or name we searched for. The comment isn't really necessary as it just states what is obvious from the error.

if params[:q]
@notes = @notes.joins(:comments)
@notes = if @user
@notes.where("to_tsvector('english', comments_notes.body) @@ plainto_tsquery('english', ?)", params[:q])

This comment has been minimized.

@tomhughes

tomhughes Oct 9, 2018

Member

Does the file really change name to comment_notes in this case? That is kind of worrying and makes this part of the code very messy... I wonder if we should always start from Note.all and then filter by user instead of coming at the notes from the user - that should hopefully avoid having to make this condition extra complicated.

This comment has been minimized.

@tomhughes

tomhughes Oct 9, 2018

Member

I definitely think there is a case for starting form Note.all and then using doing the user filter as .joins(:comments).where(:note_comments => { :author => @user }) so that we can do this condition in one consistent way.

This comment has been minimized.

@ENT8R

ENT8R Oct 10, 2018

Contributor

and then using doing the user filter as .joins(:comments).where(:note_comments => { :author => @user })

Well, but there is no author existing in the table: https://github.com/openstreetmap/openstreetmap-website/blob/master/app/models/note_comment.rb#L9-L10 we can only search by author_ip or author_id... Is the latter one maybe achievable through .joins(:comments).where(:note_comments => { :author_id => @user.id })?

This comment has been minimized.

@tomhughes

tomhughes Oct 10, 2018

Member

Sorry, that's what I meant. I was playing with with various things and copied the wrong one - you don't need .id on the user object though as rails will do that anyway.

end
rescue ArgumentError
# return a more generic error so that everybody knows what is wrong
raise OSM::APIBadUserInput, "The date is in a wrong format"

This comment has been minimized.

@tomhughes

tomhughes Oct 9, 2018

Member

Which date? You've parsed two dates but as there both in the same block you have no idea which one failed... This should probably be split up so the error can be more helpful like Date XXX not recognised or something.

raise OSM::APIBadUserInput, "The date is in a wrong format"
end
@notes = @notes.where("(created_at > '#{from}' AND created_at < '#{to}')")

This comment has been minimized.

@tomhughes

tomhughes Oct 9, 2018

Member

There's no need for parentheses here and in any case there's no need to resort to raw SQL as you can do where(:created_at => from .. to) instead.

# Find the notes we want to return
@notes = @notes.order("updated_at DESC").limit(result_limit).preload(:comments)
@notes = @notes.order("updated_at DESC").distinct.limit(result_limit).preload(:comments)

This comment has been minimized.

@tomhughes

tomhughes Oct 9, 2018

Member

Why was a distinct sort needed here? Under what conditions do we get duplicates where we didn't before?

ENT8R added some commits Oct 10, 2018

@mmd-osm

This comment has been minimized.

Contributor

mmd-osm commented Oct 11, 2018

@ENT8R : please take a moment to update the documentation on the Wiki page once this gets merged.

@ENT8R

This comment has been minimized.

Contributor

ENT8R commented Oct 11, 2018

Yepp, that's on my list...

@tomhughes tomhughes merged commit caef582 into openstreetmap:master Oct 11, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 84.882%
Details
@tomhughes

This comment has been minimized.

Member

tomhughes commented Oct 11, 2018

Merged - note that I renamed the id parameter to user as it would be very confusing to have id mean a user ID in a notes method! It also makes it consistent with changesets. Sorry I didn't notice that before.

@ENT8R

This comment has been minimized.

Contributor

ENT8R commented Oct 11, 2018

Thank you very much πŸŽ‰

@ENT8R ENT8R deleted the ENT8R:notes-search branch Oct 13, 2018

@ENT8R

This comment has been minimized.

Contributor

ENT8R commented Oct 13, 2018

@ENT8R : please take a moment to update the documentation on the Wiki page once this gets merged.

I updated it now: https://wiki.openstreetmap.org/wiki/API_v0.6#Search_for_notes%3A_GET_.2Fapi.2F0.6.2Fnotes.2Fsearch

@ENT8R

This comment has been minimized.

Contributor

ENT8R commented Oct 13, 2018

BTW: How long does it take that the changes are live on https://www.openstreetmap.org and not just the testing APIs?

@tomhughes

This comment has been minimized.

Member

tomhughes commented Oct 13, 2018

Half an hour once I deploy it, which I just did.

@mmd-osm

This comment has been minimized.

Contributor

mmd-osm commented Oct 13, 2018

@ENT8R : thanks for updating the Wiki! One thing I'm not entirely sure of it the date format. https://ruby-doc.org/stdlib-2.1.1/libdoc/time/rdoc/Time.html mentions some heuristic to detect date format. However, it would probably be better to recommend one format in the documentation, e.g. iso8601.

By the way, you should be able to provide a full timestamp down to the second, rather than just some day. of the year (yyyy-mm-dd).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment