-
Notifications
You must be signed in to change notification settings - Fork 183
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
[RED-1821] Cater for endpoints with CBP support #548
Conversation
7106a9d
to
6253045
Compare
spec/live/cbp_support.rb
Outdated
describe '/groups/{id}/memberships' do | ||
before do | ||
VCR.use_cassette("cbp_group_memberships_all_groups") do | ||
@groups = client.groups.fetch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use let
instead of instance vars?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done it ✅
spec/core/cbp_helper.rb
Outdated
@response_body = collection.response.body | ||
end | ||
end | ||
it 'expects an array with the correct element types' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe?
it 'expects an array with the correct element types' do | |
it "returns a list of #{@resource_klass}" do |
I think in this context it should be available, you're inside a method....
However, I'm not a fan of extracting whole tests in methods. But I can't verbalize that, so maybe it's a subjective thing. I'm ok with a bit of duplication in the code, or using a list of test params like we do in Go.
The context could also use some more wording.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done it ✅
also using the described_class var provided by rspec, much nicer
spec/core/cbp_helper.rb
Outdated
def expect_cbp_response_for(collection) | ||
context collection.path.to_s, :vcr do | ||
before do | ||
@resource_klass = collection.instance_variable_get(:@resource_class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any way around poking inside the private variables of classes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, this was taken care of
lib/zendesk_api/collection.rb
Outdated
@@ -365,7 +365,8 @@ def intentional_obp_request? | |||
end | |||
|
|||
def supports_cbp? | |||
@resource_class.const_defined?(:CBP_ACTIONS) && @resource_class.const_get(:CBP_ACTIONS).any? { |supported_path| path.end_with?(supported_path) } | |||
@resource_class.const_defined?(:CBP_ACTIONS) && | |||
@resource_class.const_get(:CBP_ACTIONS).any? { |supported_path_regex| path.match?(supported_path_regex) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this was a class method in Resource, returning an empty array, this part would simplify to:
@resource_class.cbp_path_regexes.any? { |r| r =~ path }
Note that now if the type of data a method returns changes, likely the name needs to reflect that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done ✅
I created a module for the CBP support code. was that what you were thinking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no... the idea was to create a method in resource, returning an empty array, and override the method in the specific resources.
I'm ok with what we have but if you're curious we can chat about it 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ This is great work, see my latest comments and if there's anything you'd like to implement. When you're happy with it, I'm happy too.
a3f0305
to
4505ac7
Compare
lib/zendesk_api/cbp_support.rb
Outdated
def cbp_path_regexes | ||
const_defined?(:CBP_ACTIONS) ? const_get(:CBP_ACTIONS) : [] | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this overcomplicates things, too much indirection.
lib/zendesk_api/resources.rb
Outdated
@@ -378,7 +382,7 @@ class Ticket < Resource | |||
extend UpdateMany | |||
extend DestroyMany | |||
|
|||
CBP_ACTIONS = %w[tickets].freeze | |||
CBP_ACTIONS = [/tickets$/].freeze |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CBP_ACTIONS = [/tickets$/].freeze | |
def self.cbp_path_regexes | |
[/tickets$/].freeze | |
end |
before: body["meta"]["before_cursor"], | ||
after: body["meta"]["after_cursor"] | ||
) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's always great to see files get smaller 😄
lib/zendesk_api/pagination.rb
Outdated
def each_page!(*args, &block) | ||
warn "ZendeskAPI::Collection#each_page! is deprecated, please use ZendeskAPI::Collection#all!" | ||
all!(*args, &block) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this (and below) is not pagination logic?
lib/zendesk_api/pagination.rb
Outdated
@@ -0,0 +1,107 @@ | |||
module ZendeskAPI | |||
# Contains all methods related to pagination in an attempt to slim down collection.rb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: If you wanted to be super-clear about the fact it's only for collection, you could even namespace it by module Collection
here. That way, it wouldn't pollute the namespace in ZendeskAPI
when it should only be used in Collection
.
) | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Scrolling through the file you can kinda understand how pagination is handled. 👍
def last_page? | ||
!@next_page || @next_page == @query | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving public methods make is a bit harder to follow in case they're called from outside of Collection, but in this case I'm fine with it. 👍
lib/zendesk_api/resources.rb
Outdated
end | ||
|
||
class Group < Resource | ||
has_many :memberships, :class => GroupMembership, :path => "memberships" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor, but why not? 😄
has_many :memberships, :class => GroupMembership, :path => "memberships" | |
has_many :memberships, class: GroupMembership, path: "memberships" |
|
||
describe 'Endpoints that support CBP' do | ||
describe ZendeskAPI::Group do | ||
describe '/groups' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: I think RSpec is about describing Object#methods
. This makes the test suite into documentation. IE: describe "#fetch"
https://www.betterspecs.org/#describe
RSpec.describe ZendeskAPI::Group do
subject(:collection) { client.groups }
describe '#fetch' do
it_behaves_like 'an action that uses CBP'
end
end
fc3f2eb
to
df417ff
Compare
Description
Adding endpoints that support CBP.