Skip to content
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

Introduce ActiveRecord like methods to Quickbooks Models #330

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

kashif-umair
Copy link
Contributor

@kashif-umair kashif-umair commented Sep 17, 2016

This PR is an improvement to previous PR that was merged but then reverted because of an issue. The previous PR is #264

  • This PR introduces some methods i.e. all, where, find_by, find, count, save, first methods to quickbooks models. This change eliminates the requirement of using services to query something from Quickbooks. You can directly call Quickbooks::Models::Invoice.all to get all the invoices.
  • Another setting allow_pagination is introduced in this PR. This is a boolean setting which is set to true by default. If user sets this setting to false then no pagination will be applied and all the records will be returned on any query.

  • The golbal setting allow_pagination has been removed which I introduced in my last commit. Instead of that, I've introduced another option to skip pagination on each request basis i.e. skip_pagination. This setting will be available in both the conventional query method and the new methods introduced in this PR.
  • I'm working on updating the README.md file.

Copy link
Contributor

@drewish drewish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also seems like the new code needs some specs


# where
Quickbooks::Model::Customer.where('') #=> runs default query and returns first 20 customers
Quickbooks::Model::Customer.where(id: 170) #=> runs a conditional query and returns first 20 customers having id equal to 170
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem like a great example because you shouldn't have multiple customers with the same id.

# where
Quickbooks::Model::Customer.where('') #=> runs default query and returns first 20 customers
Quickbooks::Model::Customer.where(id: 170) #=> runs a conditional query and returns first 20 customers having id equal to 170
Quickbooks::Model::Customer.where(id: 170, skip_pagination: true) #=> runs the conditional query and returns all customers having id equal to 170 without pagination
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, would be better to use a more realistic value to demonstrate the filtering. I'm not clear what "without pagination" means? The pagination will happen behind inside that method call right?

The general approach is you first instantiate a `Service` object based on the entity you would like to retrieve. Lets retrieve a list of Customers:
There are two approaches to retrieve results from Quickbooks. However, you're encouraged to use the first approach as we'll be removing the second approach in the long run.

###### Approach 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it should be a heading level 3, e.g. ###

end

def find_by(options = {})
where(options).first
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like this should do a limit 1 to avoid fetching and parsing records that aren't used.

end

def first
all.first
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this really be calling all? seems wasteful if you just want one record

end

module ClassMethods
def all(query = nil, options = {})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants