remove CBP request attempt in favor of a more descriptive approach about endpoints that do support CBP

Author: fbvilela

lib/zendesk_api/client.rb

@@ -37,24 +37,24 @@ 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[:class]) || !resource_class_for(method).nil? || super
     end
 
     # Returns the current user (aka me)
@@ -184,7 +184,7 @@ def build_connection
 
     private
 
-    def method_as_class(method)
+    def resource_class_for(method)
       klass_as_string = ZendeskAPI::Helpers.modulize_string(Inflection.singular(method.to_s.gsub(/\W/, '')))
       ZendeskAPI::Association.class_from_namespace(klass_as_string)
     end

lib/zendesk_api/collection.rb

@@ -356,30 +356,27 @@ 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_cursor_based_pagination?
+      @resource_class.const_defined?(:CBP_ACTIONS) && @resource_class.const_get(:CBP_ACTIONS).any? { |supported_path| path.end_with?(supported_path) }
+    end
+
+    def first_of_multiple_pages?
+      @next_page.nil?
+    end
+
+    def modify_request_for_cbp
+      @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
 
     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_cursor_based_pagination? && first_of_multiple_pages?
+        modify_request_for_cbp
       end
+      @response = get_response(path_query_link)
 
       # Keep pre-existing behaviour for search/export
       if path_query_link == "search/export"
@@ -394,7 +391,6 @@ 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"]
       elsif @next_page =~ /page=(\d+)/
@@ -541,9 +537,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
 

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
@@ -1015,8 +1015,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 +1034,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 +1050,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 +1059,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)