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

Extract Division from request @ financialtransaction/Transactions #83

Merged
merged 7 commits into from Oct 24, 2016
Merged

Extract Division from request @ financialtransaction/Transactions #83

merged 7 commits into from Oct 24, 2016

Conversation

WietseWind
Copy link
Contributor

@WietseWind WietseWind commented Oct 24, 2016

Fix for division number in URL: redundant when filtering on Division eq 1337.

Improved version of fix in pull request 82. Prevents users to use the setDivision method when already filtering for a division ID.

Use case: when not using PHP, but PHP (this class) is still being used (under the hood). This is de case for nodum aPaas when doing something like this:

{% for division in [ 136107, 84319, 84324, 128622, 84927 ] %}
    {% set inkoopboek = api.Exact_Online.getTransactions( 
        "Division eq " ~ division ~ " and FinancialYear eq 2016 and JournalDescription eq 'Inkoopboek'",
        '', 'Date', {} 
    ) %}

    <h3>{{ division }}</h3>
    {{ inkoopboek|table }}
{% endfor %}

@stephangroen
Copy link
Member

Thanks @WietseWind

Your code only applies this for financialtransaction/Transactions, but it seems to be possible for all endpoints with a Division, am I right?

If that is the case we could change this to be generally applicable to prevent inconsistent behaviour across entities.

@WietseWind
Copy link
Contributor Author

@stephangroen It is, indeed. What would you propose? Checking for $this->url !== 'current/Me' in Findable, just as the check in the (Connection) get method when invoking the formatUrl method?

@stephangroen
Copy link
Member

I think that should do the trick, Me is the only exceptional entity to this rule.

Could you also change the if statements to not be one-liners (include brackets)? This is more in line with the code style.

@kvij
Copy link
Contributor

kvij commented Oct 24, 2016

I do not know when'current/Me' is used but is it not enough to check $this->isFillable('Division') && preg_match...? This should also improve performance as the regexp is only executed on entities that have a Division attribute.

@WietseWind
Copy link
Contributor Author

@stephangroen Fixed coding style
@kvij Done, better indeed.

@stephangroen stephangroen merged commit a0a1dc5 into picqer:master Oct 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants