Api search support #99

masonnl · 2017-04-05T18:48:29Z

Initial thoughts on wrapping Pivotal Tracker's /search endpoint in this gem.

coveralls · 2017-04-05T18:50:36Z

Coverage decreased (-0.06%) to 96.474% when pulling 0233427 on masonnl:api-search-support into 032c0cf on dashofcode:master.

coveralls · 2017-04-05T18:52:15Z

Coverage decreased (-0.06%) to 96.474% when pulling 0233427 on masonnl:api-search-support into 032c0cf on dashofcode:master.

masonnl · 2017-04-05T18:51:47Z

lib/tracker_api/client.rb

@@ -113,7 +116,7 @@ def paginate(path, options = {}, &block)
          if block_given?
            yield(data, @last_response)
          else
-            data.concat(@last_response.body) if @last_response.body.is_a?(Array)
+            data.concat(@last_response.body) if @last_response.body.is_a?(Array) || @last_response.body.is_a?(Hash)
          end
        end
      end


It seems like #paginate is only ever expecting Arrays. I didn't dig in on the repercussions of also allowing Hashes; tests pass.

masonnl · 2017-04-05T18:52:52Z

lib/tracker_api/endpoints/search.rb

+        @client = client
+      end
+
+      def get(project_id, query, params={})


TODO: Add method documentation. This shouldn't be too verbose. Pivotal Tracker allows just about anything.

masonnl · 2017-04-05T18:57:32Z

lib/tracker_api/resources/search_result_container.rb

@@ -0,0 +1,12 @@
+module TrackerApi
+  module Resources
+    class SearchResultContainer


Question 1: Notice this is including the Shared::Base. These resources don't require IDs though, so are there any changes to be made to the top level Base? It seems strange to have Resources without IDs. Am I missing anything?

Observation: I didn't make this a Shared resource since there isn't any attributes that are shared between a top-level SearchResultContainer or EpicsSearchResult or StoriesSearchResult, and this allows endpoints to be made specifically for epic or story search.

Probably makes sense to move the attribute :id and Equalizer code into its own subclass of Base so classes that don't need an id don't have to have one. I think it is fine to leave for now since it doesn't hurt anything and I am working on a major refactor that will change much of this anyway. So not worth the effort now.

jonseaberg · 2017-04-06T01:21:31Z

lib/tracker_api/endpoints/search.rb

+        raise ArgumentError, 'Valid query required to search' unless query.is_a?(String)
+
+        params.key?(:body) ? params[:body][:query] = query : params[:body] = { query: query }
+        data = client.paginate("/projects/#{project_id}/search", params)


The Tracker API docs do not explicitly state the search endpoint is paginated like they do with the stories endpoint. My guess is that the search endpoint is not paginated so you might be able to use a single get here.

Ah you're right. I missed that from the docs. Thanks!

jonseaberg · 2017-04-06T01:24:02Z

lib/tracker_api/endpoints/search.rb

+
+        raise Errors::UnexpectedData, 'Hash of search results expect' unless data.is_a? Hash
+
+        create_search_results(data)


I think Virtus does all the conversion for you so you might be able to return Resources::SearchResultContainer.new(data) here.

jonseaberg · 2017-04-06T01:26:19Z

lib/tracker_api/resources/epics_search_result.rb

+    class EpicsSearchResult
+      include Shared::Base
+
+      attribute :epics, Array


If you specify the type of object in the array Virtus will coerce them for you. attribute :epics, Array[Resources::Epic]

- The search endpoint is not paginated, so use a standard `GET` instead of attempting to send paginated params. - Virtus will coerce responses into specified Resources if the resource is specified in the attribute. This greatly improves readability of `#search` and more clearly documents what we expect to receive from a search request.

coveralls · 2017-04-06T15:15:26Z

Coverage decreased (-0.06%) to 96.474% when pulling 4cc37c0 on masonnl:api-search-support into 032c0cf on dashofcode:master.

coveralls · 2017-04-06T15:15:26Z

Coverage decreased (-0.06%) to 96.474% when pulling 4cc37c0 on masonnl:api-search-support into 032c0cf on dashofcode:master.

coveralls · 2017-04-06T15:15:26Z

Coverage decreased (-0.06%) to 96.474% when pulling 4cc37c0 on masonnl:api-search-support into 032c0cf on dashofcode:master.

coveralls · 2017-04-06T17:01:22Z

Coverage decreased (-0.1%) to 96.404% when pulling 032000d on masonnl:api-search-support into 032c0cf on dashofcode:master.

coveralls · 2017-04-06T17:46:22Z

Coverage increased (+0.1%) to 96.671% when pulling 50ec77f on masonnl:api-search-support into 032c0cf on dashofcode:master.

coveralls · 2017-04-06T17:53:40Z

Coverage increased (+0.1%) to 96.671% when pulling 50ec77f on masonnl:api-search-support into 032c0cf on dashofcode:master.

forest · 2017-04-08T05:24:42Z

lib/tracker_api/resources/search_result_container.rb

@@ -0,0 +1,12 @@
+module TrackerApi
+  module Resources
+    class SearchResultContainer


Probably makes sense to move the attribute :id and Equalizer code into its own subclass of Base so classes that don't need an id don't have to have one. I think it is fine to leave for now since it doesn't hurt anything and I am working on a major refactor that will change much of this anyway. So not worth the effort now.

forest · 2017-04-08T05:25:35Z

test/minitest_helper.rb

@@ -28,7 +28,8 @@
 PT_USER_1 = { username: 'trackerapi1', password: 'trackerapi1', token: 'd55c3bc1f74346b843ca84ba340b29bf', project_id: 1027488, workspace_id: 375106, id: 1266314 }
 PT_USER_2 = { username: 'trackerapi2', password: 'trackerapi2', token: 'ab4c5895f57995bb7547986eacf91160', project_ids: [1027488, 1027492], workspace_id: 581707, id: 1266316 }
 PT_USER_3 = { username: 'trackerapi3', password: 'trackerapi3', token: '77f9b9a466c436e6456939208c84c973', project_id: 1027494 }
-PT_USERS  = [PT_USER_1, PT_USER_2, PT_USER_3]
+PT_USER_4 = { username: 'trackerapi4', password: 'trackerapi4', token: 'b764325e019df5c28896d1aa2fe27225', project_id: 1991595 }


Why create another tracker account? Can you just use one of the existing ones?

It probably makes more sense to use one of the existing users, but when I started writing the test I wanted to search for something in our company's sandbox account so I could see the search coming back with stuff I expected. That said, it would probably work just fine using a search term with one of the other tokens provided that token is still valid and the search term matches something in a project dependant on that token.

You can use any of the 3 accounts. Create a few stories in the account and then search for them. Should be fine.

coveralls · 2017-04-10T18:46:31Z

Coverage increased (+0.1%) to 96.674% when pulling ed5f9e6 on masonnl:api-search-support into 032c0cf on dashofcode:master.

wip on search (working)

0233427

masonnl force-pushed the api-search-support branch from 14add6b to 0233427 Compare April 5, 2017 18:49

masonnl commented Apr 5, 2017

View reviewed changes

masonnl mentioned this pull request Apr 5, 2017

Api seach support masonnl/tracker_api#1

Merged

2 tasks

jonseaberg reviewed Apr 6, 2017

View reviewed changes

Add #search to the project

032000d

masonnl added 3 commits April 6, 2017 10:37

specs for Project#search

23719a7

Add README listing for Project#search

712b77d

revert changes made to Client#paginate

50ec77f

masonnl changed the title ~~[WIP] Api search support~~ Api search support Apr 6, 2017

forest reviewed Apr 8, 2017

View reviewed changes

masonnl added 2 commits April 10, 2017 11:38

revert changes made to Client#paginate

c0f450b

feedback - use an existing project to search

ed5f9e6

forest approved these changes Apr 10, 2017

View reviewed changes

forest merged commit 62caf2a into irphilli:master Apr 10, 2017


		raise Errors::UnexpectedData, 'Hash of search results expect' unless data.is_a? Hash

		create_search_results(data)

Api search support #99

Api search support #99

Uh oh!

Conversation

masonnl commented Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Apr 6, 2017

Uh oh!

coveralls commented Apr 6, 2017

Uh oh!

coveralls commented Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

masonnl Apr 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Apr 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

masonnl commented Apr 5, 2017 •

edited

Loading

coveralls commented Apr 5, 2017 •

edited

Loading

coveralls commented Apr 5, 2017 •

edited

Loading

coveralls commented Apr 6, 2017 •

edited

Loading

coveralls commented Apr 6, 2017 •

edited

Loading

coveralls commented Apr 6, 2017 •

edited

Loading

coveralls commented Apr 6, 2017 •

edited

Loading

masonnl Apr 8, 2017 •

edited

Loading

coveralls commented Apr 10, 2017 •

edited

Loading