wetcat-studios / fortie

Laravel 5 package for Fortnox API
Apache License 2.0
19 stars 20 forks source link

Improve query functions #51

Closed viktormaar closed 4 years ago

viktormaar commented 4 years ago

Summary: In my daily practice I often need to retrieve either only the recently changed records or all the stored records, but rarest the first page by the default Fortnox limit (which is 100 record/page). This PR helps on this, and also adds some new features for parametering the queries seamlessly.

In order to avoid breaking changes I decided to keep the functionality of all() method, despite the naming is not perfect. So the $fortie->customers()->all(); still retrieves the first page by the default limit

Features:

These modifiers also can be combinated (except for count()). E.g.: Query only the active customers changed in the last 3 hours, sort them by customer number descending, and get all them without pagination.

$fortie->customers()
    ->filter('active')
    ->timespan('-3 hours')
    ->sortBy('CustomerNumber')
    ->sortOrder('DESC')
    ->unlimited()
    ->all();

Notes: I implemented only for the customers and the units by now. Let me know if you as package maintainer are interested on this PR, so I`ll apply it to the rest.

agoransson commented 4 years ago

Apologies for late handling of this - I will get to this a.s.a.p.

agoransson commented 4 years ago

I've finally had time to look at it while the family is sleeping tonight, and I like it. It's a very well needed improvement to the API. The "all()" method is of course poorly designed as you note.

I'm a little bit concerned with the mix of resources though. For example, AbstractResponse could be moved to a move generic place I think - but I'm sure that's what you've got in mind as well.

If you feel like improving the structure before I continue the merge I'd be a very happy "maintainer".

viktormaar commented 4 years ago

Thank you for reviewing! In this case I´m going to apply the changes to the other provider classes. Maybe by traits... 🤔 Exactly the AbstractResponse and the MetaInformation classes what I am also concerned about. I do not like this kind of solution at all, but I wanted to keep the structure of the "pure" Fortnox API response somehow. I´ll refactor it. Changing the PR status back to "Draft" is not possible, so I just continue working on this, as soon as I can.

agoransson commented 4 years ago

Its fine to keep pushing to this PR. We'll merge when we're both happy =)

Don't stress finishing it - work when you have time and energy. I barely have time myself as you notice =)