[openstreetmap/openstreetmap-website] Enhanced the notes search endpoint (#1955)

[Tom Hughes]

tomhughes requested changes on this pull request.



>  
-    # Get any conditions that need to be applied
-    @notes = closed_condition(Note.all)
-    @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)

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

>  
-    # Get any conditions that need to be applied
-    @notes = closed_condition(Note.all)
-    @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)
+    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"

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.

> -    @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)
+    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"
+    else
+      @notes = closed_condition(Note.all)
+    end
+
+    # Filter by a given string
+    if params[:q]
+      @notes = @notes.joins(:comments)
+      @notes = if @user
+                 @notes.where("to_tsvector('english', comments_notes.body) @@ plainto_tsquery('english', ?)", params[:q])

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.

> +                 @notes.where("to_tsvector('english', note_comments.body) @@ plainto_tsquery('english', ?)", params[:q])
+               end
+    end
+
+    # Filter by a given start date and an optional end date
+    if params[:from]
+      begin
+        from = Time.parse(params[:from])
+        to = if params[:to]
+               Time.parse(params[:to])
+             else
+               Time.now
+             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"

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.

> +
+    # Filter by a given start date and an optional end date
+    if params[:from]
+      begin
+        from = Time.parse(params[:from])
+        to = if params[:to]
+               Time.parse(params[:to])
+             else
+               Time.now
+             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"
+      end
+
+      @notes = @notes.where("(created_at > '#{from}' AND created_at < '#{to}')")

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)

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

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/1955#pullrequestreview-163031960
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20181009/273d5111/attachment-0001.html>