-
Notifications
You must be signed in to change notification settings - Fork 8
Add proper #all implementation #157
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
base: development
Are you sure you want to change the base?
Changes from all commits
9be0479
d90e9ac
a932c55
0586b8c
f84639d
035e4f0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require 'fortnox/api/mappers/base' | ||
|
|
||
| module Fortnox | ||
| module API | ||
| module Mapper | ||
| class Metadata < Fortnox::API::Mapper::Base | ||
| KEY_MAP = { | ||
| total_resources: '@TotalResources', | ||
| total_pages: '@TotalPages', | ||
| current_page: '@CurrentPage' | ||
| }.freeze | ||
| JSON_ENTITY_WRAPPER = 'MetaInformation' | ||
| end | ||
|
|
||
| Registry.register(Metadata.canonical_name_sym, Metadata) | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require 'fortnox/api/types' | ||
|
|
||
| module Fortnox | ||
| module API | ||
| module Model | ||
| class Metadata < Model::Base | ||
| STUB = { | ||
| current_page: '', | ||
| total_resources: '', | ||
| total_pages: '' | ||
| }.freeze | ||
|
|
||
| attribute :current_page, Types::Required::Integer.is(:read_only) | ||
| attribute :total_resources, Types::Required::Integer.is(:read_only) | ||
| attribute :total_pages, Types::Required::Integer.is(:read_only) | ||
| end | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,8 +7,15 @@ module API | |
| module Repository | ||
| module Loaders | ||
| def all | ||
| response_hash = get(self.class::URI) | ||
| instantiate_collection_response(response_hash) | ||
| results = [] | ||
|
|
||
| loop do | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Something like this might be a little clearer: # In the pagination module
def pages
(1..total_pages ).to_a # Returns an array with [1,2,...,n] where n == total_pages
end
# In this file
def all
pages.each_with_object([]) do |page_number, results|
response_hash = get(self.class::URI, query: { page: page_number })
results.push( *instantiate_collection_response( response_hash ))
end
endThat way we don't have to do bounds checking, so we can drop
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm.. I like that way that looks but that doesn't solve for the inital request to get the number of total pages.. I guess we could do one request and then only start paginating if there are more pages but that makes it kind of ugly.. Any suggestions?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My updated suggestion: def all( limit: -1 )
# Cast and clamp to natural number
requested_number_of_entities = Integer(limit)
limit_imposed = requested_number_of_entities.positive?
# Max 500 entities per request
request_limit = limit_imposed ? [requested_number_of_entities,500].max : 500
# Start at the first page
page = 1
results = []
loop do
response_hash = get(self.class::URI, query: { page: page, limit: request_limit })
results.push( *instantiate_collection_response( response_hash ))
total_pages = response_hash['MetaInformation']['@TotalPages']
limit_reached = limit_imposed && results.length >= requested_number_of_entities
no_more_pages = total_pages == page
break if limit_reached or no_more_pages
page += 1
end
results = results[0...requested_number_of_entities] unless requested_number_of_entities.negative?
return results
end
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [I updated it a bit after I ran it locally and got some errors] That code works for getting all when < 500 as well as limiting the result to the requested limit. I tested pagination by setting the The test code I used, after starting the debug console with reload!
Fortnox::API.configure do |config|
config.client_secret = '[SECRET]'
config.access_token = '[UUID]'
end
r = Fortnox::API::Repository::Invoice.new
r.all.lengthSometimes with a |
||
| response_hash = get(self.class::URI, query: { page: next_page }) | ||
| results += instantiate_collection_response(response_hash) | ||
| break if last_page? | ||
| end | ||
|
|
||
| results | ||
| end | ||
|
|
||
| def only(filter) | ||
|
|
@@ -53,6 +60,9 @@ def escape(key, value) | |
| private | ||
|
|
||
| def instantiate_collection_response(response_hash) | ||
| metadata_hash = @metadata_mapper.wrapped_json_hash_to_entity_hash(response_hash) | ||
| @metadata = Model::Metadata.new(metadata_hash) | ||
|
|
||
| entities_hash = @mapper.wrapped_json_collection_to_entities_hash(response_hash) | ||
| entities_hash.map do |entity_hash| | ||
| instantiate(entity_hash) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module Fortnox | ||
| module API | ||
| module Repository | ||
| module Pagination | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should drop this and create a proper class for the metadata if we want methods on it instead. Right now the @metadata is an instance of the metadata model, that is fine. But the pagination shouldn't live on the repository singleton. The way me current example looks it keeps all that in the single call. Your implementation leaves things on the global repository object instead .... A better place for it would be the collection we are building, But for now I think it is ok to just stick it in the main method. Check this for more on why I think that: #149 (base gem work can be found at https://github.com/Accodeing/rest_easy) |
||
| def last_page? | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No longer needed. |
||
| current_page == total_pages | ||
| end | ||
|
|
||
| def next_page | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No longer needed. |
||
| current_page + 1 | ||
| end | ||
|
|
||
| def current_page | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No longer needed. |
||
| metadata&.current_page || 0 | ||
| end | ||
|
|
||
| def total_pages | ||
| metadata&.total_pages || 0 | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require 'spec_helper' | ||
| require 'fortnox/api' | ||
| require 'fortnox/api/mappers/metadata' | ||
| require 'fortnox/api/mappers/examples/mapper' | ||
|
|
||
| describe Fortnox::API::Mapper::Metadata do | ||
| key_map = Fortnox::API::Mapper::Metadata::KEY_MAP | ||
| json_entity_type = 'MetaInformation' | ||
|
|
||
| it_behaves_like 'mapper', key_map, json_entity_type, nil do | ||
| let(:mapper) { described_class.new } | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require 'spec_helper' | ||
| require 'fortnox/api/models/metadata' | ||
| require 'fortnox/api/models/examples/model' | ||
|
|
||
| describe Fortnox::API::Model::Metadata, type: :model do | ||
| let(:required_attributes) do | ||
| { | ||
| current_page: 1, | ||
| total_resources: 2, | ||
| total_pages: 1 | ||
| } | ||
| end | ||
|
|
||
| it 'can be initialized' do | ||
| expect { described_class.new(required_attributes) }.not_to raise_error | ||
| end | ||
| end |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Did you mean
<here or<=? This will keep it to the latest0.12.x, unclear if that was the intention :)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 actually meant
<.0.13.0introduces breaking changes and I didn't want the upgrade to be part of this PR. See https://github.com/dry-rb/dry-types/blob/master/CHANGELOG.md#v0130-2018-05-03