Merge pull request #546 from zendesk/fvilela/add_cbp_support

[RED-1821] Remove CBP attempt and specify endpoints that support CBP

Author: fbvilela

lib/zendesk_api/client.rb

@@ -37,24 +37,25 @@ class Client
     def method_missing(method, *args, &block)
       method = method.to_s
       options = args.last.is_a?(Hash) ? args.pop : {}
-
       unless config.use_resource_cache
-        raise "Resource for #{method} does not exist" unless method_as_class(method)
-        return ZendeskAPI::Collection.new(self, method_as_class(method), options)
+        resource_class = resource_class_for(method)
+        raise "Resource for #{method} does not exist" unless resource_class
+        return ZendeskAPI::Collection.new(self, resource_class, options)
       end
 
       @resource_cache[method] ||= { :class => nil, :cache => ZendeskAPI::LRUCache.new }
       if !options.delete(:reload) && (cached = @resource_cache[method][:cache].read(options.hash))
         cached
       else
-        @resource_cache[method][:class] ||= method_as_class(method)
+        @resource_cache[method][:class] ||= resource_class_for(method)
         raise "Resource for #{method} does not exist" unless @resource_cache[method][:class]
         @resource_cache[method][:cache].write(options.hash, ZendeskAPI::Collection.new(self, @resource_cache[method][:class], options))
       end
     end
 
     def respond_to?(method, *args)
-      ((cache = @resource_cache[method]) && cache[:class]) || !method_as_class(method).nil? || super
+      cache = @resource_cache[method]
+      !!(cache.to_h[:class] || resource_class_for(method) || super)
     end
 
     # Returns the current user (aka me)
@@ -184,9 +185,9 @@ def build_connection
 
     private
 
-    def method_as_class(method)
-      klass_as_string = ZendeskAPI::Helpers.modulize_string(Inflection.singular(method.to_s.gsub(/\W/, '')))
-      ZendeskAPI::Association.class_from_namespace(klass_as_string)
+    def resource_class_for(method)
+      resource_name = ZendeskAPI::Helpers.modulize_string(Inflection.singular(method.to_s.gsub(/\W/, '')))
+      ZendeskAPI::Association.class_from_namespace(resource_name)
     end
 
     def check_url

lib/zendesk_api/collection.rb

@@ -348,6 +348,14 @@ def cbp_response?(body)
       !!(body["meta"] && body["links"])
     end
 
+    def set_cbp_options
+      @options_per_page_was = @options.delete("per_page")
+      # Default to CBP by using the page param as a map
+      @options.page = { size: (@options_per_page_was || DEFAULT_PAGE_SIZE) }
+    end
+
+    # CBP requests look like: `/resources?page[size]=100`
+    # OBP requests look like: `/resources?page=2`
     def cbp_request?
       @options["page"].is_a?(Hash)
     end
@@ -356,30 +364,23 @@ def intentional_obp_request?
       Helpers.present?(@options["page"]) && !cbp_request?
     end
 
-    def should_try_cbp?(path_query_link)
-      not_supported_endpoints = %w[show_many]
-      not_supported_endpoints.none? { |endpoint| path_query_link.end_with?(endpoint) }
+    def supports_cbp?
+      @resource_class.const_defined?(:CBP_ACTIONS) && @resource_class.const_get(:CBP_ACTIONS).any? { |supported_path| path.end_with?(supported_path) }
+    end
+
+    def first_cbp_request?
+      # @next_page will be nil when making the first cbp request
+      @next_page.nil?
     end
 
     def get_resources(path_query_link)
       if intentional_obp_request?
         warn "Offset Based Pagination will be deprecated soon"
-      elsif @next_page.nil? && should_try_cbp?(path_query_link)
-        @options_per_page_was = @options.delete("per_page")
-        # Default to CBP by using the page param as a map
-        @options.page = { size: (@options_per_page_was || DEFAULT_PAGE_SIZE) }
-      end
-
-      begin
-        # Try CBP first, unless the user explicitly asked for OBP
-        @response = get_response(path_query_link)
-      rescue ZendeskAPI::Error::NetworkError => e
-        raise e if intentional_obp_request?
-        # Fallback to OBP if CBP didn't work, using per_page param
-        @options.per_page = @options_per_page_was
-        @options.page = nil
-        @response = get_response(path_query_link)
+      elsif supports_cbp? && first_cbp_request?
+        # only set cbp options if it's the first request, otherwise the options would be already in place
+        set_cbp_options
       end
+      @response = get_response(path_query_link)
 
       # Keep pre-existing behaviour for search/export
       if path_query_link == "search/export"
@@ -394,9 +395,7 @@ def set_page_and_count(body)
       @next_page, @prev_page = page_links(body)
 
       if cbp_response?(body)
-        # We'll delete the one we don't need in #next or #prev
-        @options["page"]["after"] = body["meta"]["after_cursor"]
-        @options["page"]["before"] = body["meta"]["before_cursor"]
+        set_cbp_response_options(body)
       elsif @next_page =~ /page=(\d+)/
         @options["page"] = $1.to_i - 1
       elsif @prev_page =~ /page=(\d+)/
@@ -541,9 +540,13 @@ def array_method(name, *args, &block)
       to_a.public_send(name, *args, &block)
     end
 
+    # If you call client.tickets.foo - and foo is not an attribute nor an association, it ends up here, as a new collection
     def next_collection(name, *args, &block)
       opts = args.last.is_a?(Hash) ? args.last : {}
-      opts.merge!(:collection_path => @collection_path.dup.push(name))
+      opts.merge!(collection_path: [*@collection_path, name], page: nil)
+      # why page: nil ?
+      # when you do client.tickets.fetch followed by client.tickets.foos => the request to /tickets/foos will
+      # have the options page set to whatever the last options were for the tickets collection
       self.class.new(@client, @resource_class, @options.merge(opts))
     end
 
@@ -565,5 +568,16 @@ def assert_results(results, body)
       return if results
       raise ZendeskAPI::Error::ClientError, "Expected #{@resource_class.model_key} or 'results' in response keys: #{body.keys.inspect}"
     end
+
+    def set_cbp_response_options(body)
+      @options.page = {} unless cbp_request?
+      # the line above means an intentional CBP request where page[size] is passed on the query
+      # this is to cater for CBP responses where we don't specify page[size] but the endpoint
+      # responds CBP by default. i.e  `client.trigger_categories.fetch`
+      @options.page.merge!(
+        before: body["meta"]["before_cursor"],
+        after: body["meta"]["after_cursor"]
+                           )
+    end
   end
 end

lib/zendesk_api/resources.rb

@@ -378,6 +378,8 @@ class Ticket < Resource
     extend UpdateMany
     extend DestroyMany
 
+    CBP_ACTIONS = %w[tickets].freeze
+
     # Unlike other attributes, "comment" is not a property of the ticket,
     # but is used as a "comment on save", so it should be kept unchanged,
     # See https://github.com/zendesk/zendesk_api_client_rb/issues/321

spec/core/collection_spec.rb

@@ -26,14 +26,14 @@
     let(:response_page1) { double("response_page1", body: response_body_page1) }
 
     before do
-      allow(subject).to receive(:get_response).with("test_resources").and_return(response_page1)
+      stub_json_request(:get, %r{test_resources}, json(response_body_page1))
     end
 
     context "when CBP is used" do
       it "is empty on the first page" do
-        subject.fetch
-
-        expect(subject.prev).to eq([])
+        stub_const("ZendeskAPI::TestResource::CBP_ACTIONS", %w[test_resources])
+        collection = ZendeskAPI::Collection.new(client, ZendeskAPI::TestResource)
+        expect(collection.prev).to eq([])
       end
     end
   end
@@ -543,6 +543,34 @@ def to_proc
         expect(subject.fetch(true)).to be_empty
       end
     end
+
+    context "when the request returns a Cursor Based Pagination type of response" do
+      let(:cbp_response) do
+        {
+          "meta" => {
+            "has_more" => true,
+            "after_cursor" => 'after_cursor',
+            "before_cursor" => 'before_cursor'
+          },
+          "links" => {
+            "next" => "next_page",
+            "prev" => "previous_page"
+          },
+          "test_resources" => [{ "id" => 1 }]
+        }
+      end
+      before do
+        stub_json_request(:get, %r{test_resources}, json(cbp_response))
+      end
+
+      it "sets the next and previous pages and cursors" do
+        subject.fetch
+        expect(subject.instance_variable_get(:@next_page)).to eq("next_page")
+        expect(subject.instance_variable_get(:@prev_page)).to eq("previous_page")
+        expect(subject.options.page.after).to eq("after_cursor")
+        expect(subject.options.page.before).to eq("before_cursor")
+      end
+    end
   end
 
   context "save" do
@@ -1015,8 +1043,9 @@ def to_proc
       end
     end
 
-    context "when fetching a collection" do
+    context "when fetching a collection that supports CBP" do
       before do
+        stub_const("ZendeskAPI::TestResource::CBP_ACTIONS", %w[test_resources])
         expect(subject).to receive(:get_response).with("test_resources").and_return(cbp_success_response)
       end
 
@@ -1033,18 +1062,6 @@ def to_proc
       end
     end
 
-    context "when the endpoint does not support CBP" do
-      before do
-        expect(subject).to receive(:get_response).with("test_resources").and_raise(ZendeskAPI::Error::NetworkError).twice
-      end
-
-      it "tries an OBP request after a failed CBP request" do
-        subject.per_page(2).fetch
-        expect(subject.instance_variable_get(:@options)["per_page"]).to eq(2)
-        expect(subject.instance_variable_get(:@options)["page"]).to be_nil
-      end
-    end
-
     describe "#all going through multiple pages" do
       def generate_response(index, has_more)
         {
@@ -1061,6 +1078,7 @@ def generate_response(index, has_more)
         }
       end
       before do
+        stub_const("ZendeskAPI::TestResource::CBP_ACTIONS", %w[test_resources])
         stub_json_request(:get, %r{test_resources\?page%5Bsize%5D=1}, json(generate_response(1, true)))
         stub_json_request(:get, %r{test_resources.json\/\?page%5Bafter%5D=after1&page%5Bsize%5D=1}, json(generate_response(2, true)))
         stub_json_request(:get, %r{test_resources.json\/\?page%5Bafter%5D=after2&page%5Bsize%5D=1}, json(generate_response(3, true)))
@@ -1069,12 +1087,12 @@ def generate_response(index, has_more)
 
       it "fetches all pages and yields the correct arguments" do
         expect do |b|
-          silence_logger { subject.all(&b) }
+          silence_logger { subject.per_page(1).all(&b) }
         end.to yield_successive_args(
-          [ZendeskAPI::TestResource.new(client, id: 1), "after" => "after1", "before" => "before0", "size" => 100],
-          [ZendeskAPI::TestResource.new(client, id: 2), "after" => "after2", "before" => "before1", "size" => 100],
-          [ZendeskAPI::TestResource.new(client, id: 3), "after" => "after3", "before" => "before2", "size" => 100],
-          [ZendeskAPI::TestResource.new(client, id: 4), "after" => "after4", "before" => "before3", "size" => 100]
+          [ZendeskAPI::TestResource.new(client, id: 1), "after" => "after1", "before" => "before0", "size" => 1],
+          [ZendeskAPI::TestResource.new(client, id: 2), "after" => "after2", "before" => "before1", "size" => 1],
+          [ZendeskAPI::TestResource.new(client, id: 3), "after" => "after3", "before" => "before2", "size" => 1],
+          [ZendeskAPI::TestResource.new(client, id: 4), "after" => "after4", "before" => "before3", "size" => 1]
         )
       end
     end

spec/live/ticket_spec.rb

@@ -62,6 +62,24 @@ def valid_attributes
     end
   end
 
+  describe "#show_many" do
+    let(:how_many_tickets) { 10 }
+    before do
+      VCR.use_cassette("get_tickets_show_many") do
+        @tickets = client.tickets.per_page(how_many_tickets).fetch
+      end
+
+      VCR.use_cassette("tickets_show_many_with_ids") do
+        @tickets_from_show_many = client.tickets.show_many(ids: @tickets.map(&:id)).fetch
+      end
+    end
+
+    it "returns the correct number of tickets" do
+      expect(@tickets_from_show_many.count).to eq(how_many_tickets)
+      expect(@tickets.map(&:id).sort).to eq(@tickets_from_show_many.map(&:id).sort)
+    end
+  end
+
   describe "#attributes_for_save" do
     let :ticket do
       described_class.new(instance_double(ZendeskAPI::Client), status: :new)